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

Handle Miri sysroot entirely outside the Miri driver #3411

Merged
merged 2 commits into from
Apr 15, 2024

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
Collaborator

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
Collaborator

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
Collaborator

bors commented Apr 15, 2024

📌 Commit 8727602 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Collaborator

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
Collaborator

bors commented Apr 15, 2024

⌛ Testing commit 8727602 with merge f250781...

@bors
Copy link
Collaborator

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
8 checks passed
@RalfJung RalfJung deleted the sysroot branch April 16, 2024 11:32
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.

None yet

3 participants