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
cbindgen: Disable tests rather than abusing RUSTC_BOOTSTRAP #65303
Conversation
Would running cargo build without |
That would be no different from #64733. It’s not about the final executable being “pure”. It’s about creating a situation where your process is gonna start failing when you update your toolchain to a version where a breaking change was made to an unstable feature, and normalizing other projects doing the same. It’s an important goal of Rust that updating from one toolchain to the next on the Stable release channel be as painless as possible. There is a strong promise of stability, but it needs boundaries to be viable. Experimental stuff only being available when one knowingly opts into the Nightly channel (or compiles their own rustc) is part of that story. |
Just briefly read through this. @SimonSapin Thank you for taking the time and looking into this! Very appreciated. I think FireFox was not the driving force behind having it in nixpkgs but one of the main consumers these days :-) Is it possible to introduce (test) flags in cbindgen to disable tests that need nightly features? Maybe even detect if you are running on a nightly rustc version. That way we could still run most of the tests and catch errors but would not need to resort to these kinds of hacks. |
Note that
|
@andir When passing a |
I started implementing My current WIP can be seen at https://github.com/NixOS/nixpkgs/compare/master...andir:rust-checkflags?expand=1 Going this route is probably better then disabling tests and/or requiring |
Thank you for your contributions.
|
What's the status here? If I understood correctly, the usage in cbindgen is resolved following #70259, but 10 other packages still use |
Someone has to put in the time to change all the other packages. I will most likely not invest that time. |
I think that this statement, while true, is missing the fact that nixpkgs is not a “normal” package repository: we statically know with which version of This being said, I would love for RUSTC_BOOTSTRAP to not have to be used. However, disabling tests altogether is, for the nixpkgs project itself, a bigger issue than using RUSTC_BOOTSTRAP, due to its aforementioned particularity, as we have a choice between something we know builds and works, and something we know builds and can only hope works. I think the case presented in this PR is a great one, and I would love for us to be able to get rid of RUSTC_BOOTSTRAP (eg. by patching all upstream software so it no longer requires nightly, or for cases where it's needed only for tests by introducing a feature flag for nightly-requiring tests), but it is going to be a lot more work. As such, in the current state of things, I think the best we can do would be to add comments around uses of RUSTC_BOOTSTRAP that explicitly state that no other project should use it. And then, as upstream projects become more compatible with stable Rust, to slowly remove these points of use. Another solution would be to package one instance of Rust nightly, but this has already been decided against due to the update churn it implies. I'm not sure whether this issue gives enough new information to relitigate this previous choice (by eg. keeping a less-regularly updated nightly), but if people think this should happen I think it'd deserve some discussion on the Discourse first and then either in an issue or in a PR that'd implement such an addition. Anyway, seeing as this PR cannot be landed as is (because the problem it tackles has already been solved in #70259), I am going to close it so it reduces our unactionable PR backlog. Sorry about it, and hope the suggested alternative roads will inspire some contributions! And thank you everyone for the feedback :) |
Motivation for this change
Since #64733, the packaging of
cbindgen
uses theRUSTC_BOOTSTRAP
environment variable.nixpkgs/pkgs/development/tools/rust/cbindgen/default.nix
Line 19 in 681d3c2
Background on Rust
The Rust programming language has a concept of “unstable features”. They are APIs, language features, or tool parameters whose designed is not completed yet. They may still change in incompatible way, potentially breaking users relying on them.
Having them at all in the implementation allows experimenting and iterating, rather than freezing a design while it’s still only theoretical. It is important for the long-term health of the language that it can keep experimenting this way.
To avoid unstable features becoming de-facto stable (if enough users start depending on them that there is pressure not to change them and keep an unfinished design), these features are not available in normal releases of the compiler, only on Nightly versions. And they require explicit opt-in.
rustc is a bootstrapping compiler: it is written in Rust and relies on some unstable features. In order to make it possible to build each release with the previous one, rustc’s build system uses the
RUSTC_BOOTSTRAP
environment variable to unlock unstable features despite using a bootstrap version of rustc that normally does not allow them. There is non-trivial work involved to keep all this working when breaking changes are made to unstable features.Using
RUSTC_BOOTSTRAP
in another project technically allows using unstable features with a compiler that normally doesn’t, but this environment variable was never intended for anything other than building rustc itself.Abusing
RUSTC_BOOTSTRAP
is harmfulBy relying on it anyway, NixOS contributes to normalize other projects relying on it and counting on unfinished features not changing. If enough projects adopt this strategy, this puts pressure on the Rust team to declare unfinished features to be de-facto stable, and robs them of their opportunity for experimentation.
Please don’t contribute to spoiling it for everybody.
Background on cbindgen
An optional feature of cbindgen (expanding macros) involve shelling out to
rustc
with some unstable options. Therefore this requires Rust Nightly. Building Firefox requires cbindgen, but not macro-expansion feature. It is a goal of Firefox that it can be built with stable rustc. I expect that Firefox is the motivation to package cbindgen in NixOS.Some of cbindgen’s own test suite exercise the macro-expansion feature, and therefore fails when run on stable rustc: mozilla/cbindgen#338. But it’s only some tests.
This PR
I have reproduced mozilla/cbindgen#338 by running
cargo test
in cbindgen, and verified thatcargo build
works correctly. I am told thatdoCheck = false
is the way to make the nix packaging disablecargo test
for a Rust package, but I have not tried it.Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)