library/test: always enable unstable features for miri#153369
library/test: always enable unstable features for miri#153369rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Conversation
The unstable features of the `test` crate used to be default-enabled, and manually disabled when building the beta and stable channels. Commit dae8ea9 flipped that to default-disabled, only enabled for nightly and dev channels. However, this broke miri testing on the beta/stable channels, because it also uses unstable features -- which should be fine to enable just for its own sysroot build. Now the `test` build script makes that happen by noticing the `MIRI_CALLED_FROM_SETUP` environment variable.
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
1.95-beta is currently blocked on this problem: #153314 (comment) @rustbot label +beta-nominated |
|
I looked at this again and I guess that this is a natural consequence of flipping the default, before we used to use unstable features in some places "by accident", such as in Miri, while they need an explicit opt-in. Would be nicer to set this for Miri tests from bootstrap, rather than in the build probe, but I guess that this would still break miri when executed outside of bootstrap? I think that as a hotfix for beta this is good enough. You can r=me if you tested that this unblocks the beta build (the miri test on stable/beta). |
Yes, I think a bootstrap-only fix would break miri elsewhere -- except we don't ship miri on stable or beta anyway. So maybe that's fine, although I'm not sure how else bootstrap would help this, because it has to do with how miri builds |
[beta] prepare Rust 1.95.0-beta Ref: https://forge.rust-lang.org/release/process.html#beta-pr - Replace version placeholders with 1.95.0 - Bump to beta release - library/test: always enable unstable features for miri #153369 r? cuviper
|
I am very surprised Miri is even built, let alone run, on beta/stable branches. It's a nightly-only tool after all.
Any idea which flag this is about? I don't think Miri passes any of its own arguments to the test runner. We pass stuff to cargo and rustc of course but that should work thanks to RUSTC_BOOTSTRAP. |
| let stdout = String::from_utf8(version.stdout).unwrap(); | ||
| // Miri testing uses unstable features, so always enable that for its sysroot. | ||
| // Otherwise, only enable unstable if rustc looks like a nightly or dev build. | ||
| let enable_unstable_features = std::env::var("MIRI_CALLED_FROM_SETUP").is_ok() || { |
There was a problem hiding this comment.
Note that MIRI_CALLED_FROM_SETUP is considered an internal env var of cargo-miri, it's something we might rearrange any time.
There was a problem hiding this comment.
A better env varo to check would be CARGO_CFG_MIRI. That's the build script way of checking whether --cfg miri is enabled in the crate itself.
Or you could use cfg!(miri) in the crate itself.
But I'd still like to understand why this is even needed...
| let rustc = std::env::var("RUSTC").unwrap_or_else(|_| "rustc".into()); | ||
| let version = std::process::Command::new(rustc).arg("-vV").output().unwrap(); | ||
| let stdout = String::from_utf8(version.stdout).unwrap(); | ||
| // Miri testing uses unstable features, so always enable that for its sysroot. |
There was a problem hiding this comment.
I don't think we use any unstable features of the libtest harness. So, no idea what's happening here. Is cargo forwarding the -Zunstable-features (that is just meant to enable unstable cargo features) to libtest? Why does the RUSTC_BOOTSTRAP that bootstrap sets not suffice (it apparently suffices for cargo)?
…uwer Rollup of 8 pull requests Successful merges: - #153361 (enable `PassMode::Indirect { on_stack: true, .. }` tail call arguments) - #153369 (library/test: always enable unstable features for miri) - #152283 (Properly pass offload sizes to kernel args) - #153323 (Remove `impl QueryVTable`) - #153385 (Fix comment on `is_horizontal_whitespace`) - #153394 (fix(thir): Include `NoneWithError` in Enum Struct Tail Assertion) - #153419 (rustc_llvm: add missing `-` to flag-comparison logic) - #153423 (Update dispatch2 to v0.3.1)
|
Since this happens when invoking Miri on the libcore test suite, I can only assume that bootstrap itself passes some unstable arguments to the test suite. And then the check for the RUSTC_BOOTSTRAP env var inside libtest doesn't work because Miri runs in isolation mode, not giving the program access to environment variables on the host. I see two options:
I'd prefer the first option; the second one relies on Miri currently being always-unstable but it'd be easy to forget about updating this if that ever changes. It's also conceptually wrong -- the reason this works without Miri is the RUSTC_BOOTSTRAP; let's make that also the reason it works with Miri. |
|
I opened #153437 as my proposed alternative to this PR. But I'm also fine with landing this for now so we can get the beta out of the door, and then later revert this PR and land mine. |
Rollup merge of #153369 - cuviper:unstable-libtest, r=Kobzol library/test: always enable unstable features for miri The unstable features of the `test` crate used to be default-enabled, and manually disabled when building the beta and stable channels. Commit dae8ea9 flipped that to default-disabled, only enabled for nightly and dev channels. However, this broke miri testing on the beta/stable channels, because it also uses unstable features -- which should be fine to enable just for its own sysroot build. Now the `test` build script makes that happen by noticing the `MIRI_CALLED_FROM_SETUP` environment variable.
The unstable features of the
testcrate used to be default-enabled,and manually disabled when building the beta and stable channels. Commit
dae8ea9 flipped that to default-disabled, only enabled for nightly
and dev channels.
However, this broke miri testing on the beta/stable channels, because it
also uses unstable features -- which should be fine to enable just for
its own sysroot build. Now the
testbuild script makes that happen bynoticing the
MIRI_CALLED_FROM_SETUPenvironment variable.