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

Unconditionally show update nightly hint on ICE #122200

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Mar 8, 2024

Instead of trying to guess if a update nightly hint should be shown (by checking for system time, querying version and channel info etc.), just show the update nightly hint for nightly compilers. This avoids breaking tests that match on ICE test outputs on nightly/dev channels.

Another issue is that the outdated nightly hint triggers for ICE tests, causing a mismatch with the test expectation. There doesn't seem to be any env var to suppress this.

See https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/stage0.20compiletest.20broken/near/425543681 for context.

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Mar 8, 2024
@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu force-pushed the unconditional-nightly-update-hint branch from 25086d6 to a1cb4c2 Compare March 8, 2024 19:00
@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu force-pushed the unconditional-nightly-update-hint branch from a1cb4c2 to 1a9606b Compare March 8, 2024 19:52
@jieyouxu
Copy link
Member Author

jieyouxu commented Mar 8, 2024

@rustbot ready

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2024
@onur-ozkan
Copy link
Member

We should link this PR with #122196 if I am not mistaken?

@jieyouxu
Copy link
Member Author

jieyouxu commented Mar 9, 2024

We should link this PR with #122196 if I am not mistaken?

Unfortunately there's 2 separate bugs here:

  1. The ICE message is shown conditionally on a lot of conditions which trigger for some ICE tests when tested via outdated nightly compilers causing the stderr output to differ.
  2. stage0 run-make compiletest broken  #122196 is still broken because rmake.rs isn't compiled with --sysroot passed.

@cjgillot
Copy link
Contributor

I'm lacking context about this hint.
r? compiler

@rustbot rustbot assigned lcnr and unassigned cjgillot Mar 10, 2024
@lcnr
Copy link
Contributor

lcnr commented Mar 11, 2024

same 😅
r? compiler

@rustbot rustbot assigned estebank and unassigned lcnr Mar 11, 2024
@@ -19,5 +19,7 @@ LL | same_output(foo, rpit);
LL | same_output(foo, rpit);
| ^^^^^^^^^^^^^^^^^^^^^^

note: if you are using a nightly compiler, please make sure that you have updated to the latest nightly
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any point in having "if you are using a nightly compiler"? Why not check tcx.sess.is_nightly_build() directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought install_ice_hook and report_ice were so early in rustc_driver_impl that we didn't have a tcx available?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can use rustc_feature::UnstableFeatures::from_environment(None).is_nightly_build(); for this instead.

@estebank
Copy link
Contributor

I feel like we would want to unconditionally suggest updating for nightly releases. On stable I am of many minds. We could do a check for current date and try to figure out how long ago the current release went out, and if >6 weeks, tell the user to consider updating. But that wouldn't take into account dot-releases to fix that exact ICE being released. At the same time, I would expect people to already think "maybe there's a newer version that fixes this". On stable we also need to account for non-rustup distributed rustc, like from Linux distro package managers, which would likely not want such a message, or to customize it...

For now, can you try to check for the current build release channel and remove the uncertain language from the message?

@jieyouxu
Copy link
Member Author

jieyouxu commented Mar 16, 2024

For now, can you try to check for the current build release channel and remove the uncertain language from the message?

Hmm, wouldn't this break ICE tests checking for the exact ICE message if the compiler's channel is nightly? (because normally the ICE tests are blessed under some dev channel right, which wouldn't have this message if we gated it based on channel?)

@bors
Copy link
Contributor

bors commented Apr 8, 2024

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

@rust-cloud-vms rust-cloud-vms bot force-pushed the unconditional-nightly-update-hint branch from 1a9606b to ec2a36b Compare April 8, 2024 17:19
@jieyouxu
Copy link
Member Author

jieyouxu commented Apr 8, 2024

@estebank sorry for the ping, I want to unbork cg_clif's tests. Not sure what to do here, maybe we just emit the update nightly note unconditionally, or revert the update note altogether?

@estebank
Copy link
Contributor

estebank commented Apr 8, 2024

@jieyouxu let's check if rustc_feature::UnstableFeatures::from_environment(None).is_nightly_build() works for checking that the ICE isn't happening on a stable release. If it doesn't work, lets land this PR as is.

@jieyouxu
Copy link
Member Author

jieyouxu commented Apr 8, 2024

Thanks, I'll try that

@rust-cloud-vms rust-cloud-vms bot force-pushed the unconditional-nightly-update-hint branch from ec2a36b to 5d8b8b8 Compare April 8, 2024 23:52
@bors
Copy link
Contributor

bors commented Apr 9, 2024

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

@rust-cloud-vms rust-cloud-vms bot force-pushed the unconditional-nightly-update-hint branch from 5d8b8b8 to 09dab38 Compare April 9, 2024 14:06
@estebank
Copy link
Contributor

estebank commented Apr 9, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 9, 2024

📌 Commit 09dab38 has been approved by estebank

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 Apr 9, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#121884 (Port exit-code run-make test to use rust)
 - rust-lang#122200 (Unconditionally show update nightly hint on ICE)
 - rust-lang#123568 (Clean up tests/ui by removing `does-nothing.rs`)
 - rust-lang#123609 (Don't use bytepos offsets when computing semicolon span for removal)
 - rust-lang#123612 (Set target-abi module flag for RISC-V targets)
 - rust-lang#123633 (Store all args in the unsupported Command implementation)
 - rust-lang#123668 (async closure coroutine by move body MirPass refactoring)

Failed merges:

 - rust-lang#123701 (Only assert for child/parent projection compatibility AFTER checking that theyre coming from the same place)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7dd24d8 into rust-lang:master Apr 10, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2024
Rollup merge of rust-lang#122200 - jieyouxu:unconditional-nightly-update-hint, r=estebank

Unconditionally show update nightly hint on ICE

Instead of trying to guess if a update nightly hint should be shown (by checking for system time, querying version and channel info etc.), just show the update nightly hint for nightly compilers. This avoids breaking tests that match on ICE test outputs on nightly/dev channels.

> Another issue is that the outdated nightly hint triggers for ICE tests, causing a mismatch with the test expectation. There doesn't seem to be any env var to suppress this.

See <https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/stage0.20compiletest.20broken/near/425543681> for context.
@jieyouxu jieyouxu deleted the unconditional-nightly-update-hint branch May 27, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants