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

Make cargo metadata and tree respect target #8987

Merged
merged 7 commits into from
Dec 19, 2020

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Dec 16, 2020

Previously, the metadata and tree subcommands would download
dependencies for all targets, regardless of whether those targets were
actually enabled. This PR updates them so that they only download the
same dependencies that building would do with the requested target(s).

cargo metadata still includes all targets by default; you can only opt
out using --filter-platform. Ideally it should use --target the
same way tree does, but it's too late to change that now.

Fixes #8981.

Previously, the `metadata` and `tree` subcommands would download
dependencies for all targets, regardless of whether those targets were
actually enabled. This PR updates them so that they only download the
same dependencies that building would do with the requested target(s).

`cargo metadata` still includes all targets by default; you can only opt
_out_ using `--filter-platform`. Ideally it should use `--target` the
same way `tree` does, but it's too late to change that now.

Fixes rust-lang#8981.
@rust-highfive
Copy link

r? @alexcrichton

(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 Dec 16, 2020
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Could a new test or two be added for the new behavior as well? I think @ehuss should also take a look at this since he's more familiar with cargo tree than I am.

src/cargo/ops/cargo_output_metadata.rs Show resolved Hide resolved
src/cargo/ops/cargo_output_metadata.rs Show resolved Hide resolved
src/cargo/ops/tree/graph.rs Show resolved Hide resolved
@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 17, 2020

I added a test to ensure that we only download the needed crates for cargo metadata. cargo tree already has a test that changed as a result of this.

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 18, 2020

@ehuss Great catch -- I've updated the PR to remove the unnecessary step.

@ehuss
Copy link
Contributor

ehuss commented Dec 19, 2020

I'm not super confident this won't have issues, but I can't think of any potential problems. Thanks for fixing it!

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 19, 2020

📌 Commit 260a355 has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2020
@bors
Copy link
Collaborator

bors commented Dec 19, 2020

⌛ Testing commit 260a355 with merge a79aa3a...

@bors
Copy link
Collaborator

bors commented Dec 19, 2020

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing a79aa3a to master...

@bors bors merged commit a79aa3a into rust-lang:master Dec 19, 2020
@jonhoo jonhoo deleted the metadata-target-filtering branch December 19, 2020 17:26
@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 19, 2020

I wonder, should this be tagged with relnotes, so that if it impacts people down stream they'll have an easier way finding their way back here?

@ehuss
Copy link
Contributor

ehuss commented Dec 19, 2020

I generally reserve relnotes for new big features or major changes (mostly my bar is "do most users want to know about this?"). Most PRs (like this) are included in the CHANGELOG, so people who really want to know what is going on can find it there. Usually when people run into a problem, they'll file an issue and usually it'll be obvious to me what's going on. I don't expect this to cause problems, though.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 24, 2020
Update cargo

10 commits in a3c2627fbc2f5391c65ba45ab53b81bf71fa323c..75d5d8cffe3464631f82dcd3c470b78dc1dda8bb
2020-12-14 17:21:26 +0000 to 2020-12-22 18:10:56 +0000
- Update git2 (rust-lang/cargo#9009)
- Stabilize RUSTC_WORKSPACE_WRAPPER (rust-lang/cargo#8976)
- Make cargo metadata and tree respect target (rust-lang/cargo#8987)
- Update git2 (rust-lang/cargo#8998)
- Revert rust-lang/cargo#8954 - changing rustdoc's cwd (rust-lang/cargo#8996)
- With debug HTTP mode log curl's version (rust-lang/cargo#8991)
- Reject ambiguous git dependency declaration. (rust-lang/cargo#8984)
- Fix tests not working with a different CARGO_TARGET_DIR. (rust-lang/cargo#8982)
- Add version to credential dependencies. (rust-lang/cargo#8983)
- Clarify FAQ entry wording about lockfiles (rust-lang/cargo#8978)
@ehuss ehuss added this to the 1.50.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo metadata downloads crate files for unused targets
5 participants