Skip to content

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 25, 2024

(Extracted from #3409)

This entirely moves the responsibility of setting miri-sysroot to whatever invokes the Miri driver. cargo-miri knows whether it is inside rustdoc or not and can adjust accordingly. I previously avoided doing that because there are a bunch of places that are invoking the driver (cargo-miri, the ui test suite, ./miri run, ./x.py run miri) and they all need to be adjusted now. But it is also somewhat less fragile as we usually have more information there -- and we can just decide that ./miri run file.rs --sysroot path is not supported. The advantage of this is that the driver is reasonably clean and doesn't need magic environment variables like MIRI_SYSROOT, and we don't have to fight rustc_driver to use a different default sysroot. Everything is done in cargo-miri (and the other much simpler driver wrappers) where it can hopefully be debugged much better.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 25, 2024

The issue with this is that it breaks autocfg for cross-target interpretation. We set RUSTC to be the Miri driver, and autocfg ignores RUSTC_WRAPPER, so it directly invokes the driver and there is no way for us to inject the right sysroot.

So this is probably blocked on cuviper/autocfg#26. (dtolnay's crates seem to already support RUSTC_WRAPPER.)

@bors
Copy link
Contributor

bors commented Mar 25, 2024

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

@RalfJung RalfJung marked this pull request as ready for review March 26, 2024 09:41
@RalfJung
Copy link
Member Author

autocfg got patched. :) So this should pass CI now.

It's still a bit fragile though due to rust-lang/cargo#10885. Also if we land this now cross-builds will break for anyone using autocfg older than 1.2. (But host builds should work, autocfg probes will just use the wrong sysroot.)

@RalfJung
Copy link
Member Author

RalfJung commented Mar 26, 2024

Ah the eyre build probe also still fails to apply RUSTC_WRAPPER. So landing this would break everything that uses eyre...

IOW, blocked on eyre-rs/eyre#84.

@RalfJung RalfJung marked this pull request as draft March 26, 2024 10:32
bors added a commit that referenced this pull request Mar 26, 2024
cargo-miri, miri-script, tests/ui: misc simplifications and comments

Parts of #3411 that can be landed now.
@bors

This comment was marked as resolved.

@RalfJung RalfJung force-pushed the sysroot branch 2 times, most recently from a49c23c to b0f6fea Compare March 26, 2024 15:50
@RalfJung
Copy link
Member Author

RalfJung commented Mar 26, 2024

Actually looks like eyre still works with this. I guess by using neither RUSTC_WRAPPER nor --target it ends up checking the wrong thing (it checks whether the build succeeds for the host, not the target we are actually building for) but the check succeeds. I hope eyre doesn't check for anything target-specific in its build probe. Or maybe the check fails but it is done in a way that failure is harmless (nightly feature gets deactivated and then everything works fine).

The one thing that does not work any more with this PR is setting --target but then not applying RUSTC_WRAPPER. Only autocfg ever did that. (And some old versions of anyhow/thiserror, but that has been fixed long ago.)

So I think this is currently my preferred approach to getting rid of the --sysroot command-line hackery in the Miri driver.
@rust-lang/miri how long do you think we should wait before rolling this out? autocfg 1.2 is a semver-compatible update but it has been released very recently, so it will take a bit until everyone picks it up.

@RalfJung RalfJung force-pushed the sysroot branch 3 times, most recently from 0333893 to 268ad82 Compare March 26, 2024 19:07
@RalfJung RalfJung marked this pull request as ready for review March 29, 2024 19:34
@RalfJung RalfJung added the S-waiting-on-review Status: Waiting for a review to complete label Mar 29, 2024
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Mar 31, 2024
cargo-miri, miri-script, tests/ui: misc simplifications and comments

Parts of rust-lang/miri#3411 that can be landed now.
@RalfJung
Copy link
Member Author

Also this should only break with old autocfg when using --target; plain cargo miri test should be unaffected.

@saethlin
Copy link
Member

how long do you think we should wait before rolling this out?

Based on the downloads graph on crates.io, 1.2 seems like just over a third of downloads. I'm concerned we have a surprising amount of --target usage due to our much better shim support on x86_64-unknown-linux-gnu, so I would prefer to wait a week from now.

@bors
Copy link
Contributor

bors commented Apr 6, 2024

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

@RalfJung
Copy link
Member Author

I would prefer to wait a week from now.

We have waited 2 weeks since then. Download stats for autocfg haven't really changed.

Let's ship this; we can always revert if there are too many problems.
@bors r+

@bors
Copy link
Contributor

bors commented Apr 15, 2024

📌 Commit 8727602 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 15, 2024

⌛ Testing commit 8727602 with merge f42ede2...

bors added a commit that referenced this pull request Apr 15, 2024
Handle Miri sysroot entirely outside the Miri driver

(Extracted from #3409)

This entirely moves the responsibility of setting miri-sysroot to whatever *invokes* the Miri driver.  cargo-miri knows whether it is inside rustdoc or not and can adjust accordingly. I previously avoided doing that because there are a bunch of places that are invoking the driver (cargo-miri, the ui test suite, `./miri run`, `./x.py run miri`) and they all need to be adjusted now. But it is also somewhat less fragile as we usually have more information there -- and we can just decide that `./miri run file.rs --sysroot path` is not supported. The advantage of this is that the driver is reasonably clean and doesn't need magic environment variables like MIRI_SYSROOT, and we don't have to fight rustc_driver to use a different default sysroot. Everything is done in cargo-miri (and the other much simpler driver wrappers) where it can hopefully be debugged much better.
@RalfJung
Copy link
Member Author

The build has long finished, but somehow bors didn't notice...?
@bors retry

@bors
Copy link
Contributor

bors commented Apr 15, 2024

⌛ Testing commit 8727602 with merge f250781...

@bors
Copy link
Contributor

bors commented Apr 15, 2024

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing f250781 to master...

@bors bors merged commit f250781 into rust-lang:master Apr 15, 2024
@RalfJung RalfJung deleted the sysroot branch April 16, 2024 11:32
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 13, 2024
Fix Miri sysroot for `x run`

Miri no longer (after rust-lang/miri#3411) respects `MIRI_SYSROOT` and wants to be treated like a REAL rustc, with `--sysroot`. \*pats Miri\* sure Miri, just for you :3.

fixes rust-lang#126233

r? RalfJung (or whoever else feels like it)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 13, 2024
Fix Miri sysroot for `x run`

Miri no longer (after rust-lang/miri#3411) respects `MIRI_SYSROOT` and wants to be treated like a REAL rustc, with `--sysroot`. \*pats Miri\* sure Miri, just for you :3.

fixes rust-lang#126233

r? RalfJung (or whoever else feels like it)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 14, 2024
Rollup merge of rust-lang#126238 - Nilstrieb:run,miri,run, r=RalfJung

Fix Miri sysroot for `x run`

Miri no longer (after rust-lang/miri#3411) respects `MIRI_SYSROOT` and wants to be treated like a REAL rustc, with `--sysroot`. \*pats Miri\* sure Miri, just for you :3.

fixes rust-lang#126233

r? RalfJung (or whoever else feels like it)
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 28, 2024
Fix Miri sysroot for `x run`

Miri no longer (after rust-lang/miri#3411) respects `MIRI_SYSROOT` and wants to be treated like a REAL rustc, with `--sysroot`. \*pats Miri\* sure Miri, just for you :3.

fixes #126233

r? RalfJung (or whoever else feels like it)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants