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

Only apply default-members when building root manifest #6755

Closed
wants to merge 2 commits into from

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Mar 16, 2019

Fixes #5932

The first commit includes all actual changes, the second commit is just running Asciidoctor on the already updated man-pages source, so I suggest looking at the individual commits.

@rust-highfive
Copy link

r? @ehuss

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2019
src/doc/man/options-packages.adoc Outdated Show resolved Hide resolved
src/doc/man/options-packages.adoc Outdated Show resolved Hide resolved
src/doc/man/options-packages.adoc Outdated Show resolved Hide resolved
src/doc/man/options-packages.adoc Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented Mar 16, 2019

Thanks for updating the documentation. ❤️

I'm slightly positive to this change. I want to see what others at @rust-lang/cargo think about it. The summary is:

This changes default-members to be ignored when in a non-root member to only build that member.

Without default-members, nothing changes:

Command Virtual Packages Selected
cargo in ws/ All members
cargo in ws/ Root crate only
cargo in ws/foo/ foo only
cargo in ws/foo/ foo only

With default members, this PR changes the last two lines in this table from default-members to "foo only":

Command Virtual Packages Selected
cargo in ws/ default-members only
cargo in ws/ default-members only
cargo in ws/foo/ default-members only
cargo in ws/foo/ default-members only

I'm a little concerned this may disrupt existing workflows. It may be a little more consistent with the non-default-members case.

I'm also curious what @SimonSapin thinks. He added it for servo, though it appears servo no longer uses it. I don't use any projects with this feature, so I don't have any experience with it.

@alexcrichton
Copy link
Member

This seems like a reasonable change in behavior to me as well!

@Nemo157
Copy link
Member Author

Nemo157 commented Mar 19, 2019

I've rebased your doc suggestions in.

bors added a commit that referenced this pull request Mar 19, 2019
Fix spurious error in dirty_both_lib_and_test.

On HFS, if it runs fast enough, the mtimes will be equal, causing it to fail to rebuild.

Seen on #6755 https://travis-ci.com/rust-lang/cargo/jobs/185412514
@bors
Copy link
Contributor

bors commented Apr 1, 2019

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

@alexcrichton
Copy link
Member

I'm gonna close this since it's been inactive for some time now, but it can of course be resubmitted! (just trying to help clear out the queue)

@plauche
Copy link

plauche commented Jul 18, 2019

@Nemo157 any plans to pick this back up? I've run into the same unexpected behaviour when using default-members. I may restart this work if you aren't planning on continuing with it.

@Nemo157
Copy link
Member Author

Nemo157 commented Jul 19, 2019

I've rebased the branch, and the tests still pass.

I was waiting on the feedback that @ehuss requested above, if they think that consensus on merging this can be had I'll happily open another PR for it. I'm not working on the project where I was having this issue much anymore so I didn't have the motivation to try and push for getting this merged.

@plauche
Copy link

plauche commented Jul 29, 2019

@ehuss do you have any input on moving forward with this PR? I'd like to use the default-members feature, however the current behaviour is when working in member crates is not very desirable.

@ehuss
Copy link
Contributor

ehuss commented Jul 31, 2019

I have posted a request for feedback at https://internals.rust-lang.org/t/cargo-change-in-workspace-default-members/10649. If there aren't any significant objections, then I'll go ahead and approve this PR.

@ehuss
Copy link
Contributor

ehuss commented Aug 19, 2019

I haven't seen any objections, so I'm going to go ahead and merge this. However, github won't let me reopen, so I will open a new PR and merge that (#7270).

bors added a commit that referenced this pull request Aug 19, 2019
Only apply default-members when building root manifest

This is a reopening of #6755 rebased on master.

Closes #5932
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo build in a workspaced crate builds workspaces' default members
6 participants