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

Refactor build-manifest to minimize the number of changes needed to add a new component #102565

Merged
merged 9 commits into from
Nov 9, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Oct 2, 2022

  • 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:

git checkout d44e14225ab00e164aa9ea9e8d9e1bee40f96b3e
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 #102241 and should not be merged before.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Oct 2, 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 Oct 2, 2022
@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-release Relevant to the release subteam, which will review and decide on the PR/issue. labels Oct 2, 2022
@jyn514 jyn514 changed the title Refactor build manifest to minimize the number of changes needed to add a new component Refactor build-manifest to minimize the number of changes needed to add a new component Oct 2, 2022
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 force-pushed the refactor-build-manifest branch from a8138e0 to 59c3e86 Compare October 2, 2022 12:51
@Mark-Simulacrum
Copy link
Member

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.

@bors
Copy link
Contributor

bors commented Oct 8, 2022

⌛ Trying commit 59c3e8621cf1a461ca0e84f849443cc6eb95b7f9 with merge aac0f44457bc655e570a51efc55e244d3c772d85...

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a 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.

src/tools/build-manifest/README.md Show resolved Hide resolved
src/tools/build-manifest/README.md Outdated Show resolved Hide resolved
src/tools/build-manifest/src/main.rs Outdated Show resolved Hide resolved
src/tools/build-manifest/src/main.rs Outdated Show resolved Hide resolved
@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 Oct 8, 2022
@bors
Copy link
Contributor

bors commented Oct 8, 2022

☀️ Try build successful - checks-actions
Build commit: aac0f44457bc655e570a51efc55e244d3c772d85 (aac0f44457bc655e570a51efc55e244d3c772d85)

@Mark-Simulacrum
Copy link
Member

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.
@jyn514 jyn514 force-pushed the refactor-build-manifest branch from 59c3e86 to f1ffb42 Compare October 27, 2022 17:16
@rust-log-analyzer

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.
@jyn514 jyn514 force-pushed the refactor-build-manifest branch from 1b12dd4 to fde05ea Compare October 27, 2022 17:54
@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 27, 2022
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2022
@rust-timer
Copy link
Collaborator

Queued 16d56136c42dabd7da968fcd4a16adc8f159bc6e with parent 68c836a, future comparison URL.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 30, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (16d56136c42dabd7da968fcd4a16adc8f159bc6e): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.7% [3.3%, 4.3%] 3
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.0% [-1.0%, -1.0%] 1

Cycles

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2022
@jyn514 jyn514 force-pushed the refactor-build-manifest branch from 361446e to 8810174 Compare October 30, 2022 21:44
@jyn514 jyn514 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 30, 2022
@Mark-Simulacrum
Copy link
Member

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

@rustbot rustbot 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 Nov 6, 2022
@jyn514
Copy link
Member Author

jyn514 commented Nov 9, 2022

Looks pretty good to me :)

; RUSTUP_DIST_SERVER=https://dev-static.rust-lang.org rustup toolchain install nightly-2022-10-29
info: syncing channel updates for 'nightly-2022-10-29-x86_64-unknown-linux-gnu'
info: latest update on 2022-10-29, rust version 1.66.0-nightly (16d56136c 2022-10-29)
info: downloading component 'cargo'
info: downloading component 'clippy'
info: downloading component 'rust-docs'
 19.0 MiB /  19.0 MiB (100 %)   9.2 MiB/s in  2s ETA:  0s
info: downloading component 'rust-std'
 29.6 MiB /  29.6 MiB (100 %)  11.2 MiB/s in  3s ETA:  0s
info: downloading component 'rustc'
 67.9 MiB /  67.9 MiB (100 %)  11.2 MiB/s in  7s ETA:  0s
info: downloading component 'rustfmt'
info: installing component 'cargo'
  3 IO-ops /   3 IO-ops (100 %)   2 IOPS in  1s ETA:  0s    
info: installing component 'clippy'
info: installing component 'rust-docs'
 19.0 MiB /  19.0 MiB (100 %) 875.2 KiB/s in  1m  7s ETA:  0s
info: installing component 'rust-std'
 29.6 MiB /  29.6 MiB (100 %)   6.4 MiB/s in  5s ETA:  0s
 13 IO-ops /  13 IO-ops (100 %)   0 IOPS in  6s ETA: Unknown
info: installing component 'rustc'
 67.9 MiB /  67.9 MiB (100 %)  15.6 MiB/s in  4s ETA:  0s
  6 IO-ops /   6 IO-ops (100 %)   0 IOPS in 26s ETA: Unknown
info: installing component 'rustfmt'
  1 IO-ops /   1 IO-ops (100 %)   0 IOPS in  1s ETA: Unknown

  nightly-2022-10-29-x86_64-unknown-linux-gnu installed - rustc 1.66.0-nightly (16d56136c 2022-10-29)

info: checking for self-updates
; cargo +nightly-2022-10-29-x86_64-unknown-linux-gnu --version
cargo 1.66.0-nightly (7e484fc1a 2022-10-27)
; rustc +nightly-2022-10-29-x86_64-unknown-linux-gnu --version
rustc 1.66.0-nightly (16d56136c 2022-10-29)

@jyn514
Copy link
Member Author

jyn514 commented Nov 9, 2022

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Nov 9, 2022

📌 Commit 8810174 has been approved by Mark-Simulacrum

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 9, 2022
@bors
Copy link
Contributor

bors commented Nov 9, 2022

⌛ Testing commit 8810174 with merge 0aaad9e...

@bors
Copy link
Contributor

bors commented Nov 9, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 0aaad9e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 9, 2022
@bors bors merged commit 0aaad9e into rust-lang:master Nov 9, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 9, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0aaad9e): 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.7% [2.3%, 3.1%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.9% [-3.9%, -3.9%] 1
All ❌✅ (primary) - - 0

Cycles

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

Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 12, 2022
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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 25, 2022
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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 25, 2022
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.
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…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.
@jyn514 jyn514 deleted the refactor-build-manifest branch February 25, 2023 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants