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

Build for gnu instead of musl #882

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

t-botz
Copy link

@t-botz t-botz commented Dec 13, 2022

Redoing #389 which was cancelled by #554 and reintroduced the error.

Fixes #520

Redoing Schniz#389 which was cancelled by Schniz#554 and reintroduced the error.

Fixes Schniz#520
@changeset-bot
Copy link

changeset-bot bot commented Dec 13, 2022

⚠️ No Changeset found

Latest commit: 7b7e49e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

- uses: actions/checkout@v3
- name: Build release binary
run: cargo build --release --target x86_64-unknown-linux-musl
- name: Strip binary from debug symbols
run: strip target/x86_64-unknown-linux-musl/release/fnm
Copy link
Author

Choose a reason for hiding this comment

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

no need for strip anymore since Cargo 1.59

@t-botz
Copy link
Author

t-botz commented Dec 13, 2022

Another small win with this PR is that the binary goes from 7.2MB to 6.5MB...
IMO there's very little reason to build a static binary with musl. fnm is meant for a dev environment supporting node, I can't see how such environment would be missing libc...

@t-botz t-botz mentioned this pull request Dec 13, 2022
@Schniz
Copy link
Owner

Schniz commented Dec 13, 2022

This means vc alpine-based docker images will fail to use fnm. Right?
Can we add a gnu build? Then download it conditionally on the install script if it’s not an alpine Linux?
I wonder if we should rethink the build process and copy from Starship: https://github.com/starship/starship/blob/master/.github/workflows/release.yml

@Schniz
Copy link
Owner

Schniz commented Dec 13, 2022

Ahh and also thank you very much for digging deep and figuring out the issue. Reaaaaally appreciate it 🙌

@t-botz
Copy link
Author

t-botz commented Dec 13, 2022

This means vc alpine-based docker images will fail to use fnm. Right?

Thats right, I am not sure who does that (fnm in a docker image kind of defy the purpose IMO) but I guess if you ask, you might have encountered the use case.

Can we add a gnu build? Then download it conditionally on the install script if it’s not an alpine Linux?
I wonder if we should rethink the build process and copy from Starship

You mean using a matrix and cross? Seems like a good idea to me and fairly standard.
For the install script may be do it based on ldd --version | grep GLIBC? If no ldd or no GLIBC then use musl build?
(similar to rustup-init.sh)
I can definitely work on that.

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.

Connection errors
2 participants