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

Allow setting libc to glibc on non-glibc platform #176

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

joonamo
Copy link
Contributor

@joonamo joonamo commented Mar 18, 2022

Issue

Option --libc can 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=musl or --platform=linuxmusl on any platform to install prebuild linuxmusl packages for cross-platform target.

On the other hand, if I pass --platform=linux on Alpine Linux, linuxmusl packages are still downloaded. If I pass --libc=glibc, it tries to download linuxglibc packages that don't exist. This effectively prevents building cross-platform packages for glibc Linux targets on Alpine Linux.

Additionally, this PR allows passing --libc option 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 linuxglibc name instead of just linux, just like linuxmusl. This would be a backwards-incompatible change and I wanted to make this change as compatible as possible.

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
Copy link
Member

Choose a reason for hiding this comment

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

Could be a breaking change if someone is setting the LIBC env var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

joonamo commented Mar 19, 2022

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.

Command Expected Alpine original Alpine this PR Debian original Debian this PR Darwin original Darwin this PR
node_modules/.bin/prebuild-install --verbose (depends on platform) sharp-v0.30.3-napi-v5-linuxmusl-arm64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-arm64.tar.gz ✅ sharp-v0.30.3-napi-v5-linux-arm64.tar.gz ✅ sharp-v0.30.3-napi-v5-linux-arm64.tar.gz ✅ sharp-v0.30.3-napi-v5-darwin-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-darwin-x64.tar.gz ✅
node_modules/.bin/prebuild-install --platform=linux --arch=x64 --verbose (depatable) sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ❓(Should this be linux package?) sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ❓(Should this be linux package?) sharp-v0.30.3-napi-v5-linux-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linux-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linux-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linux-x64.tar.gz ✅
LIBC=glibc node_modules/.bin/prebuild-install --platform=linux --arch=x64 --verbose sharp-v0.30.3-napi-v5-linux-x64.tar.gz sharp-v0.30.3-napi-v5-linuxglibc-x64.tar.gz ❌ (Not found, incorrect package) sharp-v0.30.3-napi-v5-linux-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxglibc-x64.tar.gz ❌ (Not found, incorrect package) sharp-v0.30.3-napi-v5-linux-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxglibc-x64.tar.gz ❌ (Not found, incorrect package) sharp-v0.30.3-napi-v5-linux-x64.tar.gz ✅
node_modules/.bin/prebuild-install --platform=linux --arch=x64 --libc=glibc --verbose sharp-v0.30.3-napi-v5-linux-x64.tar.gz sharp-v0.30.3-napi-v5-linuxglibc-x64.tar.gz ❌ (Not found, incorrect package) sharp-v0.30.3-napi-v5-linux-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxglibc-x64.tar.gz ❌ (Not found, incorrect package) sharp-v0.30.3-napi-v5-linux-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxglibc-x64.tar.gz ❌ (Not found, incorrect package) sharp-v0.30.3-napi-v5-linux-x64.tar.gz ✅
node_modules/.bin/prebuild-install --platform=linuxmusl --arch=x64 --verbose sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz sharp-v0.30.3-napi-v5-linuxmuslmusl-x64.tar.gz ❓ (Incorrect packge, out of scope for this PR) sharp-v0.30.3-napi-v5-linuxmuslmusl-x64.tar.gz ❓ (Incorrect packge, out of scope for this PR) sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅
LIBC=musl node_modules/.bin/prebuild-install --platform=linux --arch=x64 --verbose sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅
node_modules/.bin/prebuild-install --platform=linux --arch=x64 --libc=musl --verbose sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅ sharp-v0.30.3-napi-v5-linuxmusl-x64.tar.gz ✅
node_modules/.bin/prebuild-install --platform=linuxmusl --arch=x64 --libc=musl --verbose (depatable) sharp-v0.30.3-napi-v5-linuxmuslmusl-x64.tar.gz ❓ (Incorrect packge, out of scope for this PR) sharp-v0.30.3-napi-v5-linuxmuslmusl-x64.tar.gz ❓ (Incorrect packge, out of scope for this PR) sharp-v0.30.3-napi-v5-linuxmuslmusl-x64.tar.gz ❓ (Incorrect packge, out of scope for this PR) sharp-v0.30.3-napi-v5-linuxmuslmusl-x64.tar.gz ❓ (Incorrect packge, out of scope for this PR) sharp-v0.30.3-napi-v5-linuxmuslmusl-x64.tar.gz ❓ (Incorrect packge, out of scope for this PR) sharp-v0.30.3-napi-v5-linuxmuslmusl-x64.tar.gz ❓ (Incorrect packge, out of scope for this PR)

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 prebuild-install --platform=linux should install. On Debian and Darwin, it installs "linux" packages, but on Alpine Linux it installs "linuxmusl" package. I consider this to be acceptable behavior, since you can now specify --libc=glibc target.

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 --platform=linux --libc=musl to get expected results, but that would be a breaking change. We could implement something like

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.

@vweevers vweevers requested a review from lovell March 19, 2022 20:13
@lovell
Copy link
Member

lovell commented Mar 19, 2022

The sensible suggestion to split libc into its own option, rather than merging it into the OS as linuxmusl does, is something I've been expecting for a while, so thank you for making the time to submit this PR.

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 prebuild will hopefully change to reflect this also.

@joonamo
Copy link
Contributor Author

joonamo commented Mar 21, 2022

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 linuxmusl completely isn't something a first-time contributor as myself should be doing. These changes should fix the immediate problem that Alpine Linux cannot build non-musl packages by introducing the predictable cross-platform support for --libc=glibc and LIBC=glibc.

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.

Copy link
Member

@lovell lovell left a comment

Choose a reason for hiding this comment

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

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.

README.md Outdated Show resolved Hide resolved
@joonamo
Copy link
Contributor Author

joonamo commented Mar 28, 2022

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?

@lovell
Copy link
Member

lovell commented Mar 30, 2022

@joonamo Thanks, yes, it makes sense to consider libc only when the platform is "linux" for now. Happy to have that included as part of this PR as it's very much related.

@joonamo
Copy link
Contributor Author

joonamo commented Apr 1, 2022

Hi! I just pushed a change that makes prebuild-install discard libc-value completely when platform is not exactly "linux". As a side effect, --platform=linuxmusl --libc=musl works as well, as does installing win32 and darwin on Alpine Linux.

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.

Copy link
Member

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, LGTM

@vweevers
Copy link
Member

vweevers commented Apr 2, 2022

Is this semver-minor?

@lovell
Copy link
Member

lovell commented Apr 2, 2022

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.

@joonamo
Copy link
Contributor Author

joonamo commented Apr 19, 2022

Hi! Would you still require something from me or are we good to go for merge?

@vweevers
Copy link
Member

I haven't had time to review but I trust @lovell. Good to go.

@lovell would you like to help out with merging and releasing? If so, what's your npm username? I'll add you

@lovell
Copy link
Member

lovell commented Apr 19, 2022

@vweevers
Copy link
Member

Added!

Note we have a CHANGELOG.md here, which can be updated by hand or with npx hallmark cc add minor. Other than that, it's the usual npm version .. && git push --follow-tags (the tag will trigger a workflow that creates a GitHub release) and npm publish.

@lovell lovell merged commit f729abb into prebuild:master Apr 20, 2022
@lovell
Copy link
Member

lovell commented Apr 20, 2022

Published to npm as v7.1.0, thanks both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants