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

Fix build on latest nightlies when using the nightly feature #16

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

AlexTMjugador
Copy link
Contributor

@AlexTMjugador AlexTMjugador commented Feb 13, 2024

As described in #15, the stdsimd feature was removed.

I have tested that these changes do not introduce any behavior changes by running the defined unit tests.

Resolves #15.

As described in mcountryman#15,
the `stdsimd` feature was removed.

I have tested that these changes do not introduce any behavior changes
by running the defined unit tests.
@mcountryman
Copy link
Owner

Looks like the build is still failing. When I have a moment I can take a look and see why. I imagine we'll have to somehow do a #[cfg(...)] rustc version check to support old rust versions.

Also I kinda want to implement a test suite that explicitly compiles with avx512, sse, etc. support and checks that the appropriate symbols are exported to make sure the cfg rules aren't bunk.

@AlexTMjugador
Copy link
Contributor Author

AlexTMjugador commented Feb 13, 2024

Hey, thanks for the prompt review!

[...] I imagine we'll have to somehow do a #[cfg(...)] rustc version check to support old rust versions.

I thought about using the unstable #[cfg(version(..))] attribute to broaden compatibility, but then I stumbled upon the tracking issue for it explaining how it does not work with the required granularity for nightly versions: rust-lang/rust#64796. Given this fact, and that there already is some expectation for nightly features to require the latest nightly compilers (rust-lang/rust#64796 (comment)), I'd say we do not need to care about older nightly compilers. (Edit: I also thought about leveraging build scripts, but adding one for this seems a bit overkill.)

Looks like the build is still failing. When I have a moment I can take a look and see why.

My two cents on the failures:

  • The MIPS build fails because the rust-std component is missing for the target mips-unknown-linux-gnu. If we don't need such component (I think we don't as we are not using -Z build-std or something like that), we can "fix" this by not installing it. Also, I'd migrate away from the actions-rs/toolchain action because it's unmaintained; the best current alternative is dtolnay/rust-toolchain.
  • The ARM and AArch64 builds may fail because the latest tags of their images date from 2022, so the Rust compiler they have can be pretty old: https://github.com/cross-rs/cross/pkgs/container/arm-unknown-linux-gnueabi, https://github.com/cross-rs/cross/pkgs/container/aarch64-unknown-linux-gnu. For the simple cross-compilation needs of simd-adler32, I'd recommend doing away with cross entirely, as it introduces more fragile moving pieces than just using Rust's native cross-compilation support, which should require little system setup for projects without direct C dependencies: just install the Ubuntu cross-compilation toolchain package and you're good to go. Coincidentally, this is what I did for OxiPNG 😄

@CryZe
Copy link
Contributor

CryZe commented Feb 13, 2024

I believe you misidentified both issues:

  • MIPS does not have the component rust-std because MIPS is now a tier 3 target. You either build it yourself... or you don't use it at all.
  • dtolnay/rust-toolchain is very questionable, but that's just my personal opinion
  • The cross issue has nothing to do with cross itself (cross uses the host compiler) and instead is because AVX512 is being used on ARM. You can see that the feature requires a target_arch guard in basically every repository that switched to it: https://github.com/search?q=stdarch_x86_avx512&type=code (ignore the files that themselves are x86 specific)

@AlexTMjugador
Copy link
Contributor Author

AlexTMjugador commented Feb 13, 2024

Indeed, you're right that the build failure for ARM was caused due to missing cfg_attr target guards. I idle speculated about the causes in a hurry without checking, so sorry about that. I wanted to help getting the ball rolling with a fix.

I've pushed a commit that should fix the build failure on those targets. I've tested that the crate works okay for AArch64 targets by using RUSTFLAGS="-C linker=aarch64-linux-gnu-gcc" CARGO_RUNNER="qemu-aarch64" cargo test --features nightly --target aarch64-unknown-linux-gnu on a Debian box set up for cross-compilation.

I've also tested that AVX512 instructions are emitted for x86-64 targets even when explicitly compiling with AVX512 support by running RUSTLFAGS="-C target-cpu=x86-64-v4" cargo asm --color --features nightly --lib "simd_adler32::imp::avx512::imp::update_imp" and confirming that AVX512 instructions were present, such as vextracti64x4.

The question on what to do with the MIPS target remains open, though. It's fair to expect more headaches with tier 3 targets.

@mcountryman
Copy link
Owner

Hey so I'm gonna merge this and cross my fingers that the CI pipeline doesn't release a new version.

I have some sweeping changes planned to clean-up some issues others and I have had unrelated to this issue. Likely soon a new release with these changes will be coming.

Thank you @AlexTMjugador for your time and effort!

@mcountryman mcountryman merged commit cc3155d into mcountryman:main Mar 28, 2024
14 of 15 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.

Fails to build on the latest nightly when using the nightly feature
3 participants