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

unwind: don't build dependency when building for Miri #100532

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

RalfJung
Copy link
Member

This is basically re-submitting #94813.

In that PR there was a suggestion to instead have bootstrap set a RUST_CHECK env var and use that rather than doing something Miri-specific. However, such an env var would mean that when switching between ./x.py check and ./x.py build, the build script gets re-run each time, which doesn't seem good. So I think for now checking for Miri probably causes fewer problems.

r? @Mark-Simulacrum

@rustbot
Copy link
Collaborator

rustbot commented Aug 14, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2022

if env::var_os("CARGO_CFG_MIRI").is_some() {
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 there be a rerun-if-changed for this, or does cargo rerun anyway when the cfg flags change?

Copy link
Member

Choose a reason for hiding this comment

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

A dedicated rerun-if-change definitely won't hurt, I think.

That said, presumably you actually don't want to re-run the script (and rebuild the world) when switching between --cfg=miri and not, if you've previously had it build successfully? RUST_CHECK has similar behavior where once we've built LLVM, it never gets passed down (even in check builds).

I admit I've not followed the story around cfg(miri) enough to know where we're setting that or how, can you point me to that?

Copy link
Member Author

@RalfJung RalfJung Aug 15, 2022

Choose a reason for hiding this comment

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

A dedicated rerun-if-change definitely won't hurt, I think.

Done.

I admit I've not followed the story around cfg(miri) enough to know where we're setting that or how, can you point me to that?

Miri sets --cfg miri on all things it builds: here the flag is defined, here it is added.
Miri also uses a separate target dir, so for a given target dir, it should never be the case that it is sometimes built with --cfg miri and sometimes without.

RUST_CHECK has similar behavior where once we've built LLVM, it never gets passed down (even in check builds).

So that is some further extra magic and rustbuild implements?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay, if this never differs for a given target dir then it should be OK -- no need for special magic.

Yes, RUST_CHECK is only set by rustbuild (not Cargo or others); definition here.

@Mark-Simulacrum
Copy link
Member

@rustbot author

@rustbot rustbot 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 14, 2022
@RalfJung
Copy link
Member Author

@rustbot ready

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

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 16, 2022

📌 Commit 63113c8 has been approved by Mark-Simulacrum

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 Aug 16, 2022
TaKO8Ki added a commit to TaKO8Ki/rust that referenced this pull request Aug 16, 2022
…acrum

unwind: don't build dependency when building for Miri

This is basically re-submitting rust-lang#94813.

In that PR there was a suggestion to instead have bootstrap set a `RUST_CHECK` env var and use that rather than doing something Miri-specific. However, such an env var would mean that when switching between `./x.py check` and `./x.py build`, the build script gets re-run each time, which doesn't seem good. So I think for now checking for Miri probably causes fewer problems.

r? `@Mark-Simulacrum`
TaKO8Ki added a commit to TaKO8Ki/rust that referenced this pull request Aug 17, 2022
…acrum

unwind: don't build dependency when building for Miri

This is basically re-submitting rust-lang#94813.

In that PR there was a suggestion to instead have bootstrap set a `RUST_CHECK` env var and use that rather than doing something Miri-specific. However, such an env var would mean that when switching between `./x.py check` and `./x.py build`, the build script gets re-run each time, which doesn't seem good. So I think for now checking for Miri probably causes fewer problems.

r? ``@Mark-Simulacrum``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 17, 2022
…acrum

unwind: don't build dependency when building for Miri

This is basically re-submitting rust-lang#94813.

In that PR there was a suggestion to instead have bootstrap set a `RUST_CHECK` env var and use that rather than doing something Miri-specific. However, such an env var would mean that when switching between `./x.py check` and `./x.py build`, the build script gets re-run each time, which doesn't seem good. So I think for now checking for Miri probably causes fewer problems.

r? ```@Mark-Simulacrum```
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 17, 2022
…iaskrgr

Rollup of 15 pull requests

Successful merges:

 - rust-lang#99474 (Rustdoc json tests: New `@hasexact` test command)
 - rust-lang#99972 (interpret: only consider 1-ZST when searching for receiver)
 - rust-lang#100018 (Clean up `LitKind`)
 - rust-lang#100379 (triagebot: add translation-related mention groups)
 - rust-lang#100389 (Do not report cycle error when inferring return type for suggestion)
 - rust-lang#100489 (`is_knowable` use `Result` instead of `Option`)
 - rust-lang#100532 (unwind: don't build dependency when building for Miri)
 - rust-lang#100608 (needless separation of impl blocks)
 - rust-lang#100621 (Pass +atomics-32 feature for {arm,thumb}v4t-none-eabi)
 - rust-lang#100646 (Migrate emoji identifier diagnostics to `SessionDiagnostic` in rustc_interface)
 - rust-lang#100652 (Remove deferred sized checks (make them eager))
 - rust-lang#100655 (Update books)
 - rust-lang#100656 (Update cargo)
 - rust-lang#100660 (Fixed a few documentation errors)
 - rust-lang#100661 (Fixed a few documentation errors)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 56b02b2 into rust-lang:master Aug 17, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 17, 2022
@RalfJung RalfJung deleted the unwind-miri branch August 18, 2022 20:44
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants