Support rustc's `-Z panic-abort-tests` in Cargo #7460

alexcrichton · 2019-09-30T20:02:06Z

Recently added in rust-lang/rust#64158 the -Z panic-abort-tests flag
to the compiler itself will activate a mode in the test crate which
enables running tests even if they're compiled with panic=abort. It
effectively runs a test-per-process.

This commit brings the same support to Cargo, adding a -Z panic-abort-tests flag to Cargo which allows building tests in
panic=abort mode. While I wanted to be sure to add support for this in
Cargo before we stabilize the flag in rustc, I don't actually know how
we're going to stabilize this here. Today Cargo will automatically
switch test targets to panic=unwind, and so if we actually were to
stabilize this flag then this configuration would break:

[profile.dev]
panic = 'abort'

In that case tests would be compiled with panic=unwind (due to how
profiles work today) which would clash with crates also being compiled
with panic=abort. I'm hopeful though that we can perhaps either figure
out a solution for this and maybe even integrate it with the ongoing
profiles work.

rust-highfive · 2019-09-30T20:02:10Z

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2019-09-30T20:02:16Z

r? @ehuss

alexcrichton · 2019-09-30T20:03:18Z

src/cargo/ops/cargo_compile.rs

@@ -639,7 +639,7 @@ fn generate_targets<'a>(
 ) -> CargoResult<Vec<Unit<'a>>> {
    // Helper for creating a `Unit` struct.
    let new_unit = |pkg: &'a Package, target: &'a Target, target_mode: CompileMode| {
-        let unit_for = if bcx.build_config.mode.is_any_test() {
+        let unit_for = if target_mode.is_any_test() {


@ehuss I'll want to draw your eye particularly to this line. I sort of blindly made this change assuming that this is the best interpretation nowadays, and it is required to remove the check for mode.is_any_test() in profiles.rs above

ehuss · 2019-10-04T15:48:12Z

Would it be possible to squelch this warning when -Zpanic-abort-tests is enabled?

So I see a few different options to handle profile.dev.panic="abort":

Wait for named-profiles to stabilize. It changes the test profile to inherit from dev, so the test profile will naturally get abort. I'm not sure if we will stabilize in that form, but it is an option.
Another option is to detect this situation, and force the correct strategy. This would be kinda the opposite of UnitFor::panic_abort_ok, where dependencies must be unwind. I'm not sure if that's really valuable (why would you want unwind?). Or it could work the other way, where 'abort' is infectious and forces tests to be abort.

I'm curious, disregarding the above issue, can you think of any reason not to stabilize this feature as-is? Are there concerns about performance running a process per test? Or are there any situations where someone has an abort crate where they really do want unwind in tests?

From my perspective, this seems like it is much nicer for abort users, and makes much more sense. However, I've not worked on anything that uses abort, so I don't know if it is obviously better.

alexcrichton · 2019-10-15T14:01:07Z

Sorry for the delay, back now though!

I hadn't though too too much about stabilizing this option honestly, I just wanted to make sure we at least tried to prove it out in Cargo before we stabilize it in rustc to make sure it works. I suspect the stabilization here will be pretty tricky and it probably won't end up looking like this PR in the long run.

I'm not certain that Cargo would correctly handle different configurations of tests today. For example if you configure dev to panic=abort and test to panic=unwind I think that does that it says on the tin and then trivially causes a compilation error since you can't mix them.

I think the best way we'll want to implement this is long-term we just ignore the test profile panic configuration, reading whatever dev or release is. I don't think there's any use case to building only tests in panic=abort, and otherwise panic=abort forces everything to be such and panic=unwind is the default anyway.

The biggest thing I'm worried about is if we've accidentally conditioned users the wrong way, but I had forgotten about this warning. Given that we've always warned we can probably reasonly assume no one has panic configuration for tests, and I think that means we just need to probably keep ignoring panic configuration for tests?

Ok that's a bit rambly, but how about:

Let's merge this for now to get a proof-of-concept that at least somewhat works
Open a tracking issue (or link this with the rust-lang/rust tracking issue) for this feature
Note on the tracking issue that Cargo's desired strategy for handling this new feature of rustc is:
- Always ignore panic on test and bench profiles
- Force tests to match the panic strategy of dev and release (giving us the behavior of inheritance)

And... I think that'd do it?

ehuss · 2019-10-17T22:00:58Z

I like the strategy of ignoring panic for test. Why not implement that now instead of waiting?

Just an element of curiosity, I looked at how crates.io crates were using the panic setting:

~150 crates set it only on release. Most of these are binaries, with no tests.
~20 set it for both dev and release.
7 do random things (like only bench or all four profiles).

It seems like a pretty small number to me overall. Perhaps most abort crates are not published? (Which would not surprise me.)

alexcrichton · 2019-10-18T13:41:46Z

Heh, seems reasonable to me! Should be done now.

I think now the stabilization strategy for this would look like "remove everything related to it being unstable" and that should be it!

ehuss · 2019-10-18T19:57:46Z

Sounds good to me. Looks like 2 tests need to be updated for the new behavior.

Recently added in rust-lang/rust#64158 the `-Z panic-abort-tests` flag to the compiler itself will activate a mode in the `test` crate which enables running tests even if they're compiled with `panic=abort`. It effectively runs a test-per-process. This commit brings the same support to Cargo, adding a `-Z panic-abort-tests` flag to Cargo which allows building tests in `panic=abort` mode. While I wanted to be sure to add support for this in Cargo before we stabilize the flag in `rustc`, I don't actually know how we're going to stabilize this here. Today Cargo will automatically switch test targets to `panic=unwind`, and so if we actually were to stabilize this flag then this configuration would break: [profile.dev] panic = 'abort' In that case tests would be compiled with `panic=unwind` (due to how profiles work today) which would clash with crates also being compiled with `panic=abort`. I'm hopeful though that we can perhaps either figure out a solution for this and maybe even integrate it with the ongoing profiles work.

We've always ignored the `panic` settings configured in test/bench profiles, so this just continues to ignore them but in a slightly different way.

alexcrichton · 2019-10-21T15:53:24Z

@bors: r=ehuss

bors · 2019-10-21T15:53:26Z

📌 Commit 9cfdf4d has been approved by ehuss

bors · 2019-10-21T15:53:32Z

⌛ Testing commit 9cfdf4d with merge 7a13e3858bde1da98ac44ee8208980c0dce45710...

bors · 2019-10-21T16:34:28Z

💔 Test failed - checks-azure

alexcrichton · 2019-10-21T16:37:31Z

@bors: r=ehuss

bors · 2019-10-21T16:37:32Z

📌 Commit 269624f has been approved by ehuss

Support rustc's `-Z panic-abort-tests` in Cargo Recently added in rust-lang/rust#64158 the `-Z panic-abort-tests` flag to the compiler itself will activate a mode in the `test` crate which enables running tests even if they're compiled with `panic=abort`. It effectively runs a test-per-process. This commit brings the same support to Cargo, adding a `-Z panic-abort-tests` flag to Cargo which allows building tests in `panic=abort` mode. While I wanted to be sure to add support for this in Cargo before we stabilize the flag in `rustc`, I don't actually know how we're going to stabilize this here. Today Cargo will automatically switch test targets to `panic=unwind`, and so if we actually were to stabilize this flag then this configuration would break: [profile.dev] panic = 'abort' In that case tests would be compiled with `panic=unwind` (due to how profiles work today) which would clash with crates also being compiled with `panic=abort`. I'm hopeful though that we can perhaps either figure out a solution for this and maybe even integrate it with the ongoing profiles work.

bors · 2019-10-21T16:37:39Z

⌛ Testing commit 269624f with merge 02e0c39...

bors · 2019-10-21T17:59:49Z

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 02e0c39 to master...

Update cargo 6 commits in 3a9abe3f065554a7fbc59f440df2baba4a6e47ee..3ba5f27170db10af7a92f2b682e049397197b8fa 2019-10-15 15:55:35 +0000 to 2019-10-22 15:05:18 +0000 - Fix typo in `cargo install --profile` help (rust-lang/cargo#7532) - Use stricter -Z flag parsing. (rust-lang/cargo#7531) - Set timestamp on generated files in archive to now (rust-lang/cargo#7523) - Support rustc's `-Z panic-abort-tests` in Cargo (rust-lang/cargo#7460) - rustfmt for nightly changes. (rust-lang/cargo#7526) - Allow --all-features in root of virtual workspace. (rust-lang/cargo#7525)

Remove unused profile support for -Zpanic-abort-tests This removes the vestigial `PanicSetting::Inherit` setting. This was initially introduced in #7460 which added `-Zpanic-abort-tests`. This was needed at the time because `test` and `dev` profiles were separate, but they were inter-mixed when running `cargo test`. That would cause a problem if the unwind/abort settings were mixed. However, with named profiles, `test` now inherits from `dev`, so this is no longer necessary. Now that named profiles are stable, support for the old form is no longer necessary.

rust-highfive assigned Eh2406 Sep 30, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2019

rust-highfive assigned ehuss and unassigned Eh2406 Sep 30, 2019

alexcrichton commented Sep 30, 2019

View reviewed changes

alexcrichton force-pushed the panic-abort-tests branch 2 times, most recently from 4b5087f to 46bf31f Compare October 15, 2019 13:55

alexcrichton force-pushed the panic-abort-tests branch from 46bf31f to c6471ff Compare October 18, 2019 13:40

alexcrichton added 2 commits October 21, 2019 08:51

Inherit panic settings in test/bench profiles

9cfdf4d

We've always ignored the `panic` settings configured in test/bench profiles, so this just continues to ignore them but in a slightly different way.

alexcrichton force-pushed the panic-abort-tests branch from c6471ff to 9cfdf4d Compare October 21, 2019 15:53

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2019

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 21, 2019

Fix broken tests on nightly

269624f

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2019

bors merged commit 269624f into rust-lang:master Oct 21, 2019

bors deleted the panic-abort-tests branch October 21, 2019 17:59

ehuss mentioned this pull request Oct 22, 2019

Update cargo rust-lang/rust#65710

Merged

Mark-Simulacrum mentioned this pull request Dec 27, 2019

panic=abort testing / subprocess testing rust-lang/rust#67650

Open

tmandry mentioned this pull request Aug 14, 2020

Support panic=abort without #[should_panic] from libtest rust-lang/rust#62318

Closed

ehuss added this to the 1.40.0 milestone Feb 6, 2022

ehuss mentioned this pull request Mar 21, 2022

Remove unused profile support for -Zpanic-abort-tests #10495

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support rustc's `-Z panic-abort-tests` in Cargo #7460

Support rustc's `-Z panic-abort-tests` in Cargo #7460

alexcrichton commented Sep 30, 2019

rust-highfive commented Sep 30, 2019

alexcrichton commented Sep 30, 2019

alexcrichton Sep 30, 2019

ehuss commented Oct 4, 2019 •

edited

Loading

alexcrichton commented Oct 15, 2019

ehuss commented Oct 17, 2019

alexcrichton commented Oct 18, 2019

ehuss commented Oct 18, 2019

alexcrichton commented Oct 21, 2019

bors commented Oct 21, 2019

bors commented Oct 21, 2019

bors commented Oct 21, 2019

alexcrichton commented Oct 21, 2019

bors commented Oct 21, 2019

bors commented Oct 21, 2019

bors commented Oct 21, 2019

Support rustc's -Z panic-abort-tests in Cargo #7460

Support rustc's -Z panic-abort-tests in Cargo #7460

Conversation

alexcrichton commented Sep 30, 2019

rust-highfive commented Sep 30, 2019

alexcrichton commented Sep 30, 2019

alexcrichton Sep 30, 2019

Choose a reason for hiding this comment

ehuss commented Oct 4, 2019 • edited Loading

alexcrichton commented Oct 15, 2019

ehuss commented Oct 17, 2019

alexcrichton commented Oct 18, 2019

ehuss commented Oct 18, 2019

alexcrichton commented Oct 21, 2019

bors commented Oct 21, 2019

bors commented Oct 21, 2019

bors commented Oct 21, 2019

alexcrichton commented Oct 21, 2019

bors commented Oct 21, 2019

bors commented Oct 21, 2019

bors commented Oct 21, 2019

Support rustc's `-Z panic-abort-tests` in Cargo #7460

Support rustc's `-Z panic-abort-tests` in Cargo #7460

ehuss commented Oct 4, 2019 •

edited

Loading