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

Enable new behavior of --feature #5390

Merged
merged 1 commit into from
Apr 19, 2018
Merged

Conversation

matklad
Copy link
Member

@matklad matklad commented Apr 18, 2018

So far, the feedback on https://internals.rust-lang.org/t/help-us-test-the-breaking-bug-fix-to-cargo-features/7317 has been positive, so here's a PR to try this in nightly.

Note that the logic is slightly tweak for the case cargo build -p not-a-workspace-member: we want not only to resolve all ws members in this case, but to enable all of their features as well!

As a sanity check, this seems to be forward compatible with further improvements to features:

  1. when we solve the grand bug of features being unified across the whole workspace, cargo build -p not-a-member would hopefully just work without additional contortions.
  2. we might add a way to specify features per package, like cargo build -p foo -p bar --features "foo/serde bar/serde"

@matklad
Copy link
Member Author

matklad commented Apr 19, 2018

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 19, 2018

📌 Commit 038eec5 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Apr 19, 2018

⌛ Testing commit 038eec5 with merge 0a33eb2...

bors added a commit that referenced this pull request Apr 19, 2018
Enable new behavior of `--feature`

So far, the feedback on https://internals.rust-lang.org/t/help-us-test-the-breaking-bug-fix-to-cargo-features/7317 has been positive, so here's a PR to try this in nightly.

Note that the logic is slightly tweak for the case `cargo build -p not-a-workspace-member`: we want not only to resolve all ws members in this case, but to enable all of their features as well!

As a sanity check, this seems to be forward compatible with further improvements to features:

1) when we solve the grand bug of features being unified across the whole workspace, `cargo build -p not-a-member` would hopefully just work without additional contortions.
2) we might add a way to specify features per package, like `cargo build -p foo -p bar --features "foo/serde bar/serde"`
@bors
Copy link
Contributor

bors commented Apr 19, 2018

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

@bors bors merged commit 038eec5 into rust-lang:master Apr 19, 2018
matklad added a commit to matklad/rust that referenced this pull request Apr 21, 2018
Some noteble changes:

  * regression fix: rust-lang/cargo#5390
  * potentially breaking bug-fix: rust-lang/cargo#5389
  * Cargo now caches the result of `rustc -vV`. It checks `rustc` binary
    mtime and rustup toolchain settings, so it should probably "just work"
    with rustbuild.
bors added a commit to rust-lang/rust that referenced this pull request Apr 22, 2018
Update Cargo

Some noteble changes:

 * ~~regression fix: rust-lang/cargo#5390
  * ~~potentially breaking bug-fix: rust-lang/cargo#5389
  * ~~Cargo now caches the result of `rustc -vV`. It checks `rustc` binary
    mtime and rustup toolchain settings, so it should probably "just work"
    with rustbuild.~~

potentially breaking bug-fix: rust-lang/cargo#5390
@SimonSapin
Copy link
Contributor

Servo’s build broke, I think because of this, without a prior warning.

It’s not clear what the compatibility story is here.

@SimonSapin
Copy link
Contributor

(PR for that Servo build: servo/servo#20703, already including a build system change.)

@matklad
Copy link
Member Author

matklad commented Apr 28, 2018

Let's continue discussion at the tracking issue: #5364 (comment)

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