feat(profile): panic behavior can be specified for custom harness

[weihanglo]

What does this PR try to resolve?

It's libtest that requires tests to be always unwound.
For custom harnesses they don't need such a requirement.
Cargo relaxes it a bit in profile settings.

Fixes #11214

How should we test and review this PR?

First, is this change what we want? IMO it is unnecessary to restrict the panic behaviour of a custom harness.

Additional information

This error message¹ will become a lie if a user sets panic in test/bench profile with a custom harness. I am not sure what to do. We can pass a flag in to inform TomlProfile there is some custom harness, though I feel it is not worth doing that, as TomlProfile should only know about information from itself. It might also touch too many of functions if we want to provide such an accurate message.

Maybe we can tweak the message a bit, such as

`panic` setting is generally ignored for `{}` profile. It only works for custom harnesses

Mind that if you set panic=abort in profile dev/release, the warning won't show. However, the panic behaviour is still not applied at all for your cargo test units.

Footnotes

1. https://github.com/rust-lang/cargo/blob/b1b25a025565fd71dfa2db4a24f440ab4d32fc6c/src/cargo/util/toml/mod.rs#L642 ↩

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[weihanglo]

It failed on Windows with an exit code other than 101 (STATUS_STACK_BUFFER_OVERRUN). I've tried a simple project with a custom harness, but had no luck to reproduce it…

Should we ignore Windows or assert with this status code?

https://github.com/rust-lang/cargo/actions/runs/3303170253/jobs/5450785138#step:10:3026

[ehuss]

I think it would be ok to use cfg!(windows) to choose the STATUS_ACCESS_VIOLATION exit code instead of 101.

[ehuss]

I've been thinking about this for a while. Some thoughts:

Backwards compatibility

I'm a little concerned about the scenario where someone has a custom test harness that relies on the ability to unwind panics (the same limitation as libtest). This change would make that not operate quite correctly. It's hard to predict if there are any users with that scenario (I did not see any on crates.io).

I think the workaround is for the user to specify profile.test.panic = "unwind", right?

I'm having a hard time deciding to make the call on whether or not this change would be acceptable. It may be very likely that nobody is relying on this, but that seems really hard to predict. Also, the workaround may be very simple, which may be sufficient?

My only other idea would be to have a warning period, but I'm not sure how to do that in a nice way. We could detect if there is a harness=false test, then require the user to explicitly set profile.test.panic to whichever value they want (and disable the warning if it is explicitly set). I dunno if that would help, or is overkill?

Unused warning

I think it would be fine to change the implementation of the `panic` setting is ignored warning. It doesn't have to be perfect. I think I would:

1. Add a function somewhere after the UnitGraph has been computed that will do some validation. I would probably stick it somewhere in Context::compile (similar to check_collisions), though the exact location probably isn't too important.
2. Call bcx.profiles.base_profile() and see if the panic == Abort. If not, return.
3. The function would iterate over all the Units and see if any are CompileMode::Test and Target.harness() == true. If none are, return.
4. If both of the above conditions are true, display a warning along the lines of:

warning: profile setting panic = "abort" is ignored for tests and benchmarks using the standard harness
    Consider setting profile.{requested_profile}.panic = "unwind" to remove this warning.

If that is too much trouble, or too complex, or too much of a false-positive, I would consider just removing the warning altogether. I'm not sure it is all that valuable. Also, hopefully some day panic-abort-tests will be stabilized and all this can go away.

[weihanglo]

Re: Backwards compatibility

It sounds great to have a warning period. However, it's hard to give a "right" suggestion, as we don't know the intent of users even they explicitly set but rely on the ignore behaviour. Not to mention that the user can use --profile to choose an arbitrary profile to use.

Warning when harness=false seems fine, but it might be seen by a large portion of non-target custom harness users. It may end up annoying and confusing since not everyone clearly knows what's going on panic strategy.

Maybe we should only warn those also setting panic = "abort". Tell them this gonna work in a few releases later? That group of people are the only one being affected IIUC.

Re: Unused warning

It doesn't sound too complex to implement, but does introduce some of a false-positive. I lean to removing the existing warning.

[bors]

☔ The latest upstream changes (presumably #11445) made this pull request unmergeable. Please resolve the merge conflicts.

[weihanglo]

Rebased due to the refactor of cargo_compile/mod.rs

[weihanglo]

Hi @ehuss. What is the status of this? Not urgent though 😉.

[bors]

☔ The latest upstream changes (presumably #12768) made this pull request unmergeable. Please resolve the merge conflicts.

[weihanglo]

@zeroflaw

Are you testing your project with this pull request? Could you provide a minimal reproducible example so that people can look into that?
(Please open a new issue if it is not tested with this PR)

[bors]

☔ The latest upstream changes (presumably #14128) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

☔ The latest upstream changes (possibly 081d7ba) made this pull request unmergeable. Please resolve the merge conflicts.