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

Ensure that feature stabilizations are for the current version #100577

Closed
wants to merge 3 commits into from

Conversation

est31
Copy link
Member

@est31 est31 commented Aug 15, 2022

It's a common phenomenon that feature stabilizations don't make it into a particular release, but the version is still inaccurate. Often this is caught in the PR, but it can also require subsequent changes to adjust/correct the version, e.g.:

This PR adds checking to ensure that a stabilization is always for the current release: it makes tidy determine a git "base commit" for the given change, and then compares the list to that base commit. That way, tidy knows the features that have stabilized, and can compare them to the current version.

Currently, this is implemented for lang features only, but with some engineering it can be extended to library features as well.

Note: currently the PR includes a mock stabilization meant to showcase how the PR works. I will remove it once the bot issues the expected complaint.

est31 added 3 commits August 13, 2022 14:36
It's a common phenomenon that feature stabilizations don't make it into
a particular release, but the version is still inaccurate. Often this
leads to subsequent changes to adjust/correct the version, but it's easy
to miss errors.

This commit adds checking to ensure that a stabilization is always
for the current release. Adjusting versions of already stable features
is still supported, as is unstabilizing or other modifications.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 15, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2022
@est31 est31 changed the title Check that feature stabilizations contain the current version Ensure that feature stabilizations are for the current version Aug 15, 2022
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    Checking rustc_query_impl v0.0.0 (/checkout/compiler/rustc_query_impl)
    Checking rustc_passes v0.0.0 (/checkout/compiler/rustc_passes)
    Checking rustc_ast_lowering v0.0.0 (/checkout/compiler/rustc_ast_lowering)
    Checking rustc_save_analysis v0.0.0 (/checkout/compiler/rustc_save_analysis)
error[E0609]: no field `fn_align` on type `&Features`
     |
     |
1570 |                     if let (Target::Fn, false) = (target, self.tcx.features().fn_align) {
     |
     |
     = note: available fields are: `declared_lang_features`, `declared_lib_features`, `active_features`, `abi_thiscall`, `abi_unadjusted` ... and 176 others
For more information about this error, try `rustc --explain E0609`.
error: could not compile `rustc_passes` due to previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `rustc_passes` due to previous error

@Mark-Simulacrum
Copy link
Member

FWIW, there's been some recent discussion about improving this to be automatic (https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/libs.20stabilization.20placeholder).

I've not looked at the code here, but I think the effect of the proposal in Zulip would be that we just need to make sure folks are using the magic for any new stabilizations; I'm not sure we need a tidy check for that though (or how feasible it is, if we're intending to backport stabilizations for some reason, though we shouldn't do that).

@est31
Copy link
Member Author

est31 commented Aug 15, 2022

interesting, I guess that approach is even better. Closing, but I might open another PR with the implementation of that suggestion.

@est31 est31 closed this Aug 15, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2022
…ark-Simulacrum

Require stabilizations to use a placeholder instead of writing out stabilization version

Implements the idea from [this](https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/libs.20stabilization.20placeholder) zulip stream.

It's a common phenomenon that feature stabilizations don't make it into a particular release, but the version is still inaccurate. Often this is caught in the PR, but it can also require subsequent changes to adjust/correct the version. A list with examples of such PRs is given in rust-lang#100577, but it's far from complete.

This PR requires stabilization PRs to use the placeholder `CURRENT_RUSTC_VERSION`, enforced via tidy tooling. The PR also adds a tool that replaces the placeholder with the version number. It can be invoked via `./x.py run src/tools/replace-version-placeholder` and is supposed to be ran upon beta branching as well as version bumping and any backports to the beta branch.  I filed PRs to the dev guide and forge to document these changes in the release and stabilization workflows:

* The [dev guide](https://rustc-dev-guide.rust-lang.org/stabilization_guide.html#determining-the-stabilization-version) PR: rust-lang/rustc-dev-guide#1443
* The [std dev guide](https://std-dev-guide.rust-lang.org/) PR: rust-lang/std-dev-guide#43
* The [forge](https://github.com/rust-lang/rust-forge) PR: rust-lang/rust-forge#643

Alternative to rust-lang#100577 which added checking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants