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

feat(build): enable Linux musl builds #483

Merged
merged 1 commit into from
Oct 17, 2023
Merged

feat(build): enable Linux musl builds #483

merged 1 commit into from
Oct 17, 2023

Conversation

mistydemeo
Copy link
Contributor

This adds support for musl builds on Linux. This currently supports x86_64 but not aarch64, as with glibc. Sample successful build: https://github.com/mistydemeo/cargodisttest/actions/runs/6540051759/job/17759406467 A few notes:

  • We're building on an Ubuntu host, so it's not being built on a musl-native distro.
  • We use the official, default Rust tooling for this from rustup, which means musl is statically linked instead of dynamically linked like some musl-based distros prefer.
  • Because of this, dynamically linking to external dependencies will fail in musl builds. I briefly toyed with disallowing system dependency installation for that reason, but there's no reason to block people from installing cmake/whatever build tools just because they won't be able to link against some system libraries. It does, however, mean there are gotchas with musl support that we have to document, because they don't exist for glibc builds.

Fixes #75.

@mistydemeo mistydemeo requested a review from Gankra October 17, 2023 00:26
@mistydemeo mistydemeo force-pushed the linux-musl branch 2 times, most recently from ef17288 to 5dc9b4f Compare October 17, 2023 00:47
Gankra
Gankra requested changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

do we not need to setup musl-tools? i thought github's images didn't include those...

cargo-dist/src/backend/ci/github.rs Outdated
install_sh
targets: &[&String],
) -> String {
if runner == GITHUB_LINUX_RUNNER && targets.contains(&&"x86_64-unknown-linux-musl".to_owned()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i believe axoproject has a const for this target triple we can use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, but everywhere else in the file we're just inlining the triples. I figure we should be consistent.

@mistydemeo
Copy link
Contributor Author

do we not need to setup musl-tools? i thought github's images didn't include those...

I'm as surprised as you are! But you can see the build succeeded without installing it. I was able to confirm locally; so long as build-essential is installed, I can cargo build --release --target=aarch64-unknown-linux-musl without musl-tools.

@Gankra
Copy link
Contributor

Gankra commented Oct 17, 2023 •
edited
Loading

It might be worth checking a bigger project that might be pulling in transitive C deps, in case the issues only pop up when gcc gets used in anger -- oranda for instance.

@ashleygwilliams
Copy link
Member

following @Gankra's comment- i also think this feature is a really good candidate for a publicly advertised prerelease. we can also message folks who have requested this!

@mistydemeo
Copy link
Contributor Author

Boo, turns out oranda does need it. Adjusted the build to make sure we install it.

Gankra
Gankra reviewed Oct 17, 2023
View reviewed changes
cargo-dist/src/tasks.rs
Comment on lines +1891 to +1893
if target.ends_with("linux-musl")
&& self.inner.tools.cargo.host_target.ends_with("linux-gnu")
{
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, any particular motivation to dropping the "target != host_target" check? (Not sure if it would ever trip, but would it ever be wrong..?)

I was thinking we might want to simplify this + macos down to just "always run rustup if we're not building for host", but that might be too far..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just figure that since this check implicitly contains that check, there's no reason to add the extra conditional.

@mistydemeo mistydemeo merged commit 4054f1d into main Oct 17, 2023
@mistydemeo mistydemeo deleted the linux-musl branch October 17, 2023 18:35
@bjorn3
Copy link

bjorn3 commented Oct 17, 2023

Could you explicitly enable -Ctarget-feature=+crt-static? The intent is to switch the musl target to default to dynamic linking in the future. Having everyone who depends on musl statically linking explicitly specify this will make it easier to flip the switch in the future.

@ashleygwilliams
Copy link
Member

@bjorn3 gonna pull this comment out into an issue so we don't miss it!

@ashleygwilliams ashleygwilliams mentioned this pull request Oct 17, 2023
Closed
@DenuxPlays DenuxPlays mentioned this pull request Nov 19, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gankra Gankra Gankra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

musl support (prefer musl for linux?)
4 participants
@mistydemeo @Gankra @ashleygwilliams @bjorn3