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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3dc6a9
Support cross-installing of prebuilt packages via npm_config_* vars
pimterry Aug 5, 2022
2c1cccf
Apply suggestions from code review
pimterry Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions bin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,22 @@ var proc = require('child_process')
var os = require('os')
var path = require('path')

var targetMismatch = (process.env.npm_config_platform && process.platform !== process.env.npm_config_platform) ||
vweevers marked this conversation as resolved.
Show resolved Hide resolved
(process.env.npm_config_arch && process.arch !== process.env.npm_config_arch)
vweevers marked this conversation as resolved.
Show resolved Hide resolved

if (!buildFromSource()) {
proc.exec('node-gyp-build-test', function (err, stdout, stderr) {
if (err) {
if (verbose()) console.error(stderr)
preinstall()
}
})
if (targetMismatch) {
// If the target is mismatched, then we can't test prebuilds.
// Instead, we just check whether a matching prebuild exists, and fail if it doesn't.
require('./index').path()
Comment on lines +13 to +14
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 •
edited
Loading

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.

} else {
proc.exec('node-gyp-build-test', function (err, stdout, stderr) {
vweevers marked this conversation as resolved.
Show resolved Hide resolved
if (err) {
if (verbose()) console.error(stderr)
preinstall()
}
})
}
} else {
preinstall()
}
Expand Down