Skip to content

Run mir-opt panic=abort tests on CI - #160200

Open
Kobzol wants to merge 10 commits into
rust-lang:mainfrom
Kobzol:mir-opt-panic-abort-on-ci
Open

Run mir-opt panic=abort tests on CI#160200
Kobzol wants to merge 10 commits into
rust-lang:mainfrom
Kobzol:mir-opt-panic-abort-on-ci

Conversation

@Kobzol

@Kobzol Kobzol commented Jul 30, 2026

Copy link
Copy Markdown
Member

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 30, 2026
@rustbot

rustbot commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu

Copy link
Copy Markdown
Member

cf. #154607

@jieyouxu jieyouxu self-assigned this Jul 30, 2026
Comment thread src/bootstrap/src/core/build_steps/test.rs Outdated
@RalfJung

Copy link
Copy Markdown
Member

I don't know nearly enough about bootstrap to review this.^^
@rustbot reroll

@rustbot rustbot assigned jackh726 and unassigned RalfJung and jieyouxu Jul 30, 2026
// Now also run the tests for the host with panic=abort
let panic_abort_target = builder
.ensure(MirOptPanicAbortSyntheticTarget { compiler: self.compiler, base: self.target });
run(panic_abort_target);

@RalfJung RalfJung Jul 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will be redundant with --bless I think?
mir-opt --bless already takes very long due to all the sysroots it is building, I'd prefer it not to become even slower.

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is only redundant if the host target matches the hardcoded targets we have below. I can add an if condition to skip them if they match the host target.

That being said, if the stdlib it built, re-running the tests again should be very fast, they will just be ignored.

@RalfJung RalfJung Jul 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is always redundant. There are only 4 actually distinct configurations, so running more than 4 targets on --bless is redundant.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see, so the target doesn't really matter, the only thing that matters is bitwidth and the panic strategy? Yeah, in that case I will try to optimize it to always run 2 targets when not blessing, and 4 targets when blessing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah.

When checking it's good to check the actual self.target to ensure CI covers them all. But in principle only the bitwidth and panic strategy should matter.

@jieyouxu jieyouxu assigned jieyouxu and unassigned jackh726 Jul 30, 2026
@RalfJung

Copy link
Copy Markdown
Member

Cc @saethlin

@RalfJung

Copy link
Copy Markdown
Member

The existing behavior of --bless is kind of annoying since it takes so long to build all these sysroots... and it actually builds 5 sysroots when 4 should be enough. (With this PR as-is it'd become 6 sysroots!) Not sure if there is something we could improve there. Does bootstrap know whether self.target is a 32bit target or 64bit target? If yes, we could bless

  • current target
  • current target with panic=abort
  • target of other bitwidth
  • target of other bitwidth with panic=abort

And only run the first two steps for non-bless mode. That'd be nice.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2026
@Kobzol

Kobzol commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

And only run the first two steps for non-bless mode. That'd be nice.

But when we do the check, we should test all 4 combinations, right? Otherwise we could get back into the same problem, where some of the combinations are not checked on CI, and thus bless becomes "dirty".

@RalfJung

RalfJung commented Aug 7, 2026 via email

Copy link
Copy Markdown
Member

@Kobzol

Kobzol commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Ah, I see, makes sense. Thanks.

@Kobzol
Kobzol force-pushed the mir-opt-panic-abort-on-ci branch from a65622e to b63868d Compare August 7, 2026 08:23
@rustbot

This comment has been minimized.

@Kobzol

Kobzol commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Ok, I refactored the whole thing. Now when checking, we run the current target, and then the current target with the "inverted" panic strategy. And when blessing, we run the current target + four fixed targets, as before (there was a comment saying that running also the current target when blessing is desired).

It is now possible to create synthetic targets with both panic abort/unwind strategies, and the generated miropt test steps are now created as separate step instances, which means that they will be more explicit tin the step trace, and they will also be properly cached.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-bors

rust-bors Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 5bc30a9 has been approved by jieyouxu

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 Sep 1, 2026
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 2, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Sep 2, 2026
@Zalathar

Zalathar commented Sep 2, 2026

Copy link
Copy Markdown
Member

Probably failed in rollup due to bootstrap snapshot tests: #162184 (comment)

@bors r-
@bors try jobs=i686-msvc-2

@rust-bors rust-bors Bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 2, 2026
@rust-bors

rust-bors Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

This pull request was unapproved.

View changes since this unapproval

@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Sep 2, 2026
Run mir-opt panic=abort tests on CI


try-job: i686-msvc-2
@rust-bors

rust-bors Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

💔 Test for 25a5398 failed: CI. Failed job:

@rust-log-analyzer

This comment has been minimized.

@Kobzol
Kobzol force-pushed the mir-opt-panic-abort-on-ci branch from 5bc30a9 to 3ba9f00 Compare September 3, 2026 08:00
@rustbot

rustbot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@Kobzol

Kobzol commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

@bors try jobs=i686-msvc-2

@rust-bors

rust-bors Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

⌛ Trying commit 3ba9f00 with merge ff522b0

To cancel the try build, run the command @bors try cancel.

Workflow: https://github.com/rust-lang/rust/actions/runs/33731068286

rust-bors Bot pushed a commit that referenced this pull request Sep 3, 2026
Run mir-opt panic=abort tests on CI


try-job: i686-msvc-2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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