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 build --all-targets generates two copies of crate test binary #4929

Closed
rocallahan opened this issue Jan 10, 2018 · 17 comments · Fixed by #6309
Closed

cargo build --all-targets generates two copies of crate test binary #4929

rocallahan opened this issue Jan 10, 2018 · 17 comments · Fixed by #6309

Comments

@rocallahan
Copy link

Example:

[roc@glory ~]$ cargo new foobar
     Created library `foobar` project
[roc@glory ~]$ cd foobar/
[roc@glory foobar]$ cargo build --all-targets
   Compiling foobar v0.1.0 (file:///home/roc/foobar)
    Finished dev [unoptimized + debuginfo] target(s) in 0.45 secs
[roc@glory foobar]$ ls target/debug/
build  examples                 foobar-149b726de0cb9a4e.d  foobar-d74fc8c2632ddb7a.d  libfoobar.d     native
deps   foobar-149b726de0cb9a4e  foobar-d74fc8c2632ddb7a    incremental                libfoobar.rlib
[roc@glory foobar]$ target/debug/foobar-149b726de0cb9a4e 

running 1 test
test tests::it_works ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

[roc@glory foobar]$ target/debug/foobar-d74fc8c2632ddb7a 

running 1 test
test tests::it_works ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

[roc@glory foobar]$ 

I don't know why it generates two copies of the crate test binary. That seems unnecessary.

@mbrubeck
Copy link
Contributor

I suspect it is compiling the library once with the bench profile and once with the test profile. The cargo bench and cargo test commands will each run these, respectively.

@rocallahan
Copy link
Author

I see. Although that might be useful in theory, in practice it's a problem because you can't tell which binary is which.

@alexcrichton
Copy link
Member

Aha it does indeed look like @mbrubeck is right! If you build with cargo build --all-targets -v you get:

     Running `rustc --crate-name wut src/lib.rs --emit=dep-info,link -C opt-level=3 --test -C metadata=b42b4f89592c34b9 -C extra-filename=-b42b4f89592c34b9 --out-dir /home/alex/code/wut/target/debug/deps -L dependency=/home/alex/code/wut/target/debug/deps`
     Running `rustc --crate-name wut src/lib.rs --emit=dep-info,link -C debuginfo=2 --test -C metadata=f3899aa23d6f99ad -C extra-filename=-f3899aa23d6f99ad --out-dir /home/alex/code/wut/target/debug/deps -L dependency=/home/alex/code/wut/target/debug/deps`

(note the -C opt-level=3 in one and -C debuginfo=2 in the other)

I agree this is surprising though! I don't think we should build benchmarks unless --release is passed.

@rocallahan
Copy link
Author

I would have assumed that if I wanted to run benches on optimized code I would do cargo build --release --all-targets.

@rocallahan
Copy link
Author

Building a non-debuginfo optimized build into target/debug seems counterintuitive.

@lukaslueg
Copy link
Contributor

lukaslueg commented Jan 10, 2018

I'm looking at modifying ops::CompileFilter::new() to also take a release-bool, so all_targets would still mean FilterRule::All for benches if release is true but FilterRule::Just([]) if release is false, effectivly ignoring benchmarks if --all-targets is passed without --release; vice versa for tests.

Do we really want not to build tests and benchmarks in debug-mode? People may want debug-builds of their benchmarks from --all-targets in order to investigate a problem. all-targets used to mean "all the targets".

@alexcrichton
Copy link
Member

@rust-lang/cargo may wish to weigh in? To me --all-targets doesn't really make much sense because you almost never actually want all the targets as we'll add more over time and it's never everything. If we change the precise meaning of --all-targets over time that seems fine by me, too.

@ehuss
Copy link
Contributor

ehuss commented Jan 10, 2018

The fact that the bench target is written to the "debug" directory is initially surprising to me (this also happens with --benches and --bench). The --release flag seems to control which directory things will be sent to independent of the target's Profile. Would it make sense to change that? Particularly in light of rust-lang/rfcs#2282 which would put custom profiles in their own directory, it might be simpler if the Profile drives the output directory instead of having various special cases.

My intent when I changed it was that --all-targets really means "all targets" so that one can verify that everything compiles the same as-if you did everything individually. I think it would be confusing if --all-targets meant "some targets".

@lukaslueg
Copy link
Contributor

I implemented filtering out tests in --release and benchmarks without --release if --all-targets is given. By medium of failing tests I noticed that cargo check --all-targets is the strongest argument that --all-targets should mean "all targets": CompileFilter does not know about check and having cargo check --all-targets NOT check benchmarks while cargo check --all-targets --release NOT checking tests seems stupid; we could improve filtering targets based on the caller's intention but there are already a lot of hoops around target selection.
While @alexcrichton is rightfully surprised that benchmarks are built by cargo build --all-targets, this behavior is mostly silent and not harmful. Adding more knobs is verbose and confusing. FWIW, I concur with @ehuss regarding path selection, while --all-targets should continue to mean "both barrels".

@rocallahan
Copy link
Author

rocallahan commented Jan 10, 2018

I think it would make sense for cargo build --all-targets to build benches in debug mode and leave them in target/debug. Just like it would make sense for cargo build --all-targets --release to build tests in optimized mode and leave them in target/release. The latter is definitely desired so we can verify that tests pass with optimizations enabled. The former is useful so you can debug bugs in the benches.

My problem with the current situation is that currently cargo build --all-targets builds two binaries, both of which run tests when executed. One of them is built optimized, but I don't want to run tests optimized; if I did, I would have asked for it with --release. Whatever else is decided, this seems wasteful. If you decide that the current behavior is internally logical, is there a workaround I can use to build all targets in debug mode, with only one binary containing the tests?

I think a bigger problem is that apparently despite having used Rust for two years my mental model for cargo is wrong. I had assumed debug/release map to profiles, which are a specific set of compiler options, and build-targets are orthogonal to profiles. But apparently that's not true...

@rocallahan
Copy link
Author

To me --all-targets doesn't really make much sense because you almost never actually want all the targets as we'll add more over time and it's never everything.

It makes perfect sense for cargo check. I don't think I would ever want cargo check without --all-targets --- we don't ever want to break compiling some build target, even ones we're not using.

I think cargo build --all-targets also makes sense for CI. I definitely want all targets to at least be checked by CI. We could have CI do cargo check --all-targets followed by cargo build of the build targets we actually want to run, but it's simpler to just cargo build --all-targets and get everything done in one pass and get some extra checking against code generation ICEs and whatnot.

@rocallahan
Copy link
Author

Then again, take that with a grain of salt since perhaps I have no idea what a build target actually is.

I found https://doc.rust-lang.org/cargo/reference/manifest.html#the-profile-sections which explains some of my misunderstanding. Sorry about not finding that earlier. However, that page nowhere mentions "build target", so it's still unclear exactly what a build target is or how they interact with profiles.

I'm particularly confused about how --all-targets and --release interact with each other and with profiles. If --all-targets meant "build with all profiles" that would make sense, but then --release would have no effect, so it doesn't mean that. Yet clearly --all-targets does build with more than one profile, since it's building debug and optimized binaries with --release not specified.

Another thing that confuses me: I had thought of "tests" and "examples" as both being a kind of build target, but building tests is controlled by a flag in the profile, while apparently building examples are not. So whether examples are built is orthogonal to the profile, but whether tests are built is not?

@ehuss
Copy link
Contributor

ehuss commented Jan 11, 2018

that page nowhere mentions "build target", so it's still unclear exactly what a build target is or how they interact with profiles.

Yea, I think it would be nice if there was an introduction that explained and defined these different terms (when I first started learning Rust I was confused by what "crate" meant, and it still doesn't seem to be defined anywhere). Configuring a target sort of explains targets.

I'm particularly confused about how --all-targets and --release interact with each other

--all-targets builds all the targets using the default profiles that would normally be used by those targets.

Perhaps this table will clarify which profile is used based on whether or not --release is used:

Command --lib/--bin/--example --test --bench
build dev test bench
build --release release bench bench
test test test test
test --release bench bench bench
bench bench bench bench

Another point of possible confusion is that lib.rs is a "lib" target, a "test" target (for unit-tests), and a "bench" target.

Another thing that confuses me: I had thought of "tests" and "examples" as both being a kind of build target, but building tests is controlled by a flag in the profile, while apparently building examples are not. So whether examples are built is orthogonal to the profile, but whether tests are built is not?

Perhaps it is clearer to think of each thing that gets built as a pair (target, profile). When building a regular example, it uses the dev profile. If you do cargo test --example ex1, it will build the example with the test profile. Which targets get built is unrelated to the profile selection.

Let me know if you have any questions. I'm not sure if what I wrote helps. You're not the only one confused by this. @matklad in particular is a proponent of simplifying the profile selection.

@alexcrichton
Copy link
Member

FWIW historically the implementation of targets/profiles in Cargo has been pretty muddied and has had lots of subtle bugs, so I'ms orry if that's cause any confusion! Cargo itself is not doing a great job of making the distinctions here which is the root many issues.

I think building benchmarks in debug mode makes sense with cargo build --all-targets.

@stale
Copy link

stale bot commented Sep 17, 2018

As there hasn't been any activity here in over 6 months I've marked this as stale and if no further activity happens for 7 days I will close it.

I'm a bot so this may be in error! If this issue should remain open, could someone (the author, a team member, or any interested party) please comment to that effect?

The team would be especially grateful if such a comment included details such as:

  • Is this still relevant?
  • If so, what is blocking it?
  • Is it known what could be done to help move this forward?

Thank you for contributing!

(The cargo team is currently evaluating the use of Stale bot, and using #6035 as the tracking issue to gather feedback.)

If you're reading this comment from the distant future, fear not if this was closed automatically. If you believe it's still an issue please leave a comment and a team member can reopen this issue. Opening a new issue is also acceptable!

@stale stale bot added the stale label Sep 17, 2018
@dwijnand
Copy link
Member

Did #6039 fix this? Do you know, @ehuss?

@stale stale bot removed the stale label Sep 18, 2018
@ehuss
Copy link
Contributor

ehuss commented Sep 18, 2018

No. What needs to change isn't exactly clear.

  • Should --all-targets really build "all targets"? I personally think it should, otherwise it seems confusing to me.
  • Which profile should benchmarks use with build if --release is not specified? Currently they use "bench". They could be switched to "test" by default. This would cause the benchmarks to be de-duped with tests (since they would then share the same profile) and cause only one test binary to be produced (thus addressing the original concern that two test binaries are made). Using --release would result in using the "bench" profile. Changing these things might break some existing users. This seems like a decent option to me, and unleashes the ability to debug benchmarks.
  • There's a side issue where currently these benchmarks are placed in the "debug" target folder which is the wrong location (or is at least confusing). Changing the previous point would fix this.

bors added a commit that referenced this issue Nov 14, 2018
Use "test" profile for `cargo build` benchmarks.

When using `cargo build` (without `--release`), build benchmarks using the "test" profile. This was causing some confusion where the benchmark is placed in the `target/debug` directory, and also causing some duplicates that may not be expected. It also makes it easier to debug benchmarks (previously you had to edit the `[profile.bench]` profile).

Closes #5575, closes #6301, closes #4240, closes #4929.
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.

6 participants