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

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

Merged
merged 7 commits into from
Aug 27, 2022

Conversation

est31
Copy link
Member

@est31 est31 commented Aug 15, 2022

Implements the idea from this 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 #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:

Alternative to #100577 which added checking.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels 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
Copy link
Member Author

est31 commented Aug 15, 2022

cc @pietroalbini @yaahc

@rust-log-analyzer

This comment has been minimized.

@est31 est31 force-pushed the stabilization_placeholder branch from edddcdd to a76b9d6 Compare August 15, 2022 20:24
@Mark-Simulacrum
Copy link
Member

Add the sed tooling to replace the placeholder to the nightly -> beta tooling (if present), and the version bumping tooling (if present). I would like to have help with this.

We don't currently have version bump tooling or nightly/beta tooling. There's src/tools/bump-stage0, but that's not quite right: it can be run mid-cycle if we (for example) fix a beta regression.

I think we can add a src/tools/bump-version which will for now just do this replacement -- I'm not sure if by "have help" you mean that you'd like someone else to do it or provide guidance on how? I also think we can leave this out of scope for now, and merge this PR as an immediate improvement (once the dev-guide is adjusted or has a PR up to do so).

The difficulty is that there's not an obvious time to run the tool (as a human), but I think we can do it twice and that will work fine:

  • When the newly promoted beta branch is created, we currently just adjust src/ci/channel to be beta; now we'll also run a tool that does this replacement. It will have all the stabilization PRs merged into the beta and so is easy to make correct.
  • For master, we have a more difficult story, since we can't easily run the tool as part of bors merge or similar. I think the best option there is to have the tool automatically move itself back in "time" by checking out (maybe virtually via git commands, rather than on the filesystem) the pre-version bump merge repository and making its edits based on contents then. The hardest part here is applying the changes to the local copy, since the line numbers will have changed. The easy way to do that would probably be to actually checkout that just-before-merge commit and run the tool then; that's probably my recommendation, and just change files on disk and use git's natural merging functionality to take care of re-associating them.
    • The future implementation here probably looks something like generating a patch or similar that's applied to create a commit, but that seems like more moving parts at little benefit.

Those times should be added to https://forge.rust-lang.org/release/process.html.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2022
@est31
Copy link
Member Author

est31 commented Aug 20, 2022

I'm not sure if by "have help" you mean that you'd like someone else to do it or provide guidance on how?

I'd mostly like guidance/a design discussion, and your comment already provided great help. I didnt know well how the mechanism works in the detail, and how much tooling there is. It seems that a lot of the process is still manual. I guess then the easiest way would be to add a new tool, update the docs with the times when that tool is supposed to be ran, and notify the people who are doing the bumping/branching currently so that they know about the change.

That tool would literally just be a glorified sed wrapper: it would read the version from src/version, remove any newlines from that string, and then walk recursively over the entire source tree, replacing all occurences (in rs and stdout files) of CURRENT_RUSTC_VERSION with that version number.

My idea/understanding was that glorified sed tool would run in three scenarios:

  • Right before src/version is updated to the next Rust version. Currently documented here. Order is important here: It needs to be before the upgrade because the replacement tool reads from the src/version file.

  • Whenever a PR that contains CURRENT_RUSTC_VERSION for whatever reason is backported to beta (say a backported stabilization, usually doesn't happen any more). Currently documented here. The person doing the backport has to run that tool locally and add the commit to their backport PR, otherwise tidy will fail (due to the checks added by this PR), so people will be notified if this is forgotten.

  • During creation of the beta branch. Currently documented here. There is master-to-beta.sh but I don't know whether it's still being used or not.

I don't think if it's integrated into workflows like that, it won't have to do anything diff based, which would be an advantage of this approach over #100577.

Do you think the tool should be written in Rust, python, or in shell?

@Mark-Simulacrum
Copy link
Member

The creation of the beta branch is done by rust-lang/promote-release, but as you can see by those instructions (which are kept up to date and followed during every release), it's still a manual step afterwards to bump the channel, so it's reasonable to run a tool too.

Yes, as part of these workflows shouldn't require diffing, just careful commit checkouts. I think your first point in particular misses that the src/version file is updated by humans and doesn't necessarily (or typically) land as the next PR, so our process should be intentional about "cleaning up" once it actually lands. That can probably be done as part of cfg(bootstrap) removal, which is the other task I'd like us to automate with this script (at least the easy cfg_attr part).

I think the tool should be written in Rust; that'll make extending it to do more interesting things easier - for example, the cfg bootstrap changing.

@Mark-Simulacrum
Copy link
Member

I'll also add that in general we're trying to automate where easy/worthwhile, I think the tradeoffs here are towards writing the code as a tool in this repository, but we should be thinking about moving it to promote-release eventually. That mostly means not for example expecting files to be on disk for sed -i but instead reading them in and then writing them back out with Rust APIs, I think.

@est31
Copy link
Member Author

est31 commented Aug 20, 2022

Yes, as part of these workflows shouldn't require diffing, just careful commit checkouts. I think your first point in particular misses that the src/version file is updated by humans and doesn't necessarily (or typically) land as the next PR, so our process should be intentional about "cleaning up" once it actually lands.

Oh right I missed that. master-to-beta.sh currently obtains a commit hash that is the last bors commit before the src/version bump landed. It does not obtain the bors commit that the src/version bump bases upon itself. Which causes a problem with running the sed tool in that PR, as that would only consider the things the bump PR bases on, while it should consider all things upon merge. Running the sed tool after the src/version bump also causes problems though, even if you reduce the number by one, as there is the danger of stabilizations slipping in that will only appear in the next release. I'm getting where you are coming from now, especially that it can be integrated into the PR that does the cfg(bootstrap) removal.

I think the "right before src/version is bumped" approach can be salvaged though: what about adjusting tidy to extend the CURRENT_RUSTC_VERSION ban to not just non-nightly, but also src/version bumping PRs? Then if that PR races a stabilization, CI for the src/version PR will fail, and the bumping person will have to rebase interactively and re-run the tool.

@Mark-Simulacrum
Copy link
Member

Just as an fyi, the master to beta script isn't used anymore (logic is moved to promote-release), but it's the same logic so talking about it is fine.

danger of stabilizations slipping in that will only appear in the next release

I don't think I follow this point. So long as we compute the parent of src/version bump by the bors history (--first-parent), there's a linear history of commits on master. The next release branches precisely when src/version merges, not when the PR is created, so I don't think we need to do anything particularly fancy here.

FWIW, we actually also want the same behavior for cfg(bootstrap) which also today sometimes runs into this "actually we stabilized/changed in the next release, so this cfg shouldn't be removed", which is solved in the same way - by looking at state just before src/version merges.

I think the approach should work without the additional tidy restrictions, which seems better. We typically want to avoid any of the release related PRs having conflicts or other reasons they're not going to cleanly land - those are all done on a schedule, and while following the schedule for master or (next) beta related things isn't too critical, slipping or having to rebase means there's an undue load on the people running the release (Pietro or I, today, though I hope to automate more and eventually drop the requirement for super privileges to run it). Ideally all of these PRs are post, r+, done.

@est31
Copy link
Member Author

est31 commented Aug 21, 2022

danger of stabilizations slipping in that will only appear in the next release

[...] there's a linear history of commits on master. The next release branches precisely when src/version merges, not when the PR is created, so I don't think we need to do anything particularly fancy here.

By that I meant that if a stabilization is merged after the src/version bump, and before the cfg(bootstrap) update, then if someone runs the sed script, it will set the number for the prior release, but the stabilization obviously won't make it into the prior release any more but will only appear in the next release.

FWIW, we actually also want the same behavior for cfg(bootstrap) which also today sometimes runs into this "actually we stabilized/changed in the next release, so this cfg shouldn't be removed", which is solved in the same way - by looking at state just before src/version merges.

The issue is the different error conditions: if a cfg(bootstrap) is gone wrong, it will result in a build error. If a stabilization got misattributed for a different release, it won't cause any errors. This is why I suggested that tidy error. I understand that it makes the release process harder, but this entire change is about moving the hard part of attributing stabilization versions to the release process. Personally, I feel it's harder to get the version numbers right if the replacement is done in the cfg(bootstrap) PR than it is to re-run an automatted tool upon a rebase. Plus, if there is really a danger, the PR src/version bump can be merged with a high priority (say 15), so it won't do any races. It's also easier to automate this than some check that runs during cfg(bootstrap) removal.

@Mark-Simulacrum
Copy link
Member

I think my suggestion for avoiding that is to run the sed script not on latest master but rather checking out src/version bors merge parent, which should be precisely the list of things that gets stabilized. Now that I think about it, cherry picking the commit made on the beta branch would work too.

Either of these adds ~zero complexity (definitely zero to the script) and decouples the change from beta-destined parts of the process.

@est31
Copy link
Member Author

est31 commented Aug 21, 2022

Hmm yeah I like the cherry-pick idea (I was afraid that it's too complicated to first base the cfg(version) PR on the state that the beta branched from, run the tool, and then rebase it, and not run the tool). Cherry-picking is easier to describe and follow I think. Also I like the fact how both beta and master have a single "source of truth" so to say. So the updated list of what should be done is:

  • The sed tool shall be ran during creation of the beta branch, in the PR that changes src/ci/channel, but in a separate commit so that it can be cherry picked later. Currently the beta branch creation is documented here. The automated part is done in promote-release.

  • In the master bootstrap update PR, currently documented here, the commit that ran the sed tool during the creation of the beta branch shall be cherry picked and included.

  • Whenever a PR that contains CURRENT_RUSTC_VERSION for whatever reason is backported to beta (say a backported stabilization, usually doesn't happen any more), the sed tool shall be run too. Currently documented here. The person doing the backport has to run that tool locally and add the commit to their backport PR, otherwise tidy will fail (due to the checks added by this PR), so people will be notified if this is forgotten.

I think with this I know what to do now for the tool. I'll be implementing it now.

@est31 est31 changed the title Require stabilizations to use a placeholder instead of writing out the full dependency version Require stabilizations to use a placeholder instead of writing out the explicit stabilization version Aug 23, 2022
@est31 est31 changed the title Require stabilizations to use a placeholder instead of writing out the explicit stabilization version Require stabilizations to use a placeholder instead of writing out stabilization version Aug 23, 2022
@est31 est31 force-pushed the stabilization_placeholder branch from a76b9d6 to ebf1d77 Compare August 23, 2022 21:09
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Aug 23, 2022
@est31 est31 force-pushed the stabilization_placeholder branch from ebf1d77 to 6c5fa74 Compare August 23, 2022 21:54
@est31 est31 marked this pull request as ready for review August 23, 2022 21:54
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (eaadb89): 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

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.

mean1 range count2
Regressions ❌
(primary)
3.2% [3.2%, 3.2%] 1
Regressions ❌
(secondary)
7.6% [6.1%, 9.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.2% [3.2%, 3.2%] 1

Cycles

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

Footnotes

  1. the arithmetic mean of the percent change

  2. number of relevant changes

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 1, 2022
… r=Mark-Simulacrum

Also replace the version placeholder in rustc_attr

Replace the version placeholder with the current version in the rustc_attr crate too so that users won't see the placeholder but instead the explicit version. This especially fixes the bug for rustdoc not showing it but instead the placeholder.

Originally reported [here](https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/libs.20stabilization.20placeholder/near/296057188).

cc rust-lang#100591

![Screenshot_20220830_233727](https://user-images.githubusercontent.com/8872119/187548079-6207776b-4481-4351-afff-607f5b3fe03a.png)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 1, 2022
… r=Mark-Simulacrum

Also replace the version placeholder in rustc_attr

Replace the version placeholder with the current version in the rustc_attr crate too so that users won't see the placeholder but instead the explicit version. This especially fixes the bug for rustdoc not showing it but instead the placeholder.

Originally reported [here](https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/libs.20stabilization.20placeholder/near/296057188).

cc rust-lang#100591

![Screenshot_20220830_233727](https://user-images.githubusercontent.com/8872119/187548079-6207776b-4481-4351-afff-607f5b3fe03a.png)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 2, 2022
… r=Mark-Simulacrum

Also replace the version placeholder in rustc_attr

Replace the version placeholder with the current version in the rustc_attr crate too so that users won't see the placeholder but instead the explicit version. This especially fixes the bug for rustdoc not showing it but instead the placeholder.

Originally reported [here](https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/libs.20stabilization.20placeholder/near/296057188).

cc rust-lang#100591

![Screenshot_20220830_233727](https://user-images.githubusercontent.com/8872119/187548079-6207776b-4481-4351-afff-607f5b3fe03a.png)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 2, 2022
… r=Mark-Simulacrum

Also replace the version placeholder in rustc_attr

Replace the version placeholder with the current version in the rustc_attr crate too so that users won't see the placeholder but instead the explicit version. This especially fixes the bug for rustdoc not showing it but instead the placeholder.

Originally reported [here](https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/libs.20stabilization.20placeholder/near/296057188).

cc rust-lang#100591

![Screenshot_20220830_233727](https://user-images.githubusercontent.com/8872119/187548079-6207776b-4481-4351-afff-607f5b3fe03a.png)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 2, 2022
… r=Mark-Simulacrum

Also replace the version placeholder in rustc_attr

Replace the version placeholder with the current version in the rustc_attr crate too so that users won't see the placeholder but instead the explicit version. This especially fixes the bug for rustdoc not showing it but instead the placeholder.

Originally reported [here](https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/libs.20stabilization.20placeholder/near/296057188).

cc rust-lang#100591

![Screenshot_20220830_233727](https://user-images.githubusercontent.com/8872119/187548079-6207776b-4481-4351-afff-607f5b3fe03a.png)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 2, 2022
… r=Mark-Simulacrum

Also replace the version placeholder in rustc_attr

Replace the version placeholder with the current version in the rustc_attr crate too so that users won't see the placeholder but instead the explicit version. This especially fixes the bug for rustdoc not showing it but instead the placeholder.

Originally reported [here](https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/libs.20stabilization.20placeholder/near/296057188).

cc rust-lang#100591

![Screenshot_20220830_233727](https://user-images.githubusercontent.com/8872119/187548079-6207776b-4481-4351-afff-607f5b3fe03a.png)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 7, 2022
…acrum

Fix error printing mistake in tidy

Fixes a small bug in the error printing code added by rust-lang#100591 .
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 14, 2022
…ics, r=jackh726

Also replace the placeholder for the stable_features lint

Follow up of  rust-lang#101215 and rust-lang#100591 .

Fixes rust-lang#101766
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 19, 2022
…=Mark-Simulacrum

Add all submodules to the list of directories tidy skips

Tidy contains a blacklist of directories that it is not visiting. This list is also used by the `replace-version-placeholder` tool added by rust-lang#100591 , to determine the directories to do its replacement from. Generally, tidy does not check submodules, but this is not done consistently for all submodules. This PR adds the submodules that were previously missing, so that the `replace-version-placeholder` tool does not attempt to change content of the books. This was needed because `rustc-dev-guide` contains the placeholder, leading to rust-lang#102014.

Fixes rust-lang#102014
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Sep 22, 2022
…k-Simulacrum

Don't crate-locally reexport walk functions in tidy

I've moved the walk functions into their own module in rust-lang#100591 and didn't want to make changing the paths everywhere in tidy part of the PRs diff, so I just reexported the functions locally. This PR removes the crate-local reexport and instead does module level reexports. I'm not sure how much it's worth it and whether the new state is better, idk. Feel free to have any opinion on this.
bors bot added a commit to intellij-rust/intellij-rust that referenced this pull request Oct 9, 2022
9480: Convert compiler features to json and update compiler features r=mchernyavsky a=Undin

After rust-lang/rust#100591, compiler uses `CURRENT_RUSTC_VERSION` placeholder as value of `since` field which means that feature is available in the current compiler.
It's ok for the compiler, but the plugin should work with different compiler versions, so it needs some concrete version when the feature became available to annotate code properly.

The main idea to solve this problem is to use already saved values:
- if feature has a placeholder and we didn't meet it before, the current compiler version will be used to replace the placeholder. Here we assume that this code is launched with the same (or almost the same) rustc version as in rustc master
- otherwise, just use version saved previously

Previously, we saved info about compiler features directly as generated kotlin code and it made loading this info from `attributes-info` quite complicated. So, now compiler features are saved in language independent form - json file located in resources.
This approach not only allows us to deserialize data in any language. It also opens up the possibility to upload generated json somewhere and load this file from plugin to update features info

Closes #9258

changelog: Keep info about compiler features in json instead of generated Kotlin code


Co-authored-by: Arseniy Pendryak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants