Forbid setting RUSTC_BOOTSTRAP from a build script on stable

[jyn514]

Instead, recommend RUSTC_BOOTSTRAP=crate_name. If cargo is using a nightly toolchain, or if RUSTC_BOOTSTRAP was set in cargo's build environment, the error is downgraded to a warning, since the variable won't affect the build.

This is mostly the same as suggested in #7088 (comment), except that RUSTC_BOOTSTRAP= values other than 1 are treated the same as RUSTC_BOOTSTRAP=1. My reasoning was that rust-lang/rust#77802 is now on 1.50 stable, so some crates may have started using it, and I would still prefer not to give hard errors when there's no workaround.

Closes #7088.

r? @joshtriplett

[jyn514]

Note that this currently mucks around with the test suite, I couldn't figure out how to enable nightly features from an integration test: 03b2b51#diff-7b081a836e71ae12b28f42ba8f09384036be9ae8ee479b4849f43a8522e94b37R715. Maybe @Eh2406 has suggestions?

[joshtriplett]

Other than the testsuite issue, this looks good overall. And the verbiage seems reasonable to me.

[ehuss]

It looks like the issue with nightly_features_allowed is that it uses a thread-local value. When Cargo starts the compiling phase, each "Work" job runs in a separate thread, and those threads don't know about the masquerade_as_nightly_cargo override. That was done to make it work with unit-tests, but causes the problem you ran into.

I can think of a few different approaches:

• You can capture the nightly_features_allowed state outside of the Work closure (from the main thread where it is set correctly).
• Remove the thread-local stuff, and just make it a property of Config. I don't see anywhere that needs it without a Config, and that would get rid of the global. However, this would be a larger refactoring. This would be my slight preference (not a big fan of globals), but I'm not sure if you're up to doing that.
• Remove the thread-local part, but keep it as a global, and just disable the unittests that need it on nightly (per my suggestion at cargo v0.27 tests failed when build rust 1.26.2 on "stable" channel #5648 (comment)). I think I'd prefer either of the previous two options, though.

[jyn514]

Remove the thread-local stuff, and just make it a property of Config. I don't see anywhere that needs it without a Config, and that would get rid of the global. However, this would be a larger refactoring. This would be my slight preference (not a big fan of globals), but I'm not sure if you're up to doing that.

I'm happy to try that. That would remove NIGHTLY_FEATURES_ALLOWED and ENABLE_NIGHTLY_FEATURES? Should I add both to config and keep most of the logic in nightly_features_allowed or just have a single boolean?

[jyn514]

Remove the thread-local stuff, and just make it a property of Config. I don't see anywhere that needs it without a Config, and that would get rid of the global. However, this would be a larger refactoring. This would be my slight preference (not a big fan of globals), but I'm not sure if you're up to doing that.

I tried doing this, but Config isn't thread-safe, so I can't pass it to Work::new. For now I think I'll capture the state outside of the closure.

[jyn514]

This is ready for review.

[bors]

☔ The latest upstream changes (presumably #9175) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #9184) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

I think the thread_local! in features.rs can be removed.

Just as a stylistic choice, I would probably make the free functions nightly_features_allowed, maybe_allow_nightly_features, and enable_nightly_features as methods of Config instead. It's not necessary, but if you're so inclined I think it might be a little clearer.

I'm also a little concerned about frequently recomputing the channel. I was thinking it could be simplified a little by removing Config.maybe_allow_nightly_features by capturing the status when the Config is created. So it could be something like this in Config::new:

let nightly_features_allowed = is_nightly();

And only store that in Config. Then, the methods could be something like this:

fn nightly_features_allowed(&self) -> bool {
    self.nightly_features_allowed
}

fn set_nightly_features_allowed(&mut self, allowed: bool) {
    self.nightly_features_allowed = allowed;
    // This could maybe re-initialize things like nightly cli flags, but probably not necessary.
}

fn is_nightly() -> bool {
    matches!(channel(), "nightly" | "dev");
}

maybe_allow_nightly_features isn't really necessary if it is no longer global.

---

The comment at the top of build_script_env.rs may need to be changed.

[jyn514]

I think the thread_local! in features.rs can be removed.

Thanks, done.

I removed enable_nightly_features() and maybe_enable_nightly_features() since they're not doing much anymore. Instead I just used a field on Config that gets set once based on the channel at startup. I also changed a test to manually set the channel to stable, since it assumed tests were on the stable channel by default.

[jyn514]

Test failure looks spurious: E: Failed to fetch http://security.ubuntu.com/ubuntu/dists/focal-security/main/i18n/Translation-en.xz File has unexpected size (111640 != 111832). Mirror sync in progress? [IP: 91.189.91.39 80]

[ehuss]

Sorry, you had a question about the link (sometimes GitHub doesn't have a response field for inline comments).

Do you think I should link https://rustc-dev-guide.rust-lang.org/building/bootstrapping.html#complications-of-bootstrapping instead, or just remove the link altogether?

If there were a link, I would probably use https://github.com/rust-lang/cargo/issues/7088 if anything. The build script docs don't talk about bootstrapping or setting env vars before cargo, and other docs I think may be confusing. #7088 describes the motivation for the restriction and provides plenty of links and discussion if anyone wants to know more about it. Hopefully this only impacts a very tiny number of projects, so I don't think it is super important to be any sort of formal documentation. I personally would not include a link, but up to you!

BTW, I appreciate you taking the time to refactor all the nightly flag stuff, as that wasn't really a core part of the change, but I do think it helps clean things up and make testing nicer.

[jyn514]

I personally would not include a link, but up to you!

I left out the link for now. I think the projects that would be most impacted have already heard about it, and we can always add it later if someone is confused.

BTW, I appreciate you taking the time to refactor all the nightly flag stuff, as that wasn't really a core part of the change, but I do think it helps clean things up and make testing nicer.

You're very welcome!

I think this is ready to merge :)

[ehuss]

Thanks!

@bors r+

[bors]

📌 Commit 0b18165 has been approved by ehuss

[bors]

⌛ Testing commit 0b18165 with merge b219f0e...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing b219f0e to master...

[jyn514]

Sorry, you had a question about the link (sometimes GitHub doesn't have a response field for inline comments).

@ehuss Took me ages but I figured this out - if you start a review, GitHub will only let you reply to existing comments if the response becomes part of the review, so you have to be in the diff view instead of the overview tab.