dalek-cryptography / curve25519-dalek

A pure-Rust implementation of group operations on Ristretto and Curve25519
Other
853 stars 422 forks source link

Fix nightly build #619

Closed jimmygchen closed 5 months ago

jimmygchen commented 5 months ago

Addresses #618.

This PR fixes the failing build on the latest Rust nightly compiler rustc 1.78.0-nightly (f067fd608 2024-02-05).

The stdsimd feature has been split into separate features in the latest Rust nightly compiler and its usage needs to be updated to use #![feature(stdarch_x86_avx512)] instead.

See https://github.com/rust-lang/stdarch/pull/1486 and https://github.com/rust-lang/rust/issues/111137.

rozbb commented 5 months ago

Thank you, this looks great! For anyone reading: we have two SIMD implementations for x86_64—AVX2 and AVX-512. The former has already been stabilized, so it's not an issue. The latter is unstable, and was previously gated by a nightly feature stdsimd. As @jimmygchen said, this feature has been split into more fine grained features. So, we get errors in #618 like

error[E0658]: use of unstable library feature 'stdarch_x86_avx512'
  --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/curve25519-dalek-4.1.1/src/backend/vector/ifma/field.rs:25:9
   |
25 |     use core::arch::x86_64::_mm256_madd52lo_epu64;
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #111137 <https://github.com/rust-lang/rust/issues/111137> for more information
   = help: add `#![feature(stdarch_x86_avx512)]` to the crate attributes to enable
   = note: this compiler was built on 2024-02-05; consider upgrading it if it is out of date

because _mm256_madd52lo_epu64 is behind that new AVX-512 gate.

rozbb commented 5 months ago

@tarcieri happy to merge. Shall I also cut a new version (minor or patch?)?

leighmcculloch commented 5 months ago

Thanks for the fast fix.

Which version is this targeting?

rozbb commented 5 months ago

This will be a patch bump (ie +0.0.1) for curve-, ed-, and x- crates. Sound good @tarcieri ?

tarcieri commented 5 months ago

Yep, sounds good

sergiimk commented 5 months ago

This change unfortunately breaks the build for older nightlies.

Our company is probably not the only one who uses nightly rustc not just for testing, but for access to unstable features, so we don't upgrade rustc that often and still running:

rustc 1.76.0-nightly (90e321d82 2023-12-02)

Don't know if this particular change could've been done in a backwards-compatible way or not, but just wanted to mention that the latest patch release likely both fixed AND broke a lot of builds.

tarcieri commented 5 months ago

@sergiimk this is a breaking change in the nightly compiler. It requires a corresponding breaking change in this library to support.

The nightly compiler is definitionally the unstable version of the compiler. Breaking changes can come at any time, the onus is on you to keep what you're building on it stable via meticulous dependency management.

rozbb commented 5 months ago

Is there an accepted way to handle the following scenario? You have a crate that relies on nightly. You pull in curve25519-dalek as a dep, but you don't really care about performance. Can you force dalek to behave as it would in stable, while still using nightly for your own crate?

tarcieri commented 5 months ago

I think it's unreasonable to ask for support for anything but the latest version of the nightly compiler

rozbb commented 5 months ago

Right, what I was thinking is something along the lines of a negative feature like no-nightly which basically overrides our toolchain autodetect and skips all nightly features.

IIRC we had a nightly feature we killed. So not sure if the inverse is any better. Just an idea I was wondering if there's precedence for

tarcieri commented 5 months ago

@rozbb disabling the nightly cfg attribute would accomplish that. It gets set automatically in build.rs here: https://github.com/dalek-cryptography/curve25519-dalek/blob/40cf5aff99ef455acf62ff2d83a340ebaddf77e4/curve25519-dalek/build.rs#L29

rozbb commented 5 months ago

Ah nice, that answers it then