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

Bump to 1.84 #131560

Merged
merged 2 commits into from
Oct 13, 2024
Merged

Bump to 1.84 #131560

merged 2 commits into from
Oct 13, 2024

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Oct 11, 2024

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-release Relevant to the release subteam, which will review and decide on the PR/issue. labels Oct 11, 2024
@cuviper
Copy link
Member Author

cuviper commented Oct 11, 2024

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Oct 11, 2024

📌 Commit 734481c has been approved by cuviper

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 Oct 11, 2024
@cuviper
Copy link
Member Author

cuviper commented Oct 11, 2024

@bors p=1

@bors
Copy link
Contributor

bors commented Oct 12, 2024

⌛ Testing commit 734481c with merge 5dfdba2...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 12, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review 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 Oct 12, 2024
@cuviper
Copy link
Member Author

cuviper commented Oct 12, 2024

@bors retry

(hoping that #131448 lands with its fix)

@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 Oct 12, 2024
@cuviper cuviper mentioned this pull request Oct 12, 2024
@bors
Copy link
Contributor

bors commented Oct 13, 2024

⌛ Testing commit 734481c with merge 36ffc08...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 13, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review 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 Oct 13, 2024
@RalfJung
Copy link
Member

Hm, strange... seems like something is wrong with ver_cfg_rel?

@cuviper
Copy link
Member Author

cuviper commented Oct 13, 2024

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.

@slanterns
Copy link
Contributor

slanterns commented Oct 13, 2024

Maybe that's because of 1090d89#diff-5f5330cfcdb0a89b85ac3547b761c3a45c2534a85c4aaae8fea88c711a7a65b2R2738? Seems only compiler and library folders are counted.

@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2024

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

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Oct 13, 2024
@@ -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")])
Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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.

Copy link
Contributor

@slanterns slanterns Oct 13, 2024

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).

Copy link
Member

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.

Copy link
Member

@RalfJung RalfJung Oct 13, 2024

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.

Copy link
Member

@RalfJung RalfJung Oct 13, 2024

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.

Copy link
Member

@RalfJung RalfJung Oct 13, 2024

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.)

Copy link
Member Author

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!

@rust-log-analyzer

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>
@cuviper
Copy link
Member Author

cuviper commented Oct 13, 2024

@bors try

@bors
Copy link
Contributor

bors commented Oct 13, 2024

⌛ Trying commit 6e6cbdd with merge 351e7d4...

@bors
Copy link
Contributor

bors commented Oct 13, 2024

☀️ Try build successful - checks-actions
Build commit: 351e7d4 (351e7d4c03aeaf7e5e888e4f158cb8e6aef2c435)

@cuviper
Copy link
Member Author

cuviper commented Oct 13, 2024

Let's get this release train moving...

@bors r+ p=10

@bors
Copy link
Contributor

bors commented Oct 13, 2024

📌 Commit 6e6cbdd has been approved by cuviper

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 Oct 13, 2024
@bors
Copy link
Contributor

bors commented Oct 13, 2024

⌛ Testing commit 6e6cbdd with merge 9cbaf4e...

@rust-log-analyzer
Copy link
Collaborator

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.

@bors
Copy link
Contributor

bors commented Oct 13, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review 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 Oct 13, 2024
@cuviper
Copy link
Member Author

cuviper commented Oct 13, 2024

@bors retry

@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 Oct 13, 2024
@bors
Copy link
Contributor

bors commented Oct 13, 2024

⌛ Testing commit 6e6cbdd with merge 27861c4...

@bors
Copy link
Contributor

bors commented Oct 13, 2024

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 27861c4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2024
@bors bors merged commit 27861c4 into rust-lang:master Oct 13, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 13, 2024
@cuviper cuviper deleted the start-1.84 branch October 13, 2024 23:23
@rust-timer
Copy link
Collaborator

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%)

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. 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.

8 participants