Allow setting libc to glibc on non-glibc platform #176
Conversation
rc.js
Outdated
| @@ -51,6 +53,8 @@ module.exports = function (pkg) { | |||
|
|
|||
| rc.abi = napi.isNapiRuntime(rc.runtime) ? rc.target : getAbi(rc.target, rc.runtime) | |||
|
|
|||
| rc.libc = rc.libc === detectLibc.GLIBC ? '' : rc.libc | |||
There was a problem hiding this comment.
Could be a breaking change if someone is setting the LIBC env var?
There was a problem hiding this comment.
Thanks! I figured there was another place deeper in the code reading env.LIBC. I fixed the issue and pushed update to my branch. I'm going to write a longer reply below how I verified my fix should be doing what I say.
joonamo
force-pushed
the
glibc-on-non-glibc-machine
branch
from
02e6348
to
61b48e5
Compare
|
Hi again! I wrote a verification script to test my changes. You can find the script and full console outputs in this gist. My problem was with sharp library, so I ran my tests with it. Sharp contains a separate installation script which contains a similar issue. I'm going send them a similar PR once we get the issue fixed in prebuild-install. Here's a table describing what different commands installed. I ran the test scripts on Darwin, Debian and Alpine Linux machines. All machines were running on arm64, but Darwin is using x64 node to verify node on that architecture works as well.
As we can see from the table, on current version of prebuild-install, LIBC or --libc only accepts value "musl". Giving it the value "glibc" will try to download inexsistent "linuxglibc"-package. My changes fix this to download expected "linux" package on all platforms. It is still depatable what The table also reveals using platform value "linuxmusl" is problematic. Using that platform on Alpine Linux will always fail, since prebuild-install will append the libc musl value after it. It might be better to drop platform "linuxmusl" and only use if (rc.platform === 'linuxmusl' && Boolean(rc.libc)) {
rc.plaform = 'linux'
}But that feels hacky and error prone. I think a better solution might be to soft-depricate the "linuxmusl" platform and instead instruct the users to define the target libc value in readme. If you want, I can update the readme like this as part of the PR. Important thing is that this PR should not change old expected behavior, at least as far as I can see. I have a case where our build machines are on Alpine Linux, but we're targeting AWS Lambda (running glibc). I can't currently make the CI machine install correct version of sharp for our production environment with supported methods. If you have any concerns, I'm happy to look into them. |
|
The sensible suggestion to split This will be a semver major change, and, should such a change land here, the logic in sharp and other native modules that rely on |
|
Good moring! How should we proceed with this issue? Would you like me to verify some scenarios, futher update the readme or make some changes to the code? I think the big change to drop platform Once we get things settled here, I'd like to start opening a draft PR to Sharp's additional install scripts so @lovell can start considering those as well. I would like make the changes to Sharp as similar to these as possible, so users can control the installation there with just one option/environment value. |
There was a problem hiding this comment.
One small change to the docs that I think is worth making, but otherwise LGTM.
Is the LIBC environment variable an existing standard that's commonly set e.g. in build systems? It looks like it was originally introduced to the prebuild ecosystem via prebuild/prebuild#116 in a semver minor change.
joonamo
force-pushed
the
glibc-on-non-glibc-machine
branch
from
61b48e5
to
ca2bb78
Compare
|
Reading the issue opened in sharp, I realized this change might only fix Linux platform. I haven't had time to test, but even with these changes, Alpine Linux machine might still generate incorrect platform name for windows (windowsmusl) or darwin (darwinmusl). If that's the case, should I make changes in this PR or create another one? |
|
@joonamo Thanks, yes, it makes sense to consider |
|
Hi! I just pushed a change that makes prebuild-install discard libc-value completely when platform is not exactly "linux". As a side effect, I only tested this on Darwin and Alpine Linux, but you can find my extended test script and outputs in this gist. I don't have access to Windows machine to test. |
|
Is this semver-minor? |
|
Yes, originally I was concerned this might be a breaking change, but the final implementation appears to be backwards compatible, so I agree with semver-minor. |
|
Hi! Would you still require something from me or are we good to go for merge? |
|
Added! Note we have a |
|
Published to npm as v7.1.0, thanks both. |
Issue
Option
--libccan be used to download musl packages on glibc and other platforms, but cannot be used to download glibc packages on musl platforms. In other words, I can pass--libc=muslor--platform=linuxmuslon any platform to install prebuild linuxmusl packages for cross-platform target.On the other hand, if I pass
--platform=linuxon Alpine Linux,linuxmuslpackages are still downloaded. If I pass--libc=glibc, it tries to downloadlinuxglibcpackages that don't exist. This effectively prevents building cross-platform packages for glibc Linux targets on Alpine Linux.Additionally, this PR allows passing
--libcoption to npm in and it will be used when installing. This allows projects depending on prebuild-install to install desired libc variant.Discussion
It could be considered to use explicit
linuxglibcname instead of justlinux, just likelinuxmusl. This would be a backwards-incompatible change and I wanted to make this change as compatible as possible.