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

Cannot run benchmarks with panic = "abort" #3166

Closed
AaronFriel opened this issue Oct 5, 2016 · 11 comments · Fixed by #3175
Closed

Cannot run benchmarks with panic = "abort" #3166

AaronFriel opened this issue Oct 5, 2016 · 11 comments · Fixed by #3175

Comments

@AaronFriel
Copy link

AaronFriel commented Oct 5, 2016

The following error is obtained when running cargo bench, with the a panic = "abort" option in cargo.toml:

error: the linked panic runtime `panic_unwind` is not compiled with this crate's panic strategy `abort`

The cargo.toml needs to set the panic behavior in [profile.bench], e.g.:

cargo.toml:

[package]
name = "foobar"
version = "0.1.0"
authors = ["Aaron Friel <mayreply@aaronfriel.com>"]

[dependencies]
clap="^2.13.0"

[profile.bench]
panic = "abort"
@alexcrichton
Copy link
Member

Dug a bit more into this today. The dependency actually isn't needed here to trigger the bug, and if you configure profile.test with abort then cargo test is also broken.

The problem here is that libtest, Rust's testing framework, requires the unwind panic mode and is incompatible with panic = "abort". If you configure profile.dev to have panic = "abort", it actually compiles all dependencies with -Cpanic=unwind when you execute cargo test to satisfy this constraint.

The "fix" here would be to ignore panic = "abort" in the bench and test profiles.

@AaronFriel
Copy link
Author

The patch in #3175 is confusing to me. Is there any way to run tests or benchmarks with panic="abort"?

@alexcrichton
Copy link
Member

Unfortunately no because they all link to libtest which is compiled with panic=unwind :(

@AaronFriel
Copy link
Author

I see, and because libtest depends on panics to observe assert! failures, that's the blocking issue?

Is there any work on making an alternative libtest based on Result<>, with an, e.g.: try_assert! that works like try!?

@alexcrichton
Copy link
Member

Perhaps yeah, but that's more of a rust-lang/rust issue than a Cargo one.

@AaronFriel
Copy link
Author

Interesting. If I were to make an alternative libtest, is there a way to specify that in cargo.toml? Or are tests (and their return types) part of rust-lang/rust?

@alexcrichton
Copy link
Member

Yeah you can specify harness = false in which case Cargo will just compile the test as an executable, and then you can link to whatever

alexcrichton added a commit to alexcrichton/cargo that referenced this issue Nov 2, 2016
Both of these profiles link to libtest, so it's invalid to configure them with
`panic="abort"`. To prevent confusing errors just ignore the configuration for
now.

Closes rust-lang#3166
bors added a commit that referenced this issue Nov 3, 2016
Ignore `panic` configuration for test/bench profiles

Both of these profiles link to libtest, so it's invalid to configure them with
`panic="abort"`. To prevent confusing errors just ignore the configuration for
now.

Closes #3166
@bors bors closed this as completed in #3175 Nov 3, 2016
@sanmai-NL
Copy link

@alexcrichton:
Thanks for fixing the issue in 4a73741 and explaining the test harness design in this thread. I think the issue hasn't been fully resolved though. I had a problem similar to the reporter and I had to find out what was happening from this thread. Can the limitation on panic strategy please be shown as a warning? That would be way less confusing than Cargo silently ignoring the panic profile field.

@alexcrichton
Copy link
Member

@sanmai-NL seems reasonable to me! Want to open an issue for that?

@sanmai-NL
Copy link

My first reaction to your suggestion is that it would be too much overhead. I can try to make a PR for it straight away. If it doesn't cost you too much time, perhaps you can give some pointers on how to generate an appropriate warning in Cargo, e.g. similar to the current ‘Unknown field’ warning.

@alexcrichton
Copy link
Member

Sure yeah sounds good! The best way to generate warnings right now is the add_warning method (called throughout that file). I suspect that this logic would go somewhere in toml.rs.

If that doesn't end up working out though, just let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants