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

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

Closed
RalfJung opened this issue Mar 1, 2024 · 14 comments
Closed

Comments

@RalfJung
Copy link

RalfJung commented Mar 1, 2024

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 #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 SergioBenitez/version_check#23.

@tkaitchuck
Copy link
Owner

tkaitchuck commented Mar 1, 2024

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
Copy link

Nemo157 commented Mar 1, 2024

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
Copy link
Owner

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:

  • Normalize Beta as an intermediate level of stability, so people don't all pile into nightly.
  • Continue to use nightly but change the name of a feature when it changes in a backward incompatible way after a long time.
  • Avoid making things available in nightly if they lack a path to release

@tkaitchuck
Copy link
Owner

@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
Copy link

Nemo157 commented Mar 1, 2024

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
Copy link
Owner

@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
Copy link
Author

RalfJung commented Mar 2, 2024

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
Copy link
Author

RalfJung commented Mar 2, 2024

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

(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 #183. Of course when you use nightly features you have to use the latest nightly as nightly features keep changing.

@workingjubilee
Copy link

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
Copy link
Author

RalfJung commented Mar 2, 2024

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
Copy link
Owner

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: #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
Copy link
Author

RalfJung commented Mar 4, 2024

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
Copy link
Owner

tkaitchuck commented Mar 5, 2024

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: #220

@RalfJung
Copy link
Author

RalfJung commented Mar 5, 2024

We have one very minor compile break on nightly

#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 #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.

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

No branches or pull requests

4 participants