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

add dynamically-linked musl targets #82556

Closed
wants to merge 13 commits into from

Conversation

kaniini
Copy link
Contributor

@kaniini kaniini commented Feb 26, 2021

This pull request adds $arch-unknown-linux-dynmusl targets as a generic replacement for Alpine's $arch-alpine-linux-musl targets. The name dynmusl was chosen based on discussion in rust-lang/rfcs#2695 and #59302.

The platform support entries are based on the current state of the equivalent $arch-alpine-linux-musl targets.

Rationale copied from down-thread...


The dynmusl targets are intended to provide a set of targets intended for use on dynamically-linked musl distros. They are intended to act the same as the glibc targets. The dynmusl targets are based on Alpine's previous set of targets which did the same thing (and review has found some deficiencies, thanks everyone!).

I believe the dynmusl targets provide a compromise that is minimally intrusive to the rust compiler team, while providing musl-based distros like Alpine with a set of targets that meet their requirements. While not perfect (in an ideal world, the -musl targets would not assume static linking and work like glibc), it does solve the problem in a way I believe we can all live with.

These targets are required because Linux distributions generally consider static linking by default to be a policy violation. It is a policy violation in Alpine, Void and Adelie Linux, which are the top three musl-based distributions. Though it should also be said that there are staticly-linked musl distributions such as Sabotage, which would continue to be served by the -musl targets directly (if they ship Rust at all, I haven't checked).

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2021
@kaniini
Copy link
Contributor Author

kaniini commented Feb 26, 2021

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned davidtwco Feb 26, 2021
@rust-log-analyzer

This comment has been minimized.

@alex
Copy link
Member

alex commented Feb 26, 2021

Build failure looks relevant.

The docs entries have toolchain entries marked as being built, but I don't see any CI changes. Is there a reason those wouldn't be necessary?

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

The docs entries have toolchain entries marked as being built, but I don't see any CI changes. Is there a reason those wouldn't be necessary?

Yeah, I think the checkmarks should be removed from the new entries in platform-support.md.

@jyn514 jyn514 added A-target-specs Area: compile-target specifications T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 26, 2021
@ericonr
Copy link
Contributor

ericonr commented Feb 26, 2021

I have already requested this over IRC, but for completeness sake, i686 dynmusl would complete the target matrix for Void Linux as well.

@kaniini
Copy link
Contributor Author

kaniini commented Feb 26, 2021

@ericonr added in ff1825a 👍

Copy link
Contributor

@12101111 12101111 left a comment

Choose a reason for hiding this comment

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

rustbuild also need to update as dynmusl target don't need to ship rustlib/$target/lib/self-contained/*.o.

@rust-log-analyzer

This comment has been minimized.

@estebank
Copy link
Contributor

estebank commented Mar 1, 2021

@petrochenkov we could change the default behavior and when detecting we are in a non-musl platform (the opposite check you linked to earlier) emit an unconditional warning letting you know that if you meant to build a statically compiled musl binary, to be explicit by passing +crt-static or to silence the warning by explicitly passing -crt-static. The run to see what breaks would still be needed and the communication effort will be significant. We wouldn't be able to remove the warning for years due to the existing, out of date documentation out there that is not under our control.

@kaniini
Copy link
Contributor Author

kaniini commented Mar 1, 2021

I think what @estebank proposes makes the most sense. What would be involved in adding a warning for this?

@michaelforney
Copy link
Contributor

Note also that some build recipes are already broken with the current static-by-default behavior.

The main question is - why is -C target-feature=-crt-static not enough and why do we need a separate target?

Here's an example: firefox requires its build scripts to be dynamically linked, but it uses cargo --target=... which makes it impossible set flags for the build scripts, thus it is currently impossible to build firefox with *-unknown-linux-musl rust toolchains.

I think everyone in the musl community would really just like to see this fixed using the standard toolchain names. It has been a huge barrier for people trying to use rust on musl-based systems for years now.

@petrochenkov
Copy link
Contributor

@michaelforney

requires its build scripts to be dynamically linked, but it uses cargo --target=... which makes it impossible set flags for the build scripts

This is exactly the kind of issue that I meant in

If cargo makes passing such options inconvenient, then it should be pushed to make compiler option support more convenient and fine-grained instead.

in #82556 (comment) (regardless of the musl specifics).

@bors
Copy link
Contributor

bors commented Mar 2, 2021

☔ The latest upstream changes (presumably #82663) made this pull request unmergeable. Please resolve the merge conflicts.

@kaniini
Copy link
Contributor Author

kaniini commented Mar 2, 2021

I agree that it should be easier to override target behaviour but in this case the default behaviour is completely broken for musl-based distributions, which should be the primary use case for a -musl target, like glibc-based distros are the primary use case for the -gnu targets.

@workingjubilee
Copy link
Member

Wait, does setting RUSTFLAGS="-Ctarget-feature=-crt-static" with cargo build --target x86_64-unknown-linux-musl override RUSTFLAGS? I just want to make sure if I am understanding the existing behavior correctly.

@bjorn3
Copy link
Member

bjorn3 commented Mar 2, 2021

If you pass --target, cargo ignores RUSTFLAGS for crates build for the host like build scripts and proc macros.

@richfelker
Copy link

I agree that it should be easier to override target behaviour but in this case the default behaviour is completely broken for musl-based distributions, which should be the primary use case for a -musl target, like glibc-based distros are the primary use case for the -gnu targets.

Yes. This is the case where the application being compiled and build script being used to compile it have no reason to know anything about musl or that the target is musl-based, and they can't be expected to do anything to override the wrong default of static linking that might break what they're trying to do.

If on the other hand youre trying to make static binaries using musl to distribute and run on systems where they're foreign, you're in the case where you're not just using a target fed to you as whatever the host default is, but working with a specific known system. In that case you can reasonably be expected to take care of your own target options.

@nagisa
Copy link
Member

nagisa commented Mar 4, 2021

@rfcbot fcp cancel

Given the discussion is ongoing and appears to have leaned towards an alternate approach, I'm going to cancel the FCP proposing the merge of this PR as it is written currently.

@rfcbot
Copy link

rfcbot commented Mar 4, 2021

@nagisa proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 4, 2021
@estebank
Copy link
Contributor

estebank commented Mar 4, 2021

@kaniini just want to let you know that fcp cancellation is about the specifics of this PR's approach, given that there are multiple possible courses of action and we need to reach an agreement on which one to pursue. We still want to see this fixed in a way that everybody involved is happy with!

@apiraino
Copy link
Contributor

Given this comment I'm removing also the "waiting on team" label for now.

@rustbot label -S-waiting-on-team

@rustbot rustbot removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Mar 10, 2021
@petrochenkov
Copy link
Contributor

This is still waiting-on-team, the issue was nominated for discussion, but not discussed yet. Summary.

@petrochenkov petrochenkov added I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 10, 2021
@apiraino
Copy link
Contributor

@petrochenkov ah sorry then I have misunderstood the discussion from last week T-compiler meeting about this. Thanks for fixing!

@wesleywiser
Copy link
Member

Hi everyone! We have a compiler team design meeting scheduled to discuss what to do to resolve this issue on March 26 10am EDT (14:00 UTC). Meetings are held in the #t-compiler/meetings Zulip stream.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 8, 2021

The compiler team had a design team meeting and have proposed an MCP with an alternative solution to this problem: rust-lang/compiler-team#422

@kaniini
Copy link
Contributor Author

kaniini commented Apr 8, 2021

I think we can close this PR and focus on the other solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-target-specs Area: compile-target specifications S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.