Bump to 1.84 by cuviper · Pull Request #131560 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Bump to 1.84 #131560
Conversation44 Commits2 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Status: Awaiting review from the assignee but also interested parties.
Relevant to the release subteam, which will review and decide on the PR/issue.
labels
📌 Commit 734481c has been approved by cuviper
It is now in the queue for this repository.
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
This comment has been minimized.
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
This comment has been minimized.
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
Hm, strange... seems like something is wrong with ver_cfg_rel?
It looks like rust.download-rustc=if-unchanged activated, not accounting for the version change. So ver_cfg_rel("-1") tried 1.83 per the new src/version = 1.84, but the downloaded compiler was 1.83 -- assumed incomplete.
This PR modifies src/bootstrap/src/core/config.
If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
label
| @@ -2780,7 +2780,7 @@ impl Config { |
|---|
| let has_changes = !t!(helpers::git(Some(&self.src)) |
| .args(["diff-index", "--quiet", &commit]) |
| .arg("--") |
| .args([self.src.join("compiler"), self.src.join("library")]) |
| .args([self.src.join("compiler"), self.src.join("library"), self.src.join("version")]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the spot I found, but there may be multiple places that need to notice it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be src/version?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be
src/version?
Ah, in my haste I thought self.src already included that. But it's late for me, so I'll check back tomorrow.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be
src/version?Ah, in my haste I thought
self.srcalready included that. But it's late for me, so I'll check back tomorrow.
Yeah, and FYI in #131560 (comment) there is a more recent change (which I think is more suspicious).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal preference for now would be to disable this logic entirely (i.e., never use the downloaded rustc on CI), to remove any risk for the upcoming release.
It's not used in dist runners.
The logic was fatally wrong and had to be fixed 2 times already within less than a week (by missing files or folders that have to be checked for changes). If that's not very clear evidence that this is very fragile, I don't know what is.
It was never tested on CI before and I think this is quite normal. But sure, I understand your concern.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a test that checks if "if-unchanged" logic acts as expected when there is change in "compiler" tree,
What you'd need is a test which checks that if there is any change outside the "compiler" tree, nothing about the compiler changes. That would ensure that the logic is correct, and would have caught this problem. But of course that's basically impossible -- and without such a check, any mistake in the "list of folders to check" will cause subtle breakage.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not used in dist runners.
I don't see how that's sufficient. It's not like the other runners are irrelevant.
It was never tested on CI before and I think this is quite normal.
I think it is a symptom of fragile design. More robust alternatives are possible and I'd like to known whether they have been considered. See #131658.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, 3 days before a beta branch is a very bad time to land something where you think it is "normal" that it completely breaks CI for other PRs. (But this particular issue with the version file, would not have been caught by landing this earlier in a cycle. It would have been avoided by a less fragile design, though.)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my fix attempt, maybe you would like to try that here (or should I file a PR?).
It's tricky since this PR will cut off what goes into 1.83-beta -- which won't see any more version changes, but will see channel and stage0 changes. But I can cherry-pick it when I do that branch promotion, so I think for now I'll just add it here to get the release train on track. Thanks!
This comment has been minimized.
These files have important role for compiler builds, so include them in the "if-unchanged" rustc logic.
Signed-off-by: onur-ozkan work@onurozkan.dev
bors added a commit to rust-lang-ci/rust that referenced this pull request
☀️ Try build successful - checks-actions
Build commit: 351e7d4 (351e7d4c03aeaf7e5e888e4f158cb8e6aef2c435)
Let's get this release train moving...
@bors r+ p=10
📌 Commit 6e6cbdd has been approved by cuviper
It is now in the queue for this repository.
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] miri test:false 5.616
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`
Caused by:
Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
local time: Sun, Oct 13, 2024 6:13:58 PM
network time: Sun, 13 Oct 2024 18:13:59 GMT
##[error]Process completed with exit code 1.
Post job cleanup.
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
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
Finished benchmarking commit (27861c4): comparison URL.
Overall result: ❌ regressions - no action needed
@rustbot label: -perf-regression
Instruction count
This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 1.9% | [1.9%, 1.9%] | 1 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Max RSS (memory usage)
Results (primary -3.2%)
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) | - | - | 0 |
| Improvements ✅ (primary) | -3.2% | [-3.2%, -3.2%] | 1 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | -3.2% | [-3.2%, -3.2%] | 1 |
Cycles
Results (secondary 2.8%)
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% | [2.1%, 5.3%] | 6 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | -2.9% | [-2.9%, -2.9%] | 1 |
| All ❌✅ (primary) | - | - | 0 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 781.427s -> 782.891s (0.19%)
Artifact size: 331.96 MiB -> 332.14 MiB (0.05%)
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Relevant to the release subteam, which will review and decide on the PR/issue.