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

Require an opt-in for enabling min_specialization rustc feature #238

Open
alexcrichton opened this issue Jun 25, 2024 · 4 comments · May be fixed by #239
Open

Require an opt-in for enabling min_specialization rustc feature #238

alexcrichton opened this issue Jun 25, 2024 · 4 comments · May be fixed by #239

Comments

@alexcrichton
Copy link

I'm in the process of adding a new tool to the Rust compiler but it's failing to build on CI. The root of the reason appears to be that this crate will automatically enable usage of the min_specialization feature on Nightly-looking Rust channels but the way that Rust is compiling crates makes it so the min_specialization feature isn't detected but this crate doesn't catch it. This is distinct from 76dd8d2 in that -Zallow-features is used as part of the rustc build but it's used in a non-default way. Overall the end-state is that this crate is causing a build failure as a transitive dependency of the tool I'm adding.

Given that context I'd like to pose a question, how would you feel about making usage of min_specialization require an opt-in from users rather than implicitly enabling it on supporting Nightly channels? That would look like a crate feature, a required RUSTFLAGS entry, or perhaps an environment variable at build time.

In addition to the rationale of working as part of rust-lang/rust I'd also offer the rationale that Nightly Rust features can theoretically break at any time and users using a Nightly toolchain may not expect breakage from crates because unstable features change. Requiring an explicit opt-in would help with that since then only those opting-in would be broken which would be expected.

I'm happy to make a PR for this as well, but wanted to gauge interest in such a change before doing so to make sure I wouldn't step on any toes.

alexcrichton added a commit to alexcrichton/aHash that referenced this issue Jul 8, 2024
This commit switches away from implicitly enabling the `min_specialize`
Rust nightly feature whenever a Nightly compiler is used to instead
requiring an explicit opt-in with a new Cargo feature
`nightly-specialize`. The goal of this commit is to fix tkaitchuck#238 and has two
primary motivations:

1. In tkaitchuck#238 I'm trying to build something that depends on this crate as
   part of the Rust bootstrap process but this crate fails to build due
   to `min_specialize` not being allowed but a nightly compiler is in
   use. This is due to the fact that the way `-Zallow-features` is
   managed in the bootstrap is different than the standard Cargo way of
   doing so.

2. This removes a failure mode where if one day the `min_specialize`
   feature changes this crate won't break when built on nightly. Users
   of Nightly compilers will be able to continue using this crate if the
   feature was not explicitly opted-in to.
@alexcrichton alexcrichton linked a pull request Jul 8, 2024 that will close this issue
@tkaitchuck
Copy link
Owner

Are you saying that you have a build of the compiler where: version_check::supports_feature("specialize") returns true, but where the feature is not actually supported? Is that a bug in the compiler, in version_check, or an oddity due to running with a custom build of the compiler? Is there a different way to detect this that would not give incorrect results?

Longer term, I am planning a 1.0 release of the crate which will remove all specialization. The current design of the feature makes code using it way too complex and verbose to be practically maintainable. My plan to avoid the performance hit is just to require the application to invoke different methods to instantiate the Builder because in general the type of the key of a map is almost always known at the instantiation site.

Obviously a new version won't affect exiting packages which depend on existing versions of the crate. Do you need a work around for existing package versions?

@alexcrichton
Copy link
Author

More-or-less that's the situation yeah. When the build script is invoked in the Rust build system the RUSTFLAGS setting doesn't 100% reflect the flags the crate is going to build with. The build script thinks it has access to all nightly features but the crate itself is compiled with an extra -Zallow-features flag which doesn't actually list the feature. This is due to build-system-of-rust-itself shenanigans and I believe is difficult to change.

As for the time being no workaround is urgent as things have already landed in rust-lang/rust#126967 where extra features were allowed for specialization here. Due to the tool not actually needing specialization or performance wins it would ideally be best to remove the need for specialization to avoid unnecessary dependencies.

@tkaitchuck
Copy link
Owner

Would this fix the detection issue? SergioBenitez/version_check#22

@alexcrichton
Copy link
Author

Unfortunately I think not, not because that PR doesn't work though. It's that the compiler flags the build scripts sees are different than the crate itself due to the way the bootstrap build works

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 a pull request may close this issue.

2 participants