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

cargo test builds binaries without panic #5435

Closed
ehuss opened this issue Apr 28, 2018 · 1 comment · Fixed by #5513
Closed

cargo test builds binaries without panic #5435

ehuss opened this issue Apr 28, 2018 · 1 comment · Fixed by #5513

Comments

@ehuss
Copy link
Contributor

ehuss commented Apr 28, 2018

If you have panic="abort" set in the dev profile and run cargo test, the binaries (and examples) are built without panic set (assuming you have an integration test which triggers the inclusion of binaries). This doesn't seem correct to me.

Repro:

cargo new --lib foo
cd foo

cat > src/main.rs <<EOL
extern crate foo;
fn main() { foo::f(); }
EOL

echo "pub fn f() { panic!(\"xxx\"); }" >> src/lib.rs

mkdir tests
touch tests/t1.rs

cat >> Cargo.toml <<EOL
[profile.dev]
panic = "abort"
EOL

cargo test -v
target/debug/foo   # <- NOTE this does not abort!

Fixing this is not entirely simple. If you build the lib.rs dependency twice (once with panic, once without), then rustdoc (for doctests) will pick up both and fail to run. The code that collects the libraries to link (in Compilation.libraries) would probably need to be adjusted to collect only the libs that are needed for doctests.

I can tackle this if you'd like. One idea I had to approach this would be:

  • Fix doctests to not be over-eager in what it links with:
    • Ensure a Doctest unit is always created. (This might be helpful for fixing other bugs, such as cargo test --doc overriding all other command-line options.)
    • In run_doc_tests, instead of iterating over Compilation.libraries, grab the actual dependencies of the Doctest unit.
  • Remove the global test flag from BuildConfig.

I also wanted to double-check. This would result in lib.rs being compiled three times with cargo test when you have panic set (once with panic for bins, once without for tests, and once as a unit-test). Is that OK?

@alexcrichton
Copy link
Member

Yeah this is one of those things I'm not entirely sure about. On one hand this definitely seems like a bug and would possibly be good to fix. On the other hand though it's really subtle and unlikely to really bring up any otherwise existing bugs (and can also increase compile times as we have to compile the library twice).

I think I'd be somewhat tempted to lean towards fixing this though if we can. It may be surprising to users using cargo test to compile everything twice, but there's unfortunately not a lot we can do about that :(

ehuss added a commit to ehuss/cargo that referenced this issue May 10, 2018
Fixes rust-lang#5435

Also re-enable tests now that rust-lang#5460 has landed.
bors added a commit that referenced this issue May 10, 2018
Fix `panic` for binaries built during tests.

Fixes #5435

Also re-enable tests now that #5460 has landed.
ehuss added a commit to ehuss/cargo that referenced this issue Jun 26, 2018
Fixes rust-lang#5650.  cc rust-lang#5435

As part of my recent work on profiles, I introduced some situations where a
library can be compiled multiple times with different settings.  Doctests were
greedily grabbing all dependencies for a package, regardless of which target
is was for.  This can cause doctests to fail if it links multiple copies of
the same library.

One way to trigger this is `cargo test --release` if you have dependencies, a
build script, and `panic="abort"`.  There are other (more obscure) ways to
trigger it with profile overrides.
bors added a commit that referenced this issue Jul 5, 2018
Fix doctests linking too many libs.

Fixes #5650.  cc #5435

As part of my recent work on profiles, I introduced some situations where a
library can be compiled multiple times with different settings.  Doctests were
greedily grabbing all dependencies for a package, regardless of which target
is was for.  This can cause doctests to fail if it links multiple copies of
the same library.

One way to trigger this is `cargo test --release` if you have dependencies, a
build script, and `panic="abort"`.  There are other (more obscure) ways to
trigger it with profile overrides.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants