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

Consistently default to --lib --bins when --all-targets is not given #5146

Closed
wants to merge 6 commits into from

Conversation

infinity0
Copy link
Contributor

Fixes #5134

cargo build --help says:

    --all-targets                Build all targets (lib and bin targets by default)

but the old default behaviour was actually doing --all-targets. This meant that
--avoid-dev-deps was only effectively if one gave --lib --bins explicitly. With
this commit, `cargo build --avoid-dev-deps` with no other flags, will build lib
and bins and avoid installing dev-dependencies.
@rust-highfive
Copy link

r? @matklad

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

@infinity0
Copy link
Contributor Author

r? @alexcrichton based on #4400

cc @behnam

@rust-highfive rust-highfive assigned alexcrichton and unassigned matklad Mar 7, 2018
@alexcrichton
Copy link
Member

Thanks for the PR! I do think though that cargo check --all-targets, for example, checks all benchmarks/tests/etc today. It still does with this patch, right?

I also think that we'd probably want to fix the behavior with cargo {bench,test} rather than tweak the defaults, as it's intended to "run all tests" and "run all benchmarks" I believe?

@bors
Copy link
Contributor

bors commented Mar 13, 2018

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

@infinity0
Copy link
Contributor Author

Yes, there is a test that cargo check --all-targets checks all lib/bin/example/test/bench targets.

After resolving conflicts and fixing a minor bug from #5152, I've additionally added a commit that makes cargo {bench,test} applies to all targets (and then the --all-targets flag is redundant), as requested.

@infinity0
Copy link
Contributor Author

The tests are failing but I'm pretty sure it's not a problem with my code. If you manually create the example from build::explicit_examples you can see that --all-targets is already a bit screwed even in older cargo releases:

$ cargo --version
cargo 0.24.0
$ cargo clean && cargo test >/dev/null 2>/dev/null; target/debug/examples/hello
Hello, World!
$ cargo clean && cargo test --all-targets >/dev/null 2>/dev/null; target/debug/examples/hello

running 0 tests

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

I'll dig further, hints welcome.

@infinity0
Copy link
Contributor Author

infinity0 commented Mar 14, 2018

It looks like cargo build --examples vs cargo test --examples will each respectively overwrite target/debug/examples/goodbye with the example binary vs a test binary that tests the example file.

Currently many of cargo's own tests implicitly depend on the fact that invocations like cargo test -v do not build the test-for-the-example but instead builds the example binary, for example build::explicit_examples. If my latest commit 1d31d9f is reverted/removed from this PR then the tests will start passing again.

Alternatively, it may be possible to switch cargo {bench,test} to "all-targets by default" without changing these other tests, by changing the target path of the test-for-the-example to be something other than target/debug/examples/goodbye. However this is also fairly invasive, was not the original intention of my PR, and I'd rather not get dragged into a rabbit hole being forced to fix existing deficiencies of cargo that are unrelated to the issue I was trying to fix. I could file an issue for this however.

TL;DR I'd suggest dropping 1d31d9f from this PR and abandoning the current suggestion to have cargo {bench,test} applied to all-targets by default, but file an issue about it detailing what the problem is and how to fix it (i.e. either rewrite several-dozen tests, or rewrite the target-paths for test binaries).

@bors
Copy link
Contributor

bors commented Mar 15, 2018

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

@infinity0
Copy link
Contributor Author

There was a simpler way of achieving what I wanted and I misunderstood how the existing code handles --all-targets. I'll open another more minimal PR.

@infinity0 infinity0 closed this Mar 15, 2018
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 this pull request may close these issues.

5 participants