-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Refactor build-manifest to minimize the number of changes needed to add a new component #102565
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
a8138e0
to
59c3e86
Compare
Do we have a sense of the likely impact of the llvm-tools rename? rustup component add llvm-tools already doesn't work, so I assume this is harmless? In any case let's get a @bors try build so we can run this past dev-static nightly and verify it all works in production. |
⌛ Trying commit 59c3e8621cf1a461ca0e84f849443cc6eb95b7f9 with merge aac0f44457bc655e570a51efc55e244d3c772d85... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broadly speaking this looks like an excellent improvement, thanks! Left some relatively minor comments, hopefully.
☀️ Try build successful - checks-actions |
Kicked off a dev-static nightly. |
…w`, or have a mismatch between parsing and stringifying
Previously, these had to be hard-coded (i.e. specified in both `PkgType` and `fn package`). Now they only have to be specified in `PkgType`.
… manifest This caught a missing preview rename for `llvm-tools`.
This avoids bugs where components are added to one part of the manifest but not another.
59c3e86
to
f1ffb42
Compare
This comment has been minimized.
This comment has been minimized.
In particular, this avoids serializing and parsing the pkg to a string, which allows getting rid of `PkgType::Other` altogether
This makes it easier to remove `is_preview` from components in the future if we choose to do so.
1b12dd4
to
fde05ea
Compare
Queued 16d56136c42dabd7da968fcd4a16adc8f159bc6e with parent 68c836a, future comparison URL. |
Finished benchmarking commit (16d56136c42dabd7da968fcd4a16adc8f159bc6e): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. |
361446e
to
8810174
Compare
Per #102565 (comment) I think this is waiting on some additional testing which should hopefully be possible on your end? Let me know if not. @rustbot author |
Looks pretty good to me :)
|
@bors r=Mark-Simulacrum |
☀️ Test successful - checks-actions |
Finished benchmarking commit (0aaad9e): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. |
Don't set `is_preview` for clippy and rustfmt These have been shipped on stable for many years now and it would be very disruptive to ever remove them. Remove the `-preview` suffix from their dist components. Based on rust-lang#102565.
Don't set `is_preview` for clippy and rustfmt These have been shipped on stable for many years now and it would be very disruptive to ever remove them. Remove the `-preview` suffix from their dist components. Based on rust-lang#102565.
Don't set `is_preview` for clippy and rustfmt These have been shipped on stable for many years now and it would be very disruptive to ever remove them. Remove the `-preview` suffix from their dist components. Based on rust-lang#102565.
…rk-Simulacrum Refactor build-manifest to minimize the number of changes needed to add a new component - Add all components to `PkgType` - Automate functionality wherever possible, so functions often don't have to be manually edited - Where that's not possible, use exhaustive matches on `PkgType` instead of adding individual strings. - Add documentation for how to add a component. Improve the existing documentation for how to test changes. I tested locally that this generates an identical manifest before and after my change, as follows: ```sh git checkout d44e142 cargo +nightly run --manifest-path src/tools/build-manifest/Cargo.toml build/dist build/manifest-before 1970-01-01 http://example.com nightly git checkout refactor-build-manifest cargo +nightly run --manifest-path src/tools/build-manifest/Cargo.toml build/dist build/manifest-before 1970-01-01 http://example.com nightly sort -u build/manifest-before/channel-rust-nightly.toml | diff - <(sort -u build/manifest-after/channel-rust-nightly.toml) ``` I then verified by hand that the differences before sorting are inconsequential (mostly targets being slightly reordered). The only change in behavior is that `llvm-tools` is now properly renamed to `llvm-tools-preview`: ``` ; sort -u build/manifest-before/channel-rust-nightly.toml | diff - <(sort -u build/manifest-after/channel-rust-nightly.toml) 784a785 > [renames.llvm-tools] 894a896 > to = "llvm-tools-preview" ``` This is based on rust-lang#102241 and should not be merged before.
PkgType
PkgType
instead of adding individual strings.I tested locally that this generates an identical manifest before and after my change, as follows:
I then verified by hand that the differences before sorting are inconsequential (mostly targets being slightly reordered).
The only change in behavior is that
llvm-tools
is now properly renamed tollvm-tools-preview
:This is based on #102241 and should not be merged before.