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

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

Merged
merged 3 commits into from
Oct 21, 2019

Conversation

alexcrichton
Copy link
Member

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
Copy link

r? @Eh2406

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

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

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned Eh2406 Sep 30, 2019
@@ -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() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor

ehuss commented Oct 4, 2019

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 alexcrichton force-pushed the panic-abort-tests branch 2 times, most recently from 4b5087f to 46bf31f Compare October 15, 2019 13:55
@alexcrichton
Copy link
Member Author

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
Copy link
Contributor

ehuss commented Oct 17, 2019

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
Copy link
Member Author

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
Copy link
Contributor

ehuss commented Oct 18, 2019

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
Copy link
Member Author

@bors: r=ehuss

@bors
Copy link
Contributor

bors commented Oct 21, 2019

📌 Commit 9cfdf4d has been approved by ehuss

@bors 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
Copy link
Contributor

bors commented Oct 21, 2019

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

@bors
Copy link
Contributor

bors commented Oct 21, 2019

💔 Test failed - checks-azure

@bors 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
@alexcrichton
Copy link
Member Author

@bors: r=ehuss

@bors
Copy link
Contributor

bors commented Oct 21, 2019

📌 Commit 269624f has been approved by ehuss

@bors 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 a commit that referenced this pull request Oct 21, 2019
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
Copy link
Contributor

bors commented Oct 21, 2019

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

@bors
Copy link
Contributor

bors commented Oct 21, 2019

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

@bors bors merged commit 269624f into rust-lang:master Oct 21, 2019
@bors bors deleted the panic-abort-tests branch October 21, 2019 17:59
Centril added a commit to Centril/rust that referenced this pull request Oct 24, 2019
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)
Centril added a commit to Centril/rust that referenced this pull request Oct 24, 2019
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)
@ehuss ehuss added this to the 1.40.0 milestone Feb 6, 2022
bors added a commit that referenced this pull request Mar 25, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants