-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Bump to 1.84 #131560
Bump to 1.84 #131560
Conversation
@bors r+ rollup=never |
@bors p=1 |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
Hm, strange... seems like something is wrong with |
It looks like |
Maybe that's because of 1090d89#diff-5f5330cfcdb0a89b85ac3547b761c3a45c2534a85c4aaae8fea88c711a7a65b2R2738? Seems only |
This PR modifies If appropriate, please update |
@@ -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")]) |
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.
This is the spot I found, but there may be multiple places that need to notice it.
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.
Should it be src/version
?
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.
Bah, not that simple -- it's failing again:
https://github.com/rust-lang-ci/rust/actions/runs/11312606957/job/31460221821#step:26:2845
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.
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.
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.
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.
Yeah, and FYI in #131560 (comment) there is a more recent change (which I think is more suspicious).
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.
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.
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.
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.
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.
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.
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.
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.)
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.
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.
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 try |
Bump to 1.84 https://forge.rust-lang.org/release/process.html#bump-the-stable-version-number-t-6-days-friday-the-week-before r? ghost try-job: aarch64-apple
☀️ Try build successful - checks-actions |
Let's get this release train moving... @bors r+ p=10 |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (27861c4): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis 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.
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.
CyclesResults (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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 781.427s -> 782.891s (0.19%) |
https://forge.rust-lang.org/release/process.html#bump-the-stable-version-number-t-6-days-friday-the-week-before
r? ghost