Prevent stable libtest from supporting -Zunstable-options

[thomcc]

Took a while for me to get around to this but seems trivial (unless I'm missing some reason this will break all our tests). Fixes #75526

Basically libtest already tries to handle this in

rust/library/test/src/cli.rs

Lines 310 to 318 in 501ad02

	fn is_nightly() -> bool {
	// Whether this is a feature-staged build, i.e., on the beta or stable channel
	let disable_unstable_features =
	option_env!("CFG_DISABLE_UNSTABLE_FEATURES").map(|s| s != "0").unwrap_or(false);
	// Whether we should enable unstable features for bootstrapping
	let bootstrap = env::var("RUSTC_BOOTSTRAP").is_ok();
	bootstrap || !disable_unstable_features
	}

But that env var was not passed. I'm guessing at one point this code (or a common ancestor) was used to compile the standard library/libtest, but that is no longer the case (or perhaps it never worked, I don't have time to go digging).

I don't love that this is a "allow unstable by default" situation, as it means things like rustc-build-sysroot could accidentally get unstable (CC @RalfJung) even if this is fixed here, but it's consistent with what happens in rustc_feature, so... yeah.

This is user-facing after all, even if it's hard to imagine the outcome of that conversation being "lets continue allowing use of -Zunstable-features from stable rust" (especially since a RUSTC_BOOTSTRAP=1-shaped loophole remains)... I think it probably should get a vibe check in the t-libs meeting (and plausibly a relnote along the lines of "hey cargo test -- -Zunstable-options --some --unstable --stuff=here used to work on stable, that's been fixed, sorry").

I'll nominate it for that after CI comes up green (I've done a smoke check but don't know what (if anything) will need bootstrap to enable RUSTC_BOOTSTRAP=1 when running tests)

r? @jyn514

[jyn514]

r=me once you bring it up in the libs meeting. thank you!

[thomcc]

Nominated for discussion in t-libs meeting and mention in release notes (as a compatibility concern).

To summarize: on stable today you can pass -Zunstable-options to the libtest runner binary and it will happily let you use whatever unstable options that allows. This patch fixes that.

My stance is that anybody doing this on stable knows that they're breaking the rules (it's called -Zunstable-options for a reason...), so they shouldn't be that surprised. If they really want to continue breaking the rules, they can migrate to setting RUSTC_BOOTSTRAP=1 (which isn't really supported, but it's also not exactly something we really want to break)

It's hard to tell how much breakage this could cause by skimming through github code search, since many of our tools use the same -Zunstable-options flag name (rustc, cargo, ...), and even from the cases that look like they're invoking the test runner binary, there's no easy well to tell the rustc channel being used. That said it looks like this should cause very little impact

[ehuss]

I can find quite a few matches via codesearch. Here is a relatively rough query:

https://cs.github.com/?scopeName=All+repos&scope=&q=%2F%28-Z+%3Funstable-options.*%28--exclude-should-panic%7C--force-run-in-process%7C--ensure-time%7C--shuffle%7C--shuffle-seed%7C--report-time%29%29%7C%28%28--exclude-should-panic%7C--force-run-in-process%7C--ensure-time%7C--shuffle%7C--shuffle-seed%7C--report-time%29.*-Z+%3Funstable-options%29%7C%28test.*%28-Z+%3Funstable-options.*%28--format%5B+%3D%5Djson%29%29%29%7C%28test.*%28%28--format%5B+%3D%5Djson%29.*-Z+%3Funstable-options%29%29%2F

It is difficult to say how much of that is running on nightly, but I definitely see some workflows that are on stable. It's hard to predict such a thing, but I would not be very confident that the impact will be low.

[thomcc]

It is difficult to say how much of that is running on nightly, but I definitely see some workflows that are on stable. It's hard to predict such a thing, but I would not be very confident that the impact will be low.

Ah, thanks, that's a better query than I used. Well, I still think that to some extent these users get what they asked for. It's not like there isn't a equally-dubious migration path for them anyway.

[thomcc]

For meeting reference, these are the unstable flags which are allowed if -Zunstable-options is also passed:

--report-time
--ensure-time
--force-run-in-process
--exclude-should-panic
--shuffle
--shuffle-seed
--format json
--format junit

The most common ones seem to be --format json and --report-time. The former, and often both, are frequently used in conjunction with cargo2junit¹, which seems a somewhat popular tool which recommends that users use it as cargo test -- -Zunstable-options --format json --report-time | cargo2junit in its README.md.

---

That said, despite the popularity of the tool, I will note that the users still are explicitly invoking -Zunstable-options themselves to use it, which is noted in the help output as being (intended to be) nightly-only:

    -Z unstable-options Enable nightly-only flags:
                        unstable-options = Allow use of experimental features

And attempting to use the flags listed at the start of this comment without also passing the -Z unstable-options flag causes libtest to emit an error, for example:

$ cargo +stable test -- --report-time
error: The "report-time" flag is only accepted on the nightly compiler with -Z unstable-options

$ cargo +stable test -- --format json
error: The "json" format is only accepted on the nightly compiler

So, I do think these users should know that they were opting out of stability by doing this.

Footnotes

1. I haven't investigated if --format junit is at feature parity, but neither are stable so 🤷‍♂️. ↩

[thomcc]

The outcome from the meeting seems to be to merge this: https://rust-lang.zulipchat.com/#narrow/stream/259402-t-libs.2Fmeetings/topic/Meeting.202023-03-15/near/342101747

@bors r=jyn514

[bors]

📌 Commit ce6adcc has been approved by jyn514

It is now in the queue for this repository.

[calebcartwright]

Wish I'd seen this before, but wanted to note that another area that will see some level of fallout from this is IDE/editor integrations which support test execution via reliance on the json output

[nrc]

This will break some of our CI :-( I think it would have been better if there was a deprecation period and some notice given to the likes of cargo2junit. I appreciate it is just fixing a loophole, but this is still a de facto breaking change for users. Furthermore, unlike with libs, there is no workaround here. Because it's a tool, not a library, you can't just use a crate or copy and paste code into your own trait or something, so the user pain here is worse. In fact, the only workaround I can see is to use RUSTC_BOOTSTRAP (or pin to 1.69), and pushing users towards those seems bad.

[calebcartwright]

Wish I'd seen this before, but wanted to note that another area that will see some level of fallout from this is IDE/editor integrations which support test execution via reliance on the json output

To expand on this since it seems much of the consideration around impact was focused on users knowingly opting-in, users who utilize these IDE/editor plugins are not aware that those plugins were leveraging this under the hood.

I maintain one such plugin for VS Code that's also going to be broken by this change, as will all users of this plugin. While I definitely wouldn't proclaim it to be particularly popular, based on the download stats over the last 30 days I think it's fair to say there's a non-zero portion of the Rust population that's both unknowingly using this and which will be impacted just from this one plugin for this one editor:

I'd love to see something like Nick's suggestion in #49359 (comment) happen, but also suspect this ship has likely already sailed. I do, however, think it'd be worth reviewing the IDE/editor landscape to see if there are other cases that were utilizing this aspect of libtest, if for no reason other than communication (relnotes, comms to plugin maintainers, etc.), since there could be a lot more transitive user impact.

[vlad20012]

Ook, this PR broke the test running functionality in IntelliJ Rust. Is there a tracking issue for stabilizing --format json in the test runner?

[mati865]

Yeah, It was mentioned above previous comment: #49359

[isabelatkinson]

This has also broken CI for the mongodb crate; we rely on cargo2junit to format our test output. We've decided to pin to version 1.69 for the time being, but stabilizing --format json and --report-time would be very useful for us.

[detly]

Just some notes from someone who was using cargo2junit via the JSON output until this change:

• We migrated our Gitlab CI to cargo-nextest extremely easily (a couple of hours for a monorepo with multiple workspace projects). We considered this a better solution than pinning to 1.69 but it depends on your needs.
• If it is the jUnit XML output you want, there is a more specific issue for jUnit reporting from libtest directly. It is not a replacement for the JSON-to-jUnit workflow because (a) also unstable and (b) has some issues, but it might be better to direct your attention and effort there instead of the JSON output if that's the thing you really need.

[marcusirgens]

While I understand that this PR corrects invalid behaviour, we're also hit by both the IntelliJ issue (intellij-rust/intellij-rust#10512), and the JSON flag has been unstable for over five years (#49359). Is there a plan to stabilize this feature, or are we likely to see it idle for five more years?

I understand that users (like the IntelliJ team) have been "doing it wrong", but if the flag has been used in the wild without any warnings or feedback for five years, this feels like a breaking change to me.

[jyn514]

The best way to stabilize features is to work on them yourself. The tracking issue has some suggestions for changes that might be useful: #49359 (comment)

I understand that users (like the IntelliJ team) have been "doing it wrong", but if the flag has been used in the wild without any warnings or feedback for five years, this feels like a breaking change to me.

The flag explicitly gave an error unless you passed -Zunstable-options. I don't have a lot of sympathy for people intentionally bypassing our stability policy.