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 check,build,rustc --all-targets #4157

Closed
wants to merge 2 commits into from

Conversation

stepancheg
Copy link
Contributor

cargo check does not check all targets by default, and to check
all, you need to call it cargo check --tests --examples --bins --benches. cargo check --all-targets is a shortcut.

--all-targets is added to check, build and to rustc commands.

For consitency --all-targets added to other commands like
test although "all targets" is default behavior.

`cargo check` does not check all targets by default, and to check
all, you need to call it `cargo check --tests --examples --bins
--benches`. `cargo check --all-targets` is a shortcut.

`--all-targets` is also added to `build` and to `rustc` command.

For consitency `--all-targets` added to other commands like `test`
although "all targets" is default behavior.
@rust-highfive
Copy link

r? @alexcrichton

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

@stepancheg stepancheg force-pushed the all-targets branch 2 times, most recently from adda1a6 to 42346b9 Compare June 11, 2017 04:45
@alexcrichton
Copy link
Member

cc #2495, #3431

@matklad do you have thoughts about this feature? I'm sort of ambivalent about it.

@matklad
Copy link
Member

matklad commented Jun 15, 2017

For cargo check in particular I think that this should be the default behavior (and if it is the default, we probably don't need a flag). And probably for build and rustc it is also a good default, but I am not sure that changing default is necessary a good idea at this stage.

However, just adding a flag as in this PR seems fine to me as well, because the ability to build everything is very useful for development. (But for IntelliJ in particular, we can default our build command to cargo check --tests --examples --bins --benches once this flags hit stable).

To sum up: adding flag is fine with me (hope that's the last flag in this area), but what about changing the defaults instead?

@malbarbo
Copy link
Contributor

I think --all-targets is misleading because --target has a unrelated meaning.

@stepancheg
Copy link
Contributor Author

stepancheg commented Jun 16, 2017

@malbarbo that's good point, however, these things are already called "targets":

    --test NAME                  Check only the specified test target
    --tests                      Check all tests
    --bench NAME                 Check only the specified bench target
    --benches                    Check all benches

Any suggestions of a better name?

@behnam
Copy link
Contributor

behnam commented Jun 17, 2017

This is awesome!

Now that you're on this, maybe it's time to update Check only the specified [...] parts in the descriptions (also for other commands) to be more accurate, as these parameters are union'ed, meaning each options "adds" to the list of targets, not limiting it to "only" that target. (See also #3112 (comment)) IMHO, Check the specified [...] is a shorter, more accurate description here, but maybe we can find something even better.

@alexcrichton
Copy link
Member

@matklad I'd actually argue that we don't want this on by default. Especially for build and I'd hazard a guess with check. I personally have never had the desire for "just build everything" but rather I'm always focused on getting something working. I don't mind adding a flag for this, but it doesn't make a lot of sense to me as a default.

I agree with @malbarbo though that the usage of "target" here can be confusing when mixed with cross-compilation flags. The only other thing I can think of, however, is --all-artifacts

@matklad
Copy link
Member

matklad commented Jun 19, 2017

I personally have never had the desire for "just build everything" but rather I'm always focused on getting something working.

@alexcrichton sounds reasonable!

Not sure how to deal with ambiguity about targets: it's a fundamental problem in Cargo's terminology, that crate is called target :(

λ cargo build --help | grep target
    --test NAME                  Build only the specified test target
    --bench NAME                 Build only the specified benchmark target
    --target TRIPLE              Build for the target triple

I am not sure that adding artifacts to the mix will be better then sticking with target...

@Xanewok
Copy link
Member

Xanewok commented Jul 3, 2017

Recently I encountered the same problem with how to call those crate 'targets'. I assume using "crate type" isn't good, since it's mostly used for output type like bin, lib, rlib etc.? Maybe something along the lines of "crate build type" or "crate target" to disambiguate?

@alexcrichton
Copy link
Member

I personally think we should stick with --all-targets if we do this. It's unfortuante, yes, but there seem to be few other suggestions.

@stepancheg
Copy link
Contributor Author

Another option is to rename it to --all-build-targets. And change comment of --target to "Build for the platform target triple".

@alexcrichton
Copy link
Member

That also seems plausible to me, I'm fine either way

@bors
Copy link
Contributor

bors commented Jul 5, 2017

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

@behnam
Copy link
Contributor

behnam commented Jul 5, 2017

Another option is to rename it to --all-build-targets. And change comment of --target to "Build for the platform target triple".

My two cents: in most other compiler/build-systems "build target" almost always means the target platform. Here, it's more of a "target package" or "target crate".

@matklad
Copy link
Member

matklad commented Jul 5, 2017

@behnam yeah, I think what Cargo calls a "target" is usually known as an "artifact". But we probably can't replace existing Cargo terminology, especially for this single issue, which just exemplifies the general problem.

👍 to sticking with --all-targets. If we ever need all target triples, we could use --all-target-triples

@behnam
Copy link
Contributor

behnam commented Jul 5, 2017

Yeah, --all-targets can definitely work.

At some point, I was thinking that --all could be a synonym for --all-packages, and what's being added here could also be --all-package-crates, --all-package-targets, or even --all-crates, for short, since from the compiler's perspective, these are called crates, and from Cargo perspective, targets.

@alexcrichton
Copy link
Member

@stepancheg would you be willing to rebase this to land?

behnam pushed a commit to behnam/rust-cargo that referenced this pull request Aug 11, 2017
`cargo check` does not check all targets by default, and to check all,
you need to call it `cargo check --tests --examples --bins --benches`.

Here, we implement `--all-targets` For `check`, `build`, and `rustc`.

For consitency, `--all-targets` is also added to other commands like
`test` although "all targets" is the default behavior.

This is a rebase of <rust-lang#4157>
bors added a commit that referenced this pull request Aug 17, 2017
[check|build|rustc] Add --all-targets option

`cargo check` does not check all targets by default, and to check all,
you need to call it `cargo check --tests --examples --bins --benches`.

Here, we implement `--all-targets` For `check`, `build`, and `rustc`.

For consitency, `--all-targets` is also added to other commands like
`test` although "all targets" is the default behavior.

This is a rebase of <#4157>
@alexcrichton
Copy link
Member

Closing in favor of #4400

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.

8 participants