Enable statx on all Linux targets and short-circuit it for musl - #159540
Enable statx on all Linux targets and short-circuit it for musl#159540Gelbpunkt wants to merge 3 commits into
Conversation
|
|
This comment has been minimized.
This comment has been minimized.
Since statx is currently invoked through the syscall directly anyways, it makes no sense to gate it to glibc targets, since none of this is glibc specific at the moment. As long as the kernel is recent enough, it supports statx.
This is a requirement for using libc::statx in std and the musl baseline in the Rust compiler is currently 1.2.5.
On musl targets, we can assume that musl has support for statx and falls back to fstatat internally, so there's no need to do detection and caching of statx in std.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
The job Click to see the possible cause of the failure (guessed by this bot)Important For more information how to resolve CI failures of this job, visit this link. |
| // default unless opted into via an environment variable. The version baseline of the Rust | ||
| // compiler targets is currently 1.2.5, so we can set the variable to get access to statx in | ||
| // std, amongst other things. | ||
| cargo.env("RUST_LIBC_UNSTABLE_MUSL_V1_2_3", "1"); |
There was a problem hiding this comment.
This would break with build-std, right? Cargo wouldn't set this env var.
There was a problem hiding this comment.
That's a good point, right. Not sure how to go about that. I presume we'd want to modify cargo to set the variable for rustc processes while compiling std? That sounds fairly complicated
There was a problem hiding this comment.
Can we make this a regular cargo feature prefixed with unstable or internal-do-not-use or something like that and then enable it here?
There was a problem hiding this comment.
More recent libc versions require us to specify --cfg=libc_unstable_musl_v1_2_3 as of rust-lang/libc@452750f, but in that case just setting it here still wouldn't be enough. Having to set this here and in cargo feels a bit hacky. It might make sense to make the rustc-dep-of-std feature imply libc_unstable_musl_v1_2_3. libc already suppresses a few warnings for musl version compatibility if that feature is enabled.
There was a problem hiding this comment.
Would we actually be able to lift the musl_v1_2_3 gate on statx? I don't remember why I originally asked for the gate but I guess maybe I was thinking of struct stat which has time_t fields (and thus would have an incorrect definition without musl_v1_2_3), whereas struct statx brings its own time types https://github.com/rust-lang/libc/blob/d0c9b0f14ef624576cfcb0a1285b2b29d54f4d47/src/unix/linux_like/mod.rs#L283-L333.
If we can do that then it avoids the build-std issue. At some point, however, we should probably figure out a way to set the cfgs for libc to enable 64-bit time_t.
There was a problem hiding this comment.
statx is supported in musl only since 1.2.5, so if libc were to unconditionally expose it, I presume you'd see linker errors with older musl releases when calling it. I really think the easiest path forward here would be making rustc-dep-of-std imply libc_unstable_musl_v1_2_3 and 64-bit time_t, that'd also fix the build-std issue.
There was a problem hiding this comment.
I don't think that's a problem since we have lots of API for all platforms that isn't available until more recently. Won't most users be getting musl via the bundled version anyway, which is 1.2.5?
Regardless I think what you mentioned with rustc-dep-of-std is reasonable, though it would be best to test it by first setting the cfg in rust-lang/rust and running tests to see if anything else needs changing (to avoid some back-and-forth)
This supersedes #154981. cc @strophy and sorry for taking a while to get around to this.
The first patch is the original PR with the commit message slightly adjusted, which makes use of the statx runtime detection on all Linux targets.
Since we want to unconditionally use statx on musl targets (because we know statx is available on our supported musl versions), we can call it through
libc::statx. Making use of that requires telling the libc crate that we are on a musl version recent enough for it, which I decided to do inbootstrap. If there's a better place to set the environment variable, let me know, but this is the most straightforward way I could come up with.With that in place, we can then wire up a short circuit to
libc::statxfor musl targets and skip the detection logic that all other Linux targets use.On my
aarch64-unknown-linux-muslmachine,./x.py test library/stdstill passes with these changes.r? @tgross35 since I presume you have opinions on how I'm holding libc here