Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support cross-installing of prebuilt packages via npm_config_* vars #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pimterry
Copy link

@pimterry pimterry commented Aug 5, 2022

See vweevers/win-version-info#29 for context.

With this change, if npm_config_platform or npm_config_arch are set, and either one doesn't match the current machine, then the build isn't tested (since the test result will be wrong) and instead we just check if there's a matching prebuild available. If not, the install fails completely (because we can't easily cross-compile for targets like this).

I've tested this in win-version-info with its win32-only prebuilds, disabling the skip.js test there, on Linux.

Cross-installing for win32:

$ npm_config_platform=win32 npm install

> win-version-info@6.0.1 install
> node-gyp-build


up to date, audited 774 packages in 2s

218 packages are looking for funding
  run `npm fund` for details

=> Correctly notices there's a valid win32 prebuild available, doesn't build anything, all good.

Cross-installing for a target with no prebuilds available:

$ npm_config_platform=win98 npm install

> win-version-info@6.0.1 install
> node-gyp-build

.../node-gyp-build/index.js:60
  throw new Error('No native build was found for ' + target + '\n    loaded from: ' + dir + '\n')
  ^

Error: No native build was found for platform=win98 arch=x64 runtime=node abi=93 uv=1 libc=glibc node=16.16.0
    loaded from: .../win-version-info

    at Function.load.path (.../node-gyp-build/index.js:60:9)
    at Object.<anonymous> (.../node-gyp-build/bin.js:14:24)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47
npm ERR! code 1
npm ERR! path .../win-version-info
npm ERR! command failed
npm ERR! command sh -c node-gyp-build

npm ERR! A complete log of this run can be found in:
npm ERR!     .../.npm/_logs/2022-08-05T12_04_03_956Z-debug-0.log

=> Can't find a prebuild for the target platform, fails explicitly instead of building for the real platform incorrectly.

bin.js Show resolved Hide resolved
bin.js Show resolved Hide resolved
bin.js Outdated Show resolved Hide resolved
bin.js Outdated Show resolved Hide resolved
@vweevers vweevers added the semver-minor New features that are backward compatible label Aug 5, 2022
Co-authored-by: Vincent Weevers <mail@vincentweevers.nl>
bin.js Show resolved Hide resolved
Comment on lines +13 to +14
// Instead, we just check whether a matching prebuild exists, and fail if it doesn't.
require('./index').path()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Instead, we just check whether a matching prebuild exists, and fail if it doesn't.
require('./index').path()
// Instead, we just check whether a matching prebuild exists, and attempt to build if it doesn't.
try {
require('./index').path()
} catch (e) {
if (verbose()) console.error(e)
preinstall()
}

I think it should be attempting to do a build instead of simply failing if there is no prebuild found.
If you are running for the same platform, it is likely that it will be possible to compile the binary.

Perhaps it should only attempt the build if the platforms match?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having too many fallbacks makes behavior unpredictable. This PR makes a very reasonable assumption that if the platform doesn't match, compiling likely won't work. We still have --build-from-source as an escape hatch.

Perhaps it should only attempt the build if the platforms match?

Not sure I understand the question. If the platforms match, then the behavior is the same as before (meaning: we find & test a prebuild, or build from source).

Copy link
Contributor

@Julusian Julusian Dec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having too many fallbacks makes behavior unpredictable. This PR makes a very reasonable assumption that if the platform doesn't match, compiling likely won't work. We still have --build-from-source as an escape hatch.

Then this will be a breaking change. With 4.5.0 I can do a npm_config_arch=arm64 yarn in my project and it compiles one dependency unnecessarily. With this change the same npm_config_arch=arm64 yarn command fails, as one dependency doesn't provide prebuilds for linux-arm64.

I really shouldn't need to specify --build-from-source and force the ci to recompile the multiple other native dependencies that have already have correct prebuilds

Not sure I understand the question. If the platforms match, then the behavior is the same as before (meaning: we find & test a prebuild, or build from source).

I mean that if I am trying to build linux-arm64 from linux-x64, there is a reasonable chance that it will be able to compile it. But if I am trying to build win32-x64 from linux-x64, is it worth even trying to perform that build?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, you were talking about only a platform match, rather than both platform & arch. OK, if we keep this PR as-is (and I think we should, but I want to think about your use case for a bit) then we'll make it semver-major.

@vweevers vweevers added semver-major Changes that break backward compatibility and removed semver-minor New features that are backward compatible labels Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Changes that break backward compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants