MSRV and adopting new features.

[tkaitchuck]

In order to support LTS, many libraries work with a fixed version of the compiler not taking advantage of any new capability or feature. Specifically users do not like when the MSRV is increased in a patch release, and they would like to continue working with newer versions of the library without updating their compiler version.

Other users are very up to date, and nearly immediately after any relevant feature or capability lands in nightly there is someone opening an issue or a PR requesting it.

Right now if the code mentions or attempts to use a feature on a compiler version that is unaware of that feature results in a compile error. Ideally it would be possible to use cfg to only include code based if the feature is available. This way if a feature is released in one version, users running that version of the compiler can use it right away, while users of older compilers still get working code. This is currently not allowed.

As a workaround, a hack was introduced. A build.rs file can be used to detect if the compiler is nightly. This is not as good, because a feature may be released, but if was introduced in a version above the MSRV only nightly users can use even though it is in stable. But this appears to be the best than can be done for now. The idea is people who are running nightly compilers which are up to date, and will use and want the newer and more advanced features. Everyone else will get only the features available as of the MSRV.

Today aHash is effectively forked with one codepath that compiles with the latest and greatest for those who want the new capabilities, and one that is permanently tied to Rust 1.60.0. This was not so much planned but where things have ended up.

In that context here is a timeline of recent stdsimd breakage:

1. After support for AES or ARM lands in nightly there is an immediate PR to add support for it. This is done using the same build.rs trick to detect nightly. Everything works fine on all versions of Rust.
2. A PR is introduced to remove the build.rs flag and replace it with a feature flag. This is done to prevent future breakage of nightly as the feature is planned to be renamed. (And it is successful at this)
   • This introduces a bug. The PR author assumed for the intrinsics were stable on 1.72.0 it was ok to have the not behind the flag. I assumed that because the MSRV presubmit check against 1.60.0 passed that any version after 1.60.0 would work. It did not. The code failed to build on 1.71.1.
3. Users open multiple issues because aHash is failing to build with stable versions of rust. For example: compile error: use of unstable library feature 'stdsimd' #195
4. A patch is released to bump the MSRV to 1.72.0 as the code is working with that version and newer. This breaks many more people. Before most versions between 1.60 and 1.72 would build and only 1.71 was failing, but the MSRV check is uncircumventable so it resulted in compile failures for many more users which they were helpless to work around.
   (Note that this is a build failure in stable Rust involving released features, and nightly has nothing to do with it)
5. An emergency patch is released which hides the stable code behind the nightly feature flag so that it will build on 1.71.1 and the MSRV is lowered back down to 1.60.0. This allows stable to build again (assuming you don't enable the feature on certain rust versions).
6. Nightly rust is updated to land the rename of stdsimd feature which the originaly was introduced to prevent. Because the latest release was already working without the feature everything continued to work with the latest released versions. However there were people running against 0.7.6 which was 3 years old. (The difference between 0.7 and 0.8 only relates to very advanced features that would not affect 99% of users.) As a result builds failed for people specifically using the latest nightly with a very out of data ahash, but anyone affected could just bump the version and rebuild.
7. A patch version for the 0.7 branch was released to mitigate any failures on nightly automatically.

The current status is that aHash builds everywhere but stable features are being hidden behind nightly flags to avoid compile errors. The current plan is to release a 1.0.0 version which bumps the MSRV to 1.72.0 which removes the feature flag.

In the longer term it would be much better if this could be handled with conditional compilation. This would avoid not only the need to have flags and document them to users, but this would also have avoided the whole category of breakage on stable that occured in steps 2, 3, and 4 above. Additionally even if the PR in 1 were never created in the first place no code would have ever been broken because when the rename of the feature occured it would have simply disabled the codepath. This would be MUCH better than what we have now.

[joshlf]

As a workaround, a hack was introduced. A build.rs file can be used to detect if the compiler is nightly. This is not as good, because a feature may be released, but if was introduced in a version above the MSRV only nightly users can use even though it is in stable. But this appears to be the best than can be done for now. The idea is people who are running nightly compilers which are up to date, and will use and want the newer and more advanced features. Everyone else will get only the features available as of the MSRV.

I might be misunderstanding your use case or constraints, but I don't believe the only option is to detect nightly vs non-nightly. In zerocopy, we have the same problem, and we solve it by also detecting the stable version (via build.rs) and emitting different --cfgs for different features that are stable as of that version. Here's our list of Rust version-dependent features.

Would that approach be viable for ahash?

[tkaitchuck]

That would be a meaningful improvement from where we are now. It wouldn't have caught the error in 3 above, because as you can see here: https://github.com/tkaitchuck/aHash/actions/runs/7859222114/job/21445073070
It is passing on both older and newer versions of Rust. But yes, that is definitely worth looking into.

Have you considered generalizing this to work as a drop-in dev dependency?

[tkaitchuck]

There is a crude version of this here: https://github.com/DerickEddington/cfg_rust_features
It only has a few features and appears to work via a compilation probe, which might be undesirable from a build time POV.

[joshlf]

I don't think I have the cycles to generalize and publish it, but you're more than welcome to just copy-paste our build.rs (it's permissively licensed), and anyone reading this thread is more than welcome to make a crate out of the idea!

(Also, it'd need to be a non-dev dependency since it's invoked from build.rs when being built by your dependents.)

[joshlf]

There is a crude version of this here: https://github.com/DerickEddington/cfg_rust_features It only has a few features and appears to work via a compilation probe, which might be undesirable from a build time POV.

Yeah, IMO a standalone build.rs is the way to go here. Take a look at ours - the logic is surprisingly simple (it doesn't need to parse TOML completely, just be able to do enough to yank out some key/value pairs with a very simple, fixed format).

[RalfJung]

• A PR is introduced to remove the build.rs flag and replace it with a feature flag. This is done to prevent future breakage of nightly as the feature is planned to be renamed. (And it is successful at this)

Is that #183?

• This introduces a bug. The PR author assumed for the intrinsics were stable on 1.72.0 it was ok to have the not behind the flag. I assumed that because the MSRV presubmit check against 1.60.0 passed that any version after 1.60.0 would work. It did not. The code failed to build on 1.71.1.

Why is it building on 1.60 but not on 1.71? That shouldn't usually happen, except if the build script detect 1.71 and uses that to enable more stuff. (memoffset does that, and then of course one has to have all the "threshold" versions where new stuff gets enabled on CI.)

[Nemo157]

This happens because there is a target_feature = "aes" gate on the code, on 1.60 that was not set, 1.71 added the target feature but with the functions it allows unstable, 1.72 then stabilized those functions

> rg -m1 -C8 aarch64 src/operations.rs
106-    use core::arch::x86_64::*;
107-    unsafe {
108-        let value = transmute!(value);
109-        transmute!(_mm_aesenc_si128(value, transmute!(xor)))
110-    }
111-}
112-
113-#[cfg(any(
114:    all(target_arch = "aarch64", target_feature = "aes", not(miri)),
115-    all(feature = "nightly-arm-aes", target_arch = "arm", target_feature = "aes", not(miri)),
116-))]
117-#[allow(unused)]
118-#[inline(always)]
119-pub(crate) fn aesenc(value: u128, xor: u128) -> u128 {
120:    #[cfg(target_arch = "aarch64")]
121:    use core::arch::aarch64::*;
122-    #[cfg(target_arch = "arm")]

> rustc +1.60 --print cfg --target aarch64-apple-darwin | rg aes

> rustc +1.71 --print cfg --target aarch64-apple-darwin | rg aes
target_feature="aes"

[Nemo157]

(So to me this just looks like an accidental bug caused by testing MSRV support of complex cfg being hard, the only real solution I see is doing exhaustive testing of all supported versions and targets)

[joshlf]

(So to me this just looks like an accidental bug caused by testing MSRV support of complex cfg being hard, the only real solution I see is doing exhaustive testing of all supported versions and targets)

Yeah I think this is basically what you have to do. It's a pain and requires a good amount of CI dev work, but it's the only way to be confident that things will work as you expect.

For an example of how to do this, take a look at zerocopy's CI config. In particular, look at the toolchains axis of the matrix. Those correspond to toolchain meta-names in our Cargo.toml. We have a CI test that parses both the CI config and Cargo.toml and makes sure that every feature we use in Cargo.toml is present in the toolchains axis. Writing all of that was obviously a giant pain, but it makes CI very robust for catching things like this.

[RalfJung]

This happens because there is a target_feature = "aes" gate on the code, on 1.60 that was not set, 1.71 added the target feature but with the functions it allows unstable, 1.72 then stabilized those functions

Ah, good catch! So the stable regression here is entirely unrelated to the questions around build script nightly detection.