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

Add --exclude flag #4031

Merged
merged 5 commits into from May 28, 2017

Conversation

Projects
None yet
6 participants
@torkleyy
Contributor

torkleyy commented May 11, 2017

Allows to exclude packages in conjunction
with --all.

Addresses #2878

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive May 11, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

rust-highfive commented May 11, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@torkleyy

First contribution, would really appreciate if somebody could answer my questions.

Show outdated Hide outdated src/bin/build.rs
let spec = match (options.flag_all, &options.flag_exclude) {
(true, exclude) if exclude.is_empty() => Packages::All,
(true, exclude) => Packages::OptOut(exclude),
(false, exclude) if !exclude.is_empty() => panic!("--exclude can only be used together \

This comment has been minimized.

@torkleyy

torkleyy May 11, 2017

Contributor

How would I return a proper error here? More specifically, should I build the CargoError myself or is there some convenience function? Can I use the bail! macro from there?

@torkleyy

torkleyy May 11, 2017

Contributor

How would I return a proper error here? More specifically, should I build the CargoError myself or is there some convenience function? Can I use the bail! macro from there?

This comment has been minimized.

@matklad

matklad May 12, 2017

Member

Yep, bail! is for this use case:

let cfg = match color {
Some("auto") => Auto,
Some("always") => Always,
Some("never") => Never,
None => Auto,
Some(arg) => bail!("argument for --color must be auto, always, or \
never, but found `{}`", arg),
};

@matklad

matklad May 12, 2017

Member

Yep, bail! is for this use case:

let cfg = match color {
Some("auto") => Auto,
Some("always") => Always,
Some("never") => Never,
None => Auto,
Some(arg) => bail!("argument for --color must be auto, always, or \
never, but found `{}`", arg),
};

Show outdated Hide outdated src/bin/build.rs
Packages::All
} else {
Packages::Packages(&options.flag_package)
let spec = match (options.flag_all, &options.flag_exclude) {

This comment has been minimized.

@torkleyy

torkleyy May 11, 2017

Contributor

This match would be used in many subcommands. May I place it in cargo::util?

@torkleyy

torkleyy May 11, 2017

Contributor

This match would be used in many subcommands. May I place it in cargo::util?

This comment has been minimized.

@matklad

matklad May 12, 2017

Member

You could add Packages::from_flags(flag_all, flag_exclude) -> CargoResult<Packeges>.

@matklad

matklad May 12, 2017

Member

You could add Packages::from_flags(flag_all, flag_exclude) -> CargoResult<Packeges>.

@@ -43,6 +44,7 @@ Options:
-h, --help Print this message
-p SPEC, --package SPEC ... Package to build
--all Build all packages in the workspace
--exclude SPEC ... Exclude packages from the build

This comment has been minimized.

@torkleyy

torkleyy May 11, 2017

Contributor

It seems pretty verbose to manually handle conflicts / requirements of flags. Is there a way to let docopt do this? There is currently sort of a bug when executing cargo build -p whatever --all: The -p is silently ignored.

@torkleyy

torkleyy May 11, 2017

Contributor

It seems pretty verbose to manually handle conflicts / requirements of flags. Is there a way to let docopt do this? There is currently sort of a bug when executing cargo build -p whatever --all: The -p is silently ignored.

This comment has been minimized.

@matklad

matklad May 12, 2017

Member

Is there a way to let docopt do this?

Basically we need to switch to clap, and hopefully get rid of a lot of duplication along the way. But this is a hard task :)

@matklad

matklad May 12, 2017

Member

Is there a way to let docopt do this?

Basically we need to switch to clap, and hopefully get rid of a lot of duplication along the way. But this is a hard task :)

This comment has been minimized.

@torkleyy

torkleyy May 12, 2017

Contributor

Basically we need to switch to clap

Oh it is desired to switch to clap? It's good to hear that! Is there someone working on it already? If not, I'd love to do it.

@torkleyy

torkleyy May 12, 2017

Contributor

Basically we need to switch to clap

Oh it is desired to switch to clap? It's good to hear that! Is there someone working on it already? If not, I'd love to do it.

This comment has been minimized.

@matklad

matklad May 12, 2017

Member

Is there someone working on it already?

I've tried to do that, but haven't arrived at something very satisfying :)

It would be super if you would be able to pull this off!

@matklad

matklad May 12, 2017

Member

Is there someone working on it already?

I've tried to do that, but haven't arrived at something very satisfying :)

It would be super if you would be able to pull this off!

@torkleyy torkleyy changed the title from [WIP] Add --exclude flag to Add --exclude flag May 12, 2017

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 12, 2017

Contributor

I'd say this is ready for review.

Contributor

torkleyy commented May 12, 2017

I'd say this is ready for review.

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 15, 2017

Member

@alexcrichton what do you think about it? I feel that we definitely need something like this, but I am worried that we will have three flags (-pkg, -all, -exclude) to control this behavior.

Hm, maybe we want --pkg foo, --pkg -foo, --pkg *?

Member

matklad commented May 15, 2017

@alexcrichton what do you think about it? I feel that we definitely need something like this, but I am worried that we will have three flags (-pkg, -all, -exclude) to control this behavior.

Hm, maybe we want --pkg foo, --pkg -foo, --pkg *?

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 15, 2017

Contributor

@matklad is "-foo" allowed as argument? If that's the case, I agree that it makes more sense to only have one flag for all of them. Just used --exclude because it was mentionend in that issue.

Contributor

torkleyy commented May 15, 2017

@matklad is "-foo" allowed as argument? If that's the case, I agree that it makes more sense to only have one flag for all of them. Just used --exclude because it was mentionend in that issue.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 15, 2017

Member

Thanks for the PR @torkleyy! This seems fine to me, @matklad we've got enough flags at this point for "target selection" that it may make sense to basically house their documentation elsewhere at some point. Do you think canonicalizing in one --pkg argument with some syntax is better though? I feel like apart from docs --exclude at least is more self-explanatory than --pkg -foo.

Member

alexcrichton commented May 15, 2017

Thanks for the PR @torkleyy! This seems fine to me, @matklad we've got enough flags at this point for "target selection" that it may make sense to basically house their documentation elsewhere at some point. Do you think canonicalizing in one --pkg argument with some syntax is better though? I feel like apart from docs --exclude at least is more self-explanatory than --pkg -foo.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 16, 2017

Contributor

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

Contributor

bors commented May 16, 2017

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

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 16, 2017

Contributor

Rebased

Contributor

torkleyy commented May 16, 2017

Rebased

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 16, 2017

Member

I feel like apart from docs --exclude at least is more self-explanatory than --pkg -foo.

Yeah, --exclude is good enough. It's not obvious though what exactly are you excluding:

cargo test --all --exclude baz

Are we excluding baz package or baz module here?

Another option for flag name is --exclude--pkg.

Member

matklad commented May 16, 2017

I feel like apart from docs --exclude at least is more self-explanatory than --pkg -foo.

Yeah, --exclude is good enough. It's not obvious though what exactly are you excluding:

cargo test --all --exclude baz

Are we excluding baz package or baz module here?

Another option for flag name is --exclude--pkg.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 16, 2017

Contributor

@matklad I feel excluding a module would be a cheat somehow, so I wouldn't expect it to be possible with cargo. Also you can easily read the description of --exclude. We shouldn't make the flag longer than necessary.

Contributor

torkleyy commented May 16, 2017

@matklad I feel excluding a module would be a cheat somehow, so I wouldn't expect it to be possible with cargo. Also you can easily read the description of --exclude. We shouldn't make the flag longer than necessary.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 16, 2017

Contributor

It seems we got platform-specific behavior here:

I implemented a test which checks for this pattern:

"[..] Compiling bar v0.1.0 ([..])\n\
[..] Compiling foo v0.1.0 ([..])\n

It matches on Linux, however on Windows it seems to compile the crates the other way around.

EDIT: Now it even fails locally, so apparently behavior has changed. Fixing..
Huh? It fails both ways now... What's going on?

Contributor

torkleyy commented May 16, 2017

It seems we got platform-specific behavior here:

I implemented a test which checks for this pattern:

"[..] Compiling bar v0.1.0 ([..])\n\
[..] Compiling foo v0.1.0 ([..])\n

It matches on Linux, however on Windows it seems to compile the crates the other way around.

EDIT: Now it even fails locally, so apparently behavior has changed. Fixing..
Huh? It fails both ways now... What's going on?

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 16, 2017

Member

Huh? It fails both ways now... What's going on?

I bet foo and bar just compile in parallel in arbitrary order. You can use with_stderr_contains and with_stderr_does_not_contain (or you can put broken code in the excluded package and check that it does not break compilation).

Hm, what would happen if you compile foo and exclude bar if foo depends on bar? Should we add this test case?

Member

matklad commented May 16, 2017

Huh? It fails both ways now... What's going on?

I bet foo and bar just compile in parallel in arbitrary order. You can use with_stderr_contains and with_stderr_does_not_contain (or you can put broken code in the excluded package and check that it does not break compilation).

Hm, what would happen if you compile foo and exclude bar if foo depends on bar? Should we add this test case?

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 16, 2017

Contributor

exclude bar if foo depends on bar? Should we add this test case?

Oops, thought about this, but completely forgot it!

I'm not sure actually. I think the current behavior is just compiling it if it's a dependency.

Contributor

torkleyy commented May 16, 2017

exclude bar if foo depends on bar? Should we add this test case?

Oops, thought about this, but completely forgot it!

I'm not sure actually. I think the current behavior is just compiling it if it's a dependency.

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 25, 2017

Member

Let's assign @alexcrichton to this :)

r? @alexcrichton

Member

matklad commented May 25, 2017

Let's assign @alexcrichton to this :)

r? @alexcrichton

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 26, 2017

Member

@bors: r+

Looks great to me, thanks again for the PR @torkleyy!

Member

alexcrichton commented May 26, 2017

@bors: r+

Looks great to me, thanks again for the PR @torkleyy!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 26, 2017

Contributor

📌 Commit 9ad240b has been approved by alexcrichton

Contributor

bors commented May 26, 2017

📌 Commit 9ad240b has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 26, 2017

Contributor

⌛️ Testing commit 9ad240b with merge 50791b8...

Contributor

bors commented May 26, 2017

⌛️ Testing commit 9ad240b with merge 50791b8...

bors added a commit that referenced this pull request May 26, 2017

Auto merge of #4031 - torkleyy:exclude, r=alexcrichton
Add --exclude flag

Allows to exclude packages in conjunction
with --all.

Addresses #2878
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 26, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented May 26, 2017

💔 Test failed - status-travis

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 26, 2017

Contributor

Fixed (it failed because it expected a "filtered out" argument).

Contributor

torkleyy commented May 26, 2017

Fixed (it failed because it expected a "filtered out" argument).

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton
Member

alexcrichton commented May 26, 2017

@bors: r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 26, 2017

Contributor

📌 Commit b356af0 has been approved by alexcrichton

Contributor

bors commented May 26, 2017

📌 Commit b356af0 has been approved by alexcrichton

bors added a commit that referenced this pull request May 26, 2017

Auto merge of #4031 - torkleyy:exclude, r=alexcrichton
Add --exclude flag

Allows to exclude packages in conjunction
with --all.

Addresses #2878
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 26, 2017

Contributor

⌛️ Testing commit b356af0 with merge 9b856c7...

Contributor

bors commented May 26, 2017

⌛️ Testing commit b356af0 with merge 9b856c7...

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 26, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented May 26, 2017

💔 Test failed - status-travis

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 27, 2017

Contributor

Huh? It works for me, even after rebasing onto upstream/master. Anyways, I'll push a fix any moment...

Contributor

torkleyy commented May 27, 2017

Huh? It works for me, even after rebasing onto upstream/master. Anyways, I'll push a fix any moment...

Fix build_all_exclude
by using with_stderr_contains because
parallel build may mix up the lines.

Additionally, remove the last line of
the benchmark.
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 27, 2017

Member

@bors: r+

Ah yeah I think that's a very recent change on nightly

Member

alexcrichton commented May 27, 2017

@bors: r+

Ah yeah I think that's a very recent change on nightly

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 27, 2017

Contributor

📌 Commit dee3c2e has been approved by alexcrichton

Contributor

bors commented May 27, 2017

📌 Commit dee3c2e has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 27, 2017

Contributor

⌛️ Testing commit dee3c2e with merge 015897a...

Contributor

bors commented May 27, 2017

⌛️ Testing commit dee3c2e with merge 015897a...

bors added a commit that referenced this pull request May 27, 2017

Auto merge of #4031 - torkleyy:exclude, r=alexcrichton
Add --exclude flag

Allows to exclude packages in conjunction
with --all.

Addresses #2878
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 28, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 015897a to master...

Contributor

bors commented May 28, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 015897a to master...

@bors bors merged commit dee3c2e into rust-lang:master May 28, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@torkleyy torkleyy deleted the torkleyy:exclude branch May 28, 2017

bors added a commit that referenced this pull request Feb 26, 2018

Auto merge of #5081 - matklad:document-not-all-the-things, r=alexcric…
…hton

Support --exclude option for `cargo doc`

I think this should have been implemented when the feature was added for
other commands. Probably just an oversight.

cc #4031

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment