-
Notifications
You must be signed in to change notification settings - Fork 343
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
Conversation
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.) |
☔ The latest upstream changes (presumably #3414) made this pull request unmergeable. Please resolve the merge conflicts. |
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.) |
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. |
cargo-miri, miri-script, tests/ui: misc simplifications and comments Parts of #3411 that can be landed now.
This comment was marked as resolved.
This comment was marked as resolved.
a49c23c
to
b0f6fea
Compare
Actually looks like eyre still works with this. I guess by using neither RUSTC_WRAPPER nor The one thing that does not work any more with this PR is setting So I think this is currently my preferred approach to getting rid of the |
0333893
to
268ad82
Compare
cargo-miri, miri-script, tests/ui: misc simplifications and comments Parts of rust-lang/miri#3411 that can be landed now.
Also this should only break with old autocfg when using |
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. |
☔ The latest upstream changes (presumably #3453) made this pull request unmergeable. Please resolve the merge conflicts. |
…ts passed to the driver
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. |
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.
The build has long finished, but somehow bors didn't notice...? |
☀️ Test successful - checks-actions |
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)
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)
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)
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)
(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.