tkaitchuck / aHash

aHash is a non-cryptographic hashing algorithm that uses the AES hardware instruction
https://crates.io/crates/ahash
Apache License 2.0
1.03k stars 101 forks source link

Fragile build script: crate automatically enables "specialize" feature #216

Closed RalfJung closed 7 months ago

RalfJung commented 7 months ago

The build script of this crate automatically enables the "specialize"-feature, without any opt-in being required by crate users. That is a bad idea; it leads to issues such as https://github.com/tkaitchuck/aHash/issues/200: nightly features are subject to change without prior notice, and if that ever happens with specialize then this crate will start to fail building on nightly even for users that only want to use the stable subset of this crate.

Instead of automatically enabling nightly feature, the recommended practice is to expose a cargo feature that opts-in to nightly features. That way, crate users get control over whether they use only the stable part of also the unstable part of this crate. (This is what ahash already does e.g. for nightly-arm-aes.)

Also see https://github.com/SergioBenitez/version_check/pull/23.

tkaitchuck commented 7 months ago

That will not work. If one library wants to depend on the feature and then an application depends on that library it will fail to compile on stable. The example of nightly-arm-aes is a bad one because the removal of that check and switch to a feature flag caused horrible breakage on stable: #195

There needs to be a way to make the logic conditional on compiler availability.

Nemo157 commented 7 months ago

Rather than a feature flag one recommended approach is to use a raw --cfg opt-in, that will allow someone wanting to test the nightly feature to do so temporarily:

> RUSTFLAGS=--cfg=ahash-specialize cargo test

or permanently:

# in .cargo/config.toml
[build]
rustflags = [
  "--cfg=ahash-specialize",
]

And keeps the choice of enabling the feature entirely under the control of the final builder, not any intermediate libraries.

tkaitchuck commented 7 months ago

There is also a larger problem here: 'specialize' has been in nightly waiting to release for the last 8 years, and likely won't get into stable for at least another 3 years. This is creating a conflict between the idea the idea: "don't depend on nightly because things can change in a breaking way without notice" when applied to libraries and applications. Applications are mostly using on nightly because they don't want to use 8 year out of date code so they want everything to build reliability so they don't want libraries to depend on nightly because it means builds could break if nightly changes. Libraries are in the same position, where from their POV: "why is the application depending on nightly if they cannot tolerate breakage?"

There are a number of possible solutions:

tkaitchuck commented 7 months ago

@Nemo157 Have you ever looked up config flags to enable nightly features for your transitive dependencies? In practical terms what is the difference between that and just deleting the code?

Nemo157 commented 7 months ago

Have you ever looked up config flags to enable nightly features for your transitive dependencies?

No, because I want stability from my dependencies. I do have --cfg gated nightly features in my own crates, as a way of testing those features out myself to verify they will be useful once stabilized; but I don't try to push dependents towards using them (though they are free to do so if they notice them).

tkaitchuck commented 7 months ago

@Nemo157 Right, so in that case it is equivalent to deleting the code. As it stands aHash has hundreds of libraries and frameworks and thousands of apps which depend on it, and do so because of its performance. If they didn't care about performance they would all just use the default hasher. So I am not going to cause a large performance regression in all of those applications out of a purely hypothetical concern that someday the rust compiler team might decide that they don't like the keyword default and would prefer something else in its place.

If I am wrong, and this keyword is planned to change, or there is even a discussion about it please let me know and file an issue pointing to it, otherwise I consider this closed.

RalfJung commented 7 months ago

That will not work. If one library wants to depend on the feature and then an application depends on that library it will fail to compile on stable.

A library that enables the feature is nightly-only. If a library wants to be stable-compatible it may only enable the feature conditionally, controlled by its own "nightly" feature. This then moves up the dependency chain.

An application that depends on a library that needs a nightly feature obviously won't build on stable so I see no problem here.

There is also a larger problem here: 'specialize' has been in nightly waiting to release for the last 8 years, and likely won't get into stable for at least another 3 years. This is creating a conflict between the idea the idea: "don't depend on nightly because things can change in a breaking way without notice" when applied to libraries and applications. Applications are mostly using on nightly because they don't want to use 8 year out of date code so they want everything to build reliability so they don't want libraries to depend on nightly because it means builds could break if nightly changes. Libraries are in the same position, where from their POV: "why is the application depending on nightly if they cannot tolerate breakage?"

I don't understand your argument. Specialization is a highly experimental feature. If you want to use it obviously you need to use nightly. It doesn't matter for how long it has been experimental; it is still far from done. You can't just bully your way into getting it stabilized.

So I am not going to cause a large performance regression

Which regression? At worst this can cause performance regression for nightly users. That's completely acceptable, everyone who relies on things not regression uses stable anyway.

This issue should not be closed. This crate is a ticking time bomb of ecosystem breakage, as the stdsimd disaster showed. As it stands you are trying to use the fact that your crate is widely used to force the hand of rustc devs, to make us treat a feature as semi-stable when it isn't. That's very non-cooperative behavior.

RalfJung commented 7 months ago

The example of nightly-arm-aes is a bad one because the removal of that check and switch to a feature flag caused horrible breakage on stable: https://github.com/tkaitchuck/aHash/issues/195

(EDIT: my first analysis was wrong, I updated the comment)

That's a case of someone using an old nightly when building a crate with a nightly crate feature enabled. It makes no sense to blame this on https://github.com/tkaitchuck/aHash/pull/183. Of course when you use nightly features you have to use the latest nightly as nightly features keep changing.

workingjubilee commented 7 months ago

Clearly we should just remove the specialization feature, then, since it has simply been virtually unmaintained for 8 years, no one has proven it will be sound, even if implemented in a minimal way, and it will likely never be stabilized as-is.

RalfJung commented 7 months ago

Note that this crate only depends on min_specialization, not specialization. That's much more harmless. It is basically "obviously" sound. It is also extremely limited. The main blocker for stabilization is that there are concerns that some parts of its design might conflict with whatever the final form of "full" specialization looks like.

But the fact remains that it is an experimental feature and enabling that automatically without opt-in subverts the entire approach to nightly feature development in Rust -- harming our ability to keep developing the language.

tkaitchuck commented 7 months ago

A library that enables the feature is nightly-only. If a library wants to be stable-compatible it may only enable the feature conditionally, controlled by its own "nightly" feature. This then moves up the dependency chain. An application that depends on a library that needs a nightly feature obviously won't build on stable so I see no problem here.

It's still a regression from where the library is now.

I don't understand your argument. Specialization is a highly experimental feature. If you want to use it obviously you need to use nightly. It doesn't matter for how long it has been experimental; it is still far from done. You can't just bully your way into getting it stabilized.

I don't assume I can have any influence on when it is stabilized. In the meantime, I will use it conditionally when it is available, and not when it is have some other logic. People who wish to take the risk of breakage that inherently comes with using nightly can have it and those who don't wish to deal with instability should use stable and will see no problems.

This crate is a ticking time bomb of ecosystem breakage, as the stdsimd disaster showed.

To be clear this was breakage on stable rust. Code that compiles with 1.60.0 and 1.72.0 does not compile on 1.71.1. This was triggered by the PR attempting to avoid future problems on nightly. If you don't believe me, feel free to run the build yourself. To be clear the proposed alternative will be soon also be removed: https://github.com/tkaitchuck/aHash/pull/217

As far as it being a "ticking time bomb". When is the next change to the syntax going to be? Is there even discussion of changing it? Or are you just fear mongering? If it is actively under development, then it should be easy to simply create a PR to adapt to whatever changes are happening.

RalfJung commented 7 months ago

People who wish to take the risk of breakage that inherently comes with using nightly can have it and those who don't wish to deal with instability should use stable and will see no problems.

There seems to be a misunderstanding about how nightly works. One of the promises we make is that nightly is only unstable if you opt-in to instability. Crates like this one break that promise. There's a reason we require feature(...) attributes on nightly and don't just enable everything unconditionally: people need to know where they are relying on things that may break or otherwise change, and where not. The Rust feature development story would simply not work without that kind of control.

Nightly without any feature flags or -Z flags is intended to be entirely identical to a stable release. In fact that's how stable releases are generated -- we just block the use of these flags and otherwise use the exact same code as nightly.

As far as it being a "ticking time bomb". When is the next change to the syntax going to be? Is there even discussion of changing it? Or are you just fear mongering? If it is actively under development, then it should be easy to simply create a PR to adapt to whatever changes are happening.

This is not just about syntax changes but about any breaking changes. We don't have a schedule for breaking changes in nightly features since we do them without any planning or warning. That's the entire point of nightly.

I am not fear mongering, I am worried about crates like this causing ossification of unstable features. This crate has already caused entirely avoidable and pretty much unprecedented wide-spread nightly ecosystem breakage once (the stdsimd issue), the pain from that is ongoing. I am sure I will see ahash show up in build failures for some time to come. I'd prefer if it didn't happen again.

To be clear this was breakage on stable rust.

How can a feature that people only enable on nightly cause breakage on stable Rust? Seems like the crate feature was implemented or used incorrectly then. (I found no summary of the actual problem yet so I have to guess here.)

tkaitchuck commented 7 months ago

This really isn't about nightly. We have one very minor compile break on nightly, but 3 huge compilation breaks on stable, and it took weeks to sort out. From my point of view, your proposed cure is the disease. Moving this to a discussion here: https://github.com/tkaitchuck/aHash/discussions/220

RalfJung commented 7 months ago

We have one very minor compile break on nightly

https://github.com/tkaitchuck/aHash/issues/200 was a major compile break on nightly. It affects every build that involves this crate. It caused issues during rustc development (since ahash is in the rustc dependency tree, that's why https://github.com/tkaitchuck/aHash/pull/183 got filed), and then it caused a major disruption for many of your users (see all the backlinks in #200). Basically every project will have to update their lockfile to remain nightly-comatible, even though they didn't ask for nightly features. That's a failure of Rust's stability promise, caused by a nightly feature change, which can only come about because of automatically enabled nightly features, which violate the basic principles of how Rust nightly features work.