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

Update the existing musl targets to be dynamically linked. #422

Closed
2 of 3 tasks
pnkfelix opened this issue Apr 1, 2021 · 38 comments
Closed
2 of 3 tasks

Update the existing musl targets to be dynamically linked. #422

pnkfelix opened this issue Apr 1, 2021 · 38 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Apr 1, 2021

Update the existing musl targets to be dynamically linked.

We should change the -musl targets to be consistently dynamically linked to their libc, rather than statically linking the musl libc into them at compile-time. If a MUSL end-user wants the musl-libc to be statically-linked into their program, they will need to specify it explicitly (e.g. via -Ctarget-feature=+crt-static)

Proposed Mitigation Strategy: In order to give the current users a chance to prepare for this change, we will first land a lint on all hosts that target musl. This lint will warn that the defaults here are going to change (namely, that the implicit +crt-static is switching to -crt-static). The lint will be silenced by explicitly providing either +crt-static or -crt-static.

Why we should do this: This would make Rust's -musl targets more consistent on two axes: It would make them more consistent with Rust's non-musl targets (which likewise dynamically link to the target's libc), and it would make them consistent with what musl toolchains expect, especially the toolchains for musl-based Linux distributions. (Today, musl-based Linux distribtuons are locally patching Rust to get the behavior they expect.)

Why we might not do this: It is a breaking change. The current defaults were put in place to support a specific use case of producing shippable binaries, and changing the default will break users who are not explicitly (and, with the current defaults, redundantly) requesting +crt-static. However, the mitigation strategy is intended to address the needs of those users.

Background: Rust currently has Tier 2 targets for a number of platforms using musl for libc (various flavors of ARM, MIPS and x86) as well as some Tier 3 targets (Hexagon, PowerPC, RISC-V, S390x and ARMv7 Thumb). Currently (most of) these targets statically link to musl libc during compilation.
ts could also be added.

Mentors or Reviewers

This MCP is an outcome from the compiler-team design meeting compiler-team#416 (zulip archive)

I expect the stake-holders who participated in that meeting to be involved, though they may not be the actual mentors.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@pnkfelix pnkfelix added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Apr 1, 2021
@rustbot
Copy link
Collaborator

rustbot commented Apr 1, 2021

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Apr 1, 2021
@petrochenkov
Copy link

petrochenkov commented Apr 1, 2021

The correct combination of flags to preserve the old behavior is

-C target-feature=+crt-static -C link-self-contained=yes

because the old musl targets not only link libc statically by default, they also link libc and crt objects shipped with rustc by default, which is also undesirable and should be avoided in the new target config.

Besides that, cargo need to be convinced to pass these flags only when compiling for the target, and not for the host (proc macros, build scripts), and we must specify how to do that in the migration diagnostic and documentation.

@petrochenkov
Copy link

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Apr 1, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 8, 2021
@Timmmm
Copy link

Timmmm commented Apr 9, 2021

-C target-feature=+crt-static -C link-self-contained=yes

That's quite a pain when most users using the musl targets are primarily doing it so that they get a statically linked binary and don't have to deal with glibc compatibility issues.

This should only be done once there is a more user friendly way to get the current behaviour.

@Diggsey
Copy link

Diggsey commented Apr 9, 2021

cc @emk who maintains a widely used docker image for building static musl binaries.

@ssokolow
Copy link

ssokolow commented Apr 9, 2021

For context, I use musl targets for using pure Rust to write "shell scripts" that need to meet high standards of reliability, so I consider a Docker-based solution a deal-breaker.

Docker simply adds far too much delay and complexity to the compilation cycle for that kind of use-case... not to mention that I've spent the last few years promoting "For pure-Rust projects, Go-like static builds are as simple as rustup target add and cargo build --target" as a virtue of the language.

@Diggsey
Copy link

Diggsey commented Apr 9, 2021

Docker simply adds far too much delay and complexity to the compilation cycle

Not sure if that was directed at my comment, but I'm not suggesting that everyone should use the docker image, just that the author and users of that docker image would be impacted by this potential change and should know about it.

@ssokolow
Copy link

ssokolow commented Apr 9, 2021

I was unsure what your intent was, so I left whether I was replying to you ambiguous.

EDIT: I don't have a Zulip account (URLO, IRLO, Matrix, Reddit, StackExchange, etc. ...but not Zulip) and I'm not really in the mood to create an account on yet another service right now (assuming I even can, given how many services seem to be restricting themselves to those who have SMS numbers these days), so I'll bow out and hope my interests get adequately represented by someone else.

@wesleywiser
Copy link
Member

Hi all, MCP issues aren't meant for discussion, we have a separate Zulip thread for that. Let's please move the discussion to that thread. Thanks!

@androm3da
Copy link

I'm happy to review or contribute changes necessary for hexagon to support this change.

@vi
Copy link

vi commented Apr 9, 2021

Will buildability of static musl be still tested automatically on CI even if/when dynamic musl becomes default?

Or will static flavour of musl effectively become Tier 3 instead of Tier 2?

@matthieu-m
Copy link

A Zulip thread/stream is very awkward to parse when it comes to searching for whether a question has already been asked, discussed, or answered.

I would recommend aggregating questions as well as their answers (if any) and the salient points of the discussion (if any) together in some sort of FAQ document... for example in the above MCP itself.

That is, I am fine with the discussion occurring on Zulip (or wherever) but I recommend that it be summarized in a more accessible manner (aggregated/summary) and at a more static location (nobody likes to scroll dozens of pages).

@nikomatsakis
Copy link
Contributor

I think adding a FAQ to the template and collecting major points raised in the conversation is a good idea.

@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels May 20, 2021
@Timmmm
Copy link

Timmmm commented May 20, 2021

Uhm... from the thumbs ups here it kinda looks like everyone agreed this was a bad idea (or at least premature without a simple alternative)... and then it was accepted?

You guys should at least post the reasoning behind this decision.

@Timmmm
Copy link

Timmmm commented May 20, 2021

I skimmed the Zulip thread and it seems like the conclusion was that initially Rust will look for a system copy of Musl and dynamically link with that, if available, and fall back to static linking. And maybe there will be some kind of --static flag. That seems reasonable.

@RalfJung
Copy link
Member

It indeeds make it more compatible, but it also break existing scripts/CIs and we can no longer just fallback to musl target, expecting a statically linked binary.

The fact that our musl targets work like that is a consequence of us screwing up and not coordinating with the musl maintainers. It's convenient for some cases but bad for others.

Everyone who wants static linking should start adding -C target-feature=+crt-static to their flags now, to make this transition as smooth as possible. Of course we'll have a transition phase and everything, but keeping the current default is IMO not an option.

@vi
Copy link

vi commented Oct 18, 2023

Maybe there should be some easier way to turn on +crt-static which does not involve config files or RUSTFLAGS? E.g. special target name x86_64-unknown-linux-musl-static, which resolves to x86_64-unknown-linux-musl + target-feature=+crt-static.

@bjorn3
Copy link
Member

bjorn3 commented Oct 18, 2023

There is no ABI incompatibility between the static and dynamic version, so I don't think a separate target makes sense. Also if you are on a musl distro and want to produce statically linked executables you now need two targets installed. The dynamically linked one for rustc as otherwise it can't load proc macros, and the statically linked one for the executable to build. I think having a profile option for crt-static separate from rustflags makes more sense. You can set those per-target too as well using an env var. In addition cargo can decide to ignore it when necessary and avoid recompilation of everything when switching between the two options. Amd if you really want maybe cargo build could have an extra cli flag for this.

@NobodyXu
Copy link

NobodyXu commented Oct 18, 2023

I think having something like $cpu_arch-alpine-linux-musl makes sense for me, since AFAIK Alpine is the only distro that uses musl libc by default and have the dynamic library installed.

For all other $cpu_arch-unknown-linux-musl, they likely don't have musl libc installed and it still makes sense to link with musl libc statically most of the time.

That's literally the only reason why we use musl despite its poor malloc and dns implementation.

@kpcyrd
Copy link

kpcyrd commented Oct 18, 2023

There are multiple Linux distributions that use dynamic linking and musl libc, most notably:

  1. Alpine Linux
  2. Void Linux
  3. OpenWRT

@NobodyXu
Copy link

Thanks, I didn't know that.

@ernsteiswuerfel
Copy link

There are multiple Linux distributions that use dynamic linking and musl libc, most notably:

1. Alpine Linux

2. Void Linux

3. OpenWRT
  1. Gentoo Linux

@vi
Copy link

vi commented Oct 18, 2023

I don't think a separate target makes sense ...

I didn't mean a separate target, I meant a way to alias existing target + a compiler option. x86_64-unknown-linux-musl-static is supposed to turn into x86_64-unknown-linux-musl early in tools like Cargo or Rustup.

@est31
Copy link
Member

est31 commented Oct 18, 2023

@NobodyXu if you want to read more, please read:

  • the PR where people pointed out that statically linking musl is actually a policy violation on a bunch of musl distros: add dynamically-linked musl targets rust#82556 . the compiler team decided to just change the default instead of adding specific dynamically linked targets.

  • the hackmd from the compiler meeting where multiple approaches were studied: https://hackmd.io/YoAGSxUsRWumVvbRiHddrg

  • for historic curiosity, the original PR that added musl to rust: Experimental MUSL target support for the compiler rust#24777 . it also has a link to a reddit thread out of which the PR was born out of, which explored musl for the purposes of having a libc that one can link against statically. this explains why musl was originally statically linked. Ideally it should have been dynamically linked from the start, but these links show that people added musl as a means to get a statically linked libc. only later it was realized that both the musl maintainer and the musl distros prefer dynamic linking.

also, maybe too late at this point, but the place to discuss an MCP is in the zulip thread associated to it, not the github thread of the MCP.

@RalfJung
Copy link
Member

only later it was realized that both the musl maintainer and the musl distros prefer dynamic linking.

That's really the main point for me. We should respect the policy and decisions of the people maintaining the target we're building for.

Having cargo build --static as an alias-of-sorts for RUSTFLAGS="-C target-feature=+crt-static" cargo build makes a lot of sense; cargo knows a bunch of rustc flags (such as optimizations and debug assertions), it can also know about this one. That would be a cargo feature request.

@est31
Copy link
Member

est31 commented Oct 18, 2023

Having cargo build --static as an alias-of-sorts for RUSTFLAGS="-C target-feature=+crt-static" cargo build makes a lot of sense;

imo that would go further than just statically linking libc. I'd expect it to also statically link all the native libraries the project depends on through either build.rs or other means. cargo could set some env var or such that projects then should respect. that's usually what people want when they want static linking, they want everything to be linked statically, not just libc.

@bjorn3
Copy link
Member

bjorn3 commented Oct 18, 2023

I believe there is a cfg exported for users to determine if crt-static is used. On most targets crt-static mandates full static linking, though not all targets. There is a target spec key for this.

@est31
Copy link
Member

est31 commented Oct 18, 2023

Hmm yeah one can do #[cfg(target_feature = "crt-static")] and build.rs scripts can check CARGO_CFG_TARGET_FEATURE as per this RFC. Ultimately, this is best discussed in the appropriate issue in the cargo repo.

@NobodyXu
Copy link

Thanks @est31 will have a read of them

BurntSushi added a commit to BurntSushi/ripgrep that referenced this issue Oct 24, 2023
It looks like the musl target will, at some point, default to be
dynamically linked. This config knob should make it so that it's always
statically linked.

Ref rust-lang/compiler-team#422
Ref rust-lang/compiler-team#422 (comment)
Jake-Shadle added a commit to EmbarkStudios/cargo-deny that referenced this issue Oct 25, 2023
Upgrades, deps, and changes the config to force musl to always be
statically linked, see
[this](rust-lang/compiler-team#422) for why.
BurntSushi added a commit to BurntSushi/ripgrep that referenced this issue Nov 21, 2023
It looks like the musl target will, at some point, default to be
dynamically linked. This config knob should make it so that it's always
statically linked.

Ref rust-lang/compiler-team#422
Ref rust-lang/compiler-team#422 (comment)
lmbarros added a commit to balena-os/takeover that referenced this issue Mar 6, 2024
We were relying on the fact that our current targets link the C runtime
statically. However, this may be different for other targets and could
even change in the future (e.g., see
rust-lang/compiler-team#422).

With this commit we adopt the current recommendation, which is enabling
the crt-static target feature when building. See
https://doc.rust-lang.org/reference/linkage.html#static-and-dynamic-c-runtimes
for details.

Given the current defaults for Rust targets, this change should be a
no-op, but this shall make us safer and more resilient to Rust changes
in the long run.

Signed-off-by: Leandro Motta Barros <leandro@balena.io>
Change-type: patch
@xbjfk
Copy link

xbjfk commented Mar 23, 2024

Hi,
I know it's been quite a while since the last comment on this, but I thought I would add my two cents.
Overall, I support this change.
I accept that it will break existing CI scripts, however the current situation is even worse.
Currently, the (arguably) three largest distros shipping musl all patch rust to disable default static on musl (Gentoo, Alpine and Void) - meaning currently the musl target changes depending on what distro it's run - which is especially concerning, given Alpine's popularity as a distro for containers. Someone in a linked issue already got caught out by this.
Another problem is that a lot of packages make assumptions that musl libc means static linking - an example of this is llvm-sys, leading to distro users or maintainers having to patch this behavior out for each package that assumes it (although this behaviour is incorrect anyway even currently).
There really doesn't seem to be a way forward without annoying a large group of people, and I think it's best to rip the band aid off now and remove the necessity for downstreams to diverge as much rather than wait as more and more infrastructure depends on this implicit behavior.

@kpcyrd
Copy link

kpcyrd commented Mar 24, 2024

Could this be done as part of the upcoming 2024 edition?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests