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

allow overwriting the output of rustc --version #124339

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Apr 24, 2024

Our wonderful bisection folk have to work around crates that do incomplete nightly/feature detection, as otherwise the bisection just points to where the feature detection breaks, and not to the actual breakage they are looking for.

This is also annoying behaviour to nightly users who did not opt-in to those nightly features. Most nightly users want to be in control of the nightly breakage they get, by

  • choosing when to update rustc
  • choosing when to update dependencies
  • choosing which nightly features they are willing to take the breakage for

The reason this breakage occurs is that the build script of some crates run rustc --version, and if the version looks like nightly or dev, it will enable nightly features. These nightly features may break in random ways whenever we change something in nightly, so every release of such a crate will only work with a small range of nightly releases. This causes bisection to fail whenever it tries an unsupported nightly, even though that crate is not related to the bisection at all, but is just an unrelated dependency.

This PR (and the policy I want to establish with this FCP) is only for situations like the version_check's supports_feature function. It is explicitly not for autocfg or similar feature-detection-by-building-rust-code, irrespective of my opinions on it and the similarity of nightly breakage that can occur with such schemes. These cause much less breakage, but should the breakage become an issue, they should get covered by this policy, too.

This PR allows changing the version and release strings reported by rustc --version via the RUSTC_OVERRIDE_VERSION_STRING env var. The bisection issue is then fixed by rust-lang/cargo-bisect-rustc#335.

I mainly want to establish a compiler team policy:

We do not consider feature detection on nightly (on stable via compiler version numbering is fine) a valid use case that we need to support, and if it causes problems, we are at liberty to do what we deem best - either actively working to prevent it or to actively ignore it. We may try to work with responsive and cooperative authors, but are not obligated to.

Should they subvert the workarounds that nightly users or cargo-bisect-rustc can use, we should be able to land rustc PRs that target the specific crates that cause issues for us and outright replace their build script's logic to disable nightly detection.

I am not including links to crates, PRs or issues here, as I don't actually care about the specific use cases and don't want to make it trivial to go there and leave comments. This discussion is going to be interesting enough on its own, without branching out.

@rustbot
Copy link
Collaborator

rustbot commented Apr 24, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 24, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 24, 2024

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 24, 2024

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 24, 2024
@rust-log-analyzer

This comment has been minimized.

@wesleywiser
Copy link
Member

I have a few concerns with this approach:

  1. By linting on MIR build of the version_check::supports_feature, I believe this means we'll generate this lint anytime the crate is built even if the function isn't used. There are ways to use version_check correctly (such as using it to check if rustc's version is a specific stable release number and then enabling the use of language/libstd features stabilized in that release) so incorrectly linting on cases that are written correctly seems very bad.
    • We could instead lint on calls to version_check::supports_feature which would be resolve that particular issue and provide a lint span that is far more actionable than the current experience (most commonly "an indirect dependency depends on version_check but we won't tell you which one(s)").
  2. For crate authors that really want to do this approach, I suspect they will simply copy the relevant parts out of version_check directly into their build.rs and then remove the dependency, nullifying the lint. As you mention, there are other crates as well that can be problematic when trying to perform bisections. I would rather see a more comprehensive solution that doesn't specifically target one crate in particular.
    • For instance, we could support a RUST_ANTI_BOOTSTRAP environment variable which would cause the compiler to report itself as a stable compiler and ignore the RUST_BOOTSTRAP environment variable. Perhaps more usefully, RUSTC_PRETEND_TO_BE="whatever rustc --version output you want here" would allow completely overriding the normal version output. cargo-bisect-rustc could then be taught to set these flags automatically for the user. Something like this would resolve the issue for a number of similar crates without singling any of them out.

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 25, 2024

We already have RUSTC_FORCE_RUSTC_VERSION. I have added a test for it and made cargo-bisect-rustc support it with just a flag: rust-lang/cargo-bisect-rustc#335

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 25, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@petrochenkov
Copy link
Contributor

I read the description, but I still don't understand what is version_check::supports_feature, how it is related to the compiler, and what this PR does exactly.

@jackh726
Copy link
Member

jackh726 commented Apr 25, 2024

I'm a little...lost here. Digging around, if I understand correctly: ahash does feature detection to enable stdsimd on nightly, which was removed in recent nightlies. And because of that, there are crates that will no longer compile on the latest nightly (but will on the latest stable). In this case, this made finding a regression commit difficult.

The proposed policy in this FCP is to not just discourage this use but actively prevent it? That definitely seems a bit too far for me.

I think it definitely makes sense to treat the compilers as "stable" when bisecting - particularly because we don't have any guarantees of stability for nightly features.

I also think it might be worth thinking about why things like supports_feature exists and work to bring the language and compiler to a point that we can support to cases (and thus also have tight control on how this support changes for things like "pretending to be stable for bisecting").

(edit: seems like I was not the only one confused)

@estebank
Copy link
Contributor

Adding a flag to tell rustc to behave as stable as the PR does now will close #123404.


It should help with a (sufficient?) subset of the problematic cases that the PR initially wanted to address.

There will be cases where the bisection will be for crates using nightly features and a combination of those cases + incorrect rust version checking will continue to manifest the problem of bisecting making crates like ahash incorrectly enabling nightly features that will fail at quasi-random places in the bisection.


+1 on landing RUSTC_FORCE_RUSTC_VERSION (maybe as RUSTC_FORCE_VERSION?), after updating the PR title/description.

@oli-obk oli-obk changed the title Disable version_check::supports_feature on nightly allow overwriting the output of rustc --version Apr 25, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 25, 2024

I expanded the main PR description.

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 25, 2024

I also think it might be worth thinking about why things like supports_feature exists and work to bring the language and compiler to a point that we can support to cases (and thus also have tight control on how this support changes for things like "pretending to be stable for bisecting").

That has the opposite effect of what I'm proposing, as it endorses nightly detection. Nightly detection is absolutely incompatible with semver. It means that users testing their stable crate on nightly to be aware of upcoming changes get random failures from crates that work fine on stable.

@estebank
Copy link
Contributor

I appreciate the desire by crate authors to make things "just work™️", but it would be much better for the build.rs scripts making feature detection to provide a uses visible message asking them to enable feature flags explicitly. This will have a much smaller chance of causing things going awry.

Nemo157 added a commit to Nemo157/dotfiles that referenced this pull request May 2, 2024
Requires rust-lang/rust#124339 but drastically simplifies evading nightly-detection
@chorman0773
Copy link
Contributor

One thing I'd like is a way to say "Print whatever $RUST_VERSION the compiler is built for, but leave off nightly/dev/beta qualifiers". I've run into this with a number of projects, which will be a pain when I package various projects up in tarballs that shouldn't stop building (and shouldn't take on a defacto MxSRV) in the future because someone assumed that X nightly feature will exist forever.

@thomcc
Copy link
Member

thomcc commented May 2, 2024

it would be much better for the build.rs scripts making feature detection to provide a uses visible message asking them to enable feature flags explicitly

You can't1 actually do this, since cargo swallows the output from build scripts.

Footnotes

  1. Okay, you can open /dev/tty or the equivalent on windows2, but this has bad effects.

  2. $CONOUT or whatever, I don't remember. It's been long enough that I've forgotten the actual name.

@chorman0773
Copy link
Contributor

cargo doesn't swallow stderr IIRC.

@oli-obk

This comment was marked as outdated.

@bors

This comment was marked as resolved.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 29, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 29, 2024

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jul 29, 2024

📌 Commit 44f63d6 has been approved by wesleywiser

It is now in the queue for this repository.

@matthiaskrgr
Copy link
Member

@bors rollup=iffy

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 29, 2024
allow overwriting the output of `rustc --version`

Our wonderful bisection folk [have to work around](rust-lang#123276 (comment)) crates that do incomplete nightly/feature detection, as otherwise the bisection just points to where the feature detection breaks, and not to the actual breakage they are looking for.

This is also annoying behaviour to nightly users who did not opt-in to those nightly features. Most nightly users want to be in control of the nightly breakage they get, by

* choosing when to update rustc
* choosing when to update dependencies
* choosing which nightly features they are willing to take the breakage for

The reason this breakage occurs is that the build script of some crates run `rustc --version`, and if the version looks like nightly or dev, it will enable nightly features. These nightly features may break in random ways whenever we change something in nightly, so every release of such a crate will only work with a small range of nightly releases. This causes bisection to fail whenever it tries an unsupported nightly, even though that crate is not related to the bisection at all, but is just an unrelated dependency.

This PR (and the policy I want to establish with this FCP) is only for situations like the `version_check`'s `supports_feature` function. It is explicitly not for `autocfg` or similar feature-detection-by-building-rust-code, irrespective of my opinions on it and the similarity of nightly breakage that can occur with such schemes. These cause much less breakage, but should the breakage become an issue, they should get covered by this policy, too.

This PR allows changing the version and release strings reported by `rustc --version` via the `RUSTC_OVERRIDE_VERSION_STRING` env var. The bisection issue is then fixed by rust-lang/cargo-bisect-rustc#335.

I mainly want to establish a compiler team policy:

> We do not consider feature detection on nightly (on stable via compiler version numbering is fine) a valid use case that we need to support, and if it causes problems, we are at liberty to do what we deem best - either actively working to prevent it or to actively ignore it. We may try to work with responsive and cooperative authors, but are not obligated to.

Should they subvert the workarounds that nightly users or cargo-bisect-rustc can use, we should be able to land rustc PRs that target the specific crates that cause issues for us and outright replace their build script's logic to disable nightly detection.

I am not including links to crates, PRs or issues here, as I don't actually care about the specific use cases and don't want to make it trivial to go there and leave comments. This discussion is going to be interesting enough on its own, without branching out.
@bors
Copy link
Contributor

bors commented Jul 29, 2024

⌛ Testing commit 44f63d6 with merge ca601f8...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 29, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 29, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 30, 2024

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jul 30, 2024

📌 Commit 92f263b has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2024
@bors
Copy link
Contributor

bors commented Jul 30, 2024

⌛ Testing commit 92f263b with merge 006c8df...

@bors
Copy link
Contributor

bors commented Jul 30, 2024

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 006c8df to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 30, 2024
@bors bors merged commit 006c8df into rust-lang:master Jul 30, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Jul 30, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (006c8df): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.8% [-2.8%, -2.8%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.8% [-2.8%, -2.8%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 768.761s -> 769.469s (0.09%)
Artifact size: 331.87 MiB -> 331.90 MiB (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.