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

fix --dry-run when the change-id warning is printed #118789

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 9, 2023

previously:

Building bootstrap
   Compiling bootstrap v0.0.0 (/home/jyn/src/rust2/src/bootstrap)
    Finished dev [unoptimized] target(s) in 4.23s
thread 'main' panicked at src/bin/main.rs:147:17:
fs::write(warned_id_path, latest_change_id.to_string()) failed with No such file or directory (os error 2)

@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 9, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2023

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs and change-id in config.example.toml.

@onur-ozkan
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 13, 2023

📌 Commit 520662e has been approved by onur-ozkan

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 Dec 13, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2023
fix --dry-run when the change-id warning is printed

previously:
```
Building bootstrap
   Compiling bootstrap v0.0.0 (/home/jyn/src/rust2/src/bootstrap)
    Finished dev [unoptimized] target(s) in 4.23s
thread 'main' panicked at src/bin/main.rs:147:17:
fs::write(warned_id_path, latest_change_id.to_string()) failed with No such file or directory (os error 2)
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2023
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#118375 (Add -Zunpretty=stable-mir output test)
 - rust-lang#118538 (fix dynamic size/align computation logic for packed types with dyn trait tail)
 - rust-lang#118789 (fix --dry-run when the change-id warning is printed)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Dec 14, 2023

⌛ Testing commit 520662e with merge 529047c...

@bors
Copy link
Contributor

bors commented Dec 14, 2023

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing 529047c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 14, 2023
@bors bors merged commit 529047c into rust-lang:master Dec 14, 2023
@rustbot rustbot added this to the 1.76.0 milestone Dec 14, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (529047c): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.9% [0.8%, 1.0%] 4
Regressions ❌
(secondary)
0.5% [0.3%, 0.8%] 18
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.9% [0.8%, 1.0%] 4

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.6% [0.5%, 0.6%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
-3.8% [-3.8%, -3.8%] 1
All ❌✅ (primary) 0.3% [-0.5%, 0.6%] 5

Cycles

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.8% [0.8%, 0.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.3% [-1.3%, -1.3%] 1
Improvements ✅
(secondary)
-2.1% [-2.1%, -2.1%] 1
All ❌✅ (primary) -0.2% [-1.3%, 0.8%] 2

Binary size

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

Bootstrap: 671.102s -> 672.087s (0.15%)
Artifact size: 312.48 MiB -> 312.43 MiB (-0.02%)

@rustbot rustbot added the perf-regression Performance regression. label Dec 14, 2023
@lqd
Copy link
Member

lqd commented Dec 14, 2023

I think the noise threshold on these benchmarks was somehow lower recently, and is now picking back up to its regular level, hence regressions within a range that is usually ignored. Obviously this bootstrap PR has no impact on compiler performance whatsoever.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 14, 2023
@jyn514 jyn514 deleted the dry-run branch December 14, 2023 18:36
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Mar 6, 2025
Reduce the noise of bootstrap changelog warnings in --dry-run mode

Presently x.py displays "There have been changes to x.py since you last updated:" note only once when run normally, but on every invocation when run with `--dry-run`.
The disparity is not exactly intentonal, but just a historical accident.
It was made to be printed once in rust-lang#117815 via storing `.last-warned-change-id` on disk in `{config.out}/bootstrap` (i.e. `build/bootstrap`) directory.
But that didn't quite work for `--dry-run`, since `{config.out}/bootsrap` points to `build/tmp-dry-run/bootstrap` which *isn't* created in dry-run mode, so file creation fails.
This got fixed in rust-lang#118789 and now `--dry-run` does not save `.last-warned-change-id` at all. (Nor does it read it, since it cannot know to read from non-dry-run location)

This PR simply stops displaying the changelog altogether in --dry-run mode.

<details>
<summary>previous attempt (outdated)</summary>
This PR takes a different approach, and instead of not-writing the stamp in `--dry-run` mode it instead tries harder to yes-write it, and, specifically, creates `build/tmp-dry-run/bootstrap` directory to do so. If neccessary (i.e. if there are changes newer than the `change-id` stamp of config.toml to warn about).
Note that `build/tmp-dry-run/` was *already* being created, so making an extra `boostrap` sub-folder should not meaningfully pollute the build dir.
</details>

(Apologies for the, perhaps, excessively wordy PR, I'm new to this)
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. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants