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

statfs: fixes for s390x+musl #1835

Merged
merged 1 commit into from
Dec 16, 2023
Merged

Conversation

selfisekai
Copy link
Contributor

I believe this should fix problems in the log, except the one related to the libc crate. however, I'm not sure how to test these changes.

log from an s390x build
   Compiling nix v0.24.2
error[E0428]: the name `fs_type_t` is defined multiple times
  --> /home/buildozer/.cargo/registry/src/github.com-eae4ba8cbf2ce1c7/nix-0.24.2/src/sys/statfs.rs:33:1
   |
31 | type fs_type_t = libc::c_uint;
   | ------------------------------ previous definition of the type `fs_type_t` here
32 | #[cfg(all(target_os = "linux", target_env = "musl"))]
33 | type fs_type_t = libc::c_ulong;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `fs_type_t` redefined here
   |
   = note: `fs_type_t` must be defined only once in the type namespace of this module
   Compiling regex v1.6.0
error[E0425]: cannot find value `O_LARGEFILE` in crate `libc`
   --> /home/buildozer/.cargo/registry/src/github.com-eae4ba8cbf2ce1c7/nix-0.24.2/src/fcntl.rs:121:9
    |
121 |         O_LARGEFILE;
    |         ^^^^^^^^^^^ not found in `libc`
error[E0201]: duplicate definitions with name `optimal_transfer_size`:
   --> /home/buildozer/.cargo/registry/src/github.com-eae4ba8cbf2ce1c7/nix-0.24.2/src/sys/statfs.rs:258:5
    |
248 |     pub fn optimal_transfer_size(&self) -> u32 {
    |     ------------------------------------------ previous definition of `optimal_transfer_size` here
...
258 |     pub fn optimal_transfer_size(&self) -> libc::c_ulong {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ duplicate definition
error[E0201]: duplicate definitions with name `block_size`:
   --> /home/buildozer/.cargo/registry/src/github.com-eae4ba8cbf2ce1c7/nix-0.24.2/src/sys/statfs.rs:309:5
    |
301 |     pub fn block_size(&self) -> u32 {
    |     ------------------------------- previous definition of `block_size` here
...
309 |     pub fn block_size(&self) -> libc::c_ulong {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ duplicate definition
error[E0201]: duplicate definitions with name `maximum_name_length`:
   --> /home/buildozer/.cargo/registry/src/github.com-eae4ba8cbf2ce1c7/nix-0.24.2/src/sys/statfs.rs:367:5
    |
360 |     pub fn maximum_name_length(&self) -> u32 {
    |     ---------------------------------------- previous definition of `maximum_name_length` here
...
367 |     pub fn maximum_name_length(&self) -> libc::c_ulong {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ duplicate definition
Some errors have detailed explanations: E0201, E0425, E0428.
For more information about an error, try `rustc --explain E0201`.
error: could not compile `nix` due to 5 previous errors

selfisekai added a commit to selfisekai/nix that referenced this pull request Oct 5, 2022
s390x musl with `allow_failures`, because it currently does not compile (see nix-rust#1835)
selfisekai added a commit to selfisekai/nix that referenced this pull request Oct 5, 2022
s390x musl with `allow_failures`, because it currently does not compile (see nix-rust#1835)
@rtzoeller
Copy link
Collaborator

@selfisekai thanks for the contribution! Unfortunately it doesn't look like things are quite right, yet. You can test compiling for the s390x-unknown-linux-musl target with

cargo build --target s390x-unknown-linux-musl -Zbuild-std

I am getting more errors with these changes than without, unfortunately.


Do you have a use case for the s390x-unknown-linux-musl target? If there's a need for the nix crate to support it, I'd rather see us add it to the list of platforms we automatically build against, to ensure support does not regress. While one-off fixes are welcome, we can't guarantee that compilation won't regress if we aren't automatically testing it.

@selfisekai
Copy link
Contributor Author

unfortunately I'm unable to run it with -Zbuild-std on my machine as I'm running Rust stable from aports (rustup doesn't have the x86_64-alpine-linux-musl target, while x86_64-unknown-linux-musl produces broken binaries by default). I might try building in a Docker container.

the use case is Alpine Linux supported architectures, I made an RFC for testing on this and more architectures: nix-rust/rfcs#5

@rtzoeller
Copy link
Collaborator

Thanks for the link to the RFC; I hadn't seen it.

As a hack to unblock yourself, you might try temporarily setting RUSTC_BOOTSTRAP=1 and trying to pass -Zbuild-std. This should let you use nightly features on stable (which should absolutely not be used in production).

@nekopsykose
Copy link

nekopsykose commented Dec 11, 2022

I am getting more errors with these changes than without, unfortunately.

the errors appear to all be caused by libc now instead. things like f_namelen use u_long even though the actual musl source is unsigned for it (s390x) https://github.com/rust-lang/libc/blob/88de3880fbc3e72358023c68ecced285a830a8ca/src/unix/linux_like/linux/musl/b64/s390x.rs#L61 (the same goes for f_bsize, f_type, etc)

the other issues are also most likely also caused by libc definitions, but i didn't look too deeply. the reason there are "more errors" is because the compilation does not abort early on these first ones fixed here, and then gets to the rest of the issues.

that said, the same issue has to be raised in libc.

bors added a commit to rust-lang/libc that referenced this pull request May 18, 2023
linux/musl/s390x: change f_* constants to uint from ulong

musl defines these as `unsigned`, not `unsigned long`
https://github.com/bminor/musl/blob/7d756e1c04de6eb3f2b3d3e1141a218bb329fcfb/arch/s390x/bits/statfs.h#L2

mostly relevant to also fixing nix-rust/nix#1835

that said, i don't know if this is a huge breaking change or not, only that the current one isn't correct afaict
@nekopsykose
Copy link

libc 0.2.145 now has the necessary changes, you should try testing this again

@nekopsykose
Copy link

nekopsykose commented Jun 4, 2023

(unless it also needs the O_LARGEFILE one too)

disregard- that also made it in

@selfisekai selfisekai force-pushed the s390x-musl-statfs branch from cf327d3 to c04295a Compare June 4, 2023 19:01
@williamdes
Copy link

What is the status of this PR ?
Should it be rebased since CI fails ?

@SteveLauC
Copy link
Member

Hi @selfisekai, interested in rebasing your branch to run the CI again, the CI failures do not seem to be related to this PR

@selfisekai
Copy link
Contributor Author

rebased, seems to compile fine for s390x-unknown-linux-musl

@selfisekai
Copy link
Contributor Author

ah, no. the other way

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Thanks!

@SteveLauC SteveLauC added this pull request to the merge queue Dec 16, 2023
Merged via the queue into nix-rust:master with commit b9b4077 Dec 16, 2023
35 checks passed
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.

5 participants