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

cbindgen: Disable tests rather than abusing RUSTC_BOOTSTRAP #65303

Closed
wants to merge 1 commit into from

Conversation

SimonSapin
Copy link

Motivation for this change

Since #64733, the packaging of cbindgen uses the RUSTC_BOOTSTRAP environment variable.

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 harmful

By 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 that cargo build works correctly. I am told that doCheck = false is the way to make the nix packaging disable cargo test for a Rust package, but I have not tried it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@symphorien
Copy link
Member

Would running cargo build without RUSTC_BOOTSTRAP=1 but running cargo test with it be acceptable ? This way the build artifacts are built with stable rust but we still run tests.

@SimonSapin
Copy link
Author

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.

@andir
Copy link
Member

andir commented Jul 24, 2019

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.

@nox
Copy link

nox commented Jul 24, 2019

Note that RUSTC_BOOTSTRAP is already abused elsewhere, and that should probably be fixed too.

@SimonSapin
Copy link
Author

@andir When passing a -- argument to cargo test, the rest of the arguments are passed through to the test executable. So yes it’s possible to add runtime flags to the test suite. I’m not planning to work on that though.

@andir
Copy link
Member

andir commented Jul 25, 2019

@andir When passing a -- argument to cargo test, the rest of the arguments are passed through to the test executable. So yes it’s possible to add runtime flags to the test suite. I’m not planning to work on that though.

I started implementing checkFlags for buildRustPackage so we can restrict the tests that will be run.

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

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@meithecatte
Copy link
Contributor

What's the status here? If I understood correctly, the usage in cbindgen is resolved following #70259, but 10 other packages still use RUSTC_BOOTSTRAP.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 23, 2020
@andir
Copy link
Member

andir commented Jun 23, 2020

Someone has to put in the time to change all the other packages. I will most likely not invest that time.

@Ekleog
Copy link
Member

Ekleog commented Jan 11, 2021

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.

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 rustc the packages are built, and people overriding it already know what they are doing. As such, normal users cannot have such breakage happen regardless of whether new versions of rustc break formerly-valid tests without us knowing about it (via hydra failing CI for these packages).

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants