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

Suggest Upgrading Compiler for Gated Features #119088

Merged
merged 4 commits into from
Jan 13, 2024

Conversation

George-lewis
Copy link
Contributor

@George-lewis George-lewis commented Dec 18, 2023

This PR addresses #117318

I have a few questions:

  1. Do we want to specify the current version and release date of the compiler? I have added this in via environment variables, which I found in the code for the rustc cli where it handles the --version flag
    a. How can I handle the changing message in the tests?
  2. Do we want to only show this message when the compiler is old?
    a. How can we determine when the compiler is old?

I'll wait until we figure out the message to bless the tests

@rustbot
Copy link
Collaborator

rustbot commented Dec 18, 2023

r? @compiler-errors

(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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 18, 2023
@bjorn3
Copy link
Member

bjorn3 commented Dec 18, 2023

Do we want to only show this message when the compiler is old?

It would introduce a source of non-determinism which as I understand it would make rustc tests fail if CI stalls just a bit too long or if someone tries to recompile an older nightly version and then run it's test suite for reproducibility reasons.

@George-lewis
Copy link
Contributor Author

@bjorn3 That's a very good point about non-determinism. I think from a ui perspective it would be nice to show the message selectively, so perhaps if there's a way to make this deterministic for the tests, we can move forwards with it? Otherwise I'm ok with making this message static.

I'll wait for @compiler-errors to follow up

@Rajveer100
Copy link
Contributor

Rajveer100 commented Dec 18, 2023

@George-lewis
I have closed my PR since you have found the right place to debug, feel free to progress, meanwhile I will grind other ones.

@George-lewis
Copy link
Contributor Author

Thank you, @Rajveer100, I appreciate that, and good luck to you

@compiler-errors
Copy link
Member

r? nilstrieb

kinda too busy to continue to review this

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 27, 2023

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

@George-lewis George-lewis force-pushed the glewis/suggest-upgrading-compiler branch from 0c5cc08 to 08676d0 Compare December 27, 2023 16:35
@rust-log-analyzer

This comment has been minimized.

@George-lewis
Copy link
Contributor Author

@rustbot label -S-waiting-on-author

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 27, 2023
@George-lewis
Copy link
Contributor Author

@rustbot label -has-merge-commits

@rustbot
Copy link
Collaborator

rustbot commented Dec 27, 2023

Error: Label has-merge-commits can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@Noratrieb Noratrieb removed the has-merge-commits PR has merge commits, merge with caution. label Dec 27, 2023
@bors
Copy link
Contributor

bors commented Dec 30, 2023

☔ The latest upstream changes (presumably #119421) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 13, 2024
@George-lewis George-lewis force-pushed the glewis/suggest-upgrading-compiler branch from 0957eaa to d56cdd4 Compare January 13, 2024 17:47
@George-lewis
Copy link
Contributor Author

@Nilstrieb Rebased and re-blessed the tests

@Noratrieb
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Jan 13, 2024

📌 Commit d56cdd4 has been approved by Nilstrieb

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

bors commented Jan 13, 2024

⌛ Testing commit d56cdd4 with merge d78329b...

@bors
Copy link
Contributor

bors commented Jan 13, 2024

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing d78329b to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d78329b): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -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.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.7% [-0.7%, -0.7%] 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)
3.4% [3.4%, 3.4%] 1
Regressions ❌
(secondary)
4.1% [3.0%, 5.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.3% [-3.3%, -3.3%] 1
All ❌✅ (primary) 3.4% [3.4%, 3.4%] 1

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
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.3% [-2.3%, -2.3%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.3% [-2.3%, -2.3%] 1

Binary size

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

Bootstrap: 667.979s -> 668.17s (0.03%)
Artifact size: 308.20 MiB -> 308.19 MiB (-0.00%)

@Noratrieb
Copy link
Member

🎉 thanks!

@George-lewis
Copy link
Contributor Author

Great to finally get this in, thanks for your help @Nilstrieb 🎉

@George-lewis George-lewis deleted the glewis/suggest-upgrading-compiler branch January 14, 2024 23:32
@George-lewis George-lewis restored the glewis/suggest-upgrading-compiler branch January 14, 2024 23:32
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 3, 2024
…r-errors

Run some ui-fulldeps tests on stage 1 again

This is the second time I'm doing this... I'm starting to feel like stage1 ui-fulldeps tests were a mistake. Maybe I should have just put `#[cfg(bootstrap)]` there to let the bootstrap bumper fix it.

`@George-lewis` :)

finishes rust-lang#119088 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2024
Rollup merge of rust-lang#121945 - Nilstrieb:ignore-stage1, r=compiler-errors

Run some ui-fulldeps tests on stage 1 again

This is the second time I'm doing this... I'm starting to feel like stage1 ui-fulldeps tests were a mistake. Maybe I should have just put `#[cfg(bootstrap)]` there to let the bootstrap bumper fix it.

`@George-lewis` :)

finishes rust-lang#119088 (comment)
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants