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

Support multiple --target flags on the CLI #8167

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

alexcrichton
Copy link
Member

This commit refactors the internals of Cargo to no longer have a
singular --target flag (and singular requested_target kind throught)
but to instead have a list. The semantics of multiple --target flags
is to build the selected targets for each of the input --target flag
inputs.

For now this is gated behind -Zmultitarget as an unstable features,
since I'm not entirely sure this is the interface we want. In general
it'd be great if we had a way to simply specify Unit structures of
what to build on the CLI, but we're in general very far away from that,
so I figured that this is probably sufficient at least for testing for
now.

cc #8156

@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 Apr 27, 2020
@alexcrichton alexcrichton mentioned this pull request Apr 27, 2020
@Mark-Simulacrum
Copy link
Member

Hm, so if I'm interpreting this right, we would expect that e.g. rustbuild would (eventually) start using this to parallelize the std-building on our dist-various builders, right? That would (I imagine) see some pretty good improvement in CI times, especially on the many-core builders we're getting with GHA.

I haven't actually read the PR to determine if that's true, though.

@alexcrichton
Copy link
Member Author

@Mark-Simulacrum I'd be pretty surprised if rustbuild could take advantage of this tbh, although it's perhaps not completely out of the question. It would likely take a large-ish refactoring because there'd no longer be a Std-per-target but only a Std-per-stage, which isn't what all builds want all the time anyway.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Looks like there are a couple nightly-only tests that need to be fixed.

I noticed that it does not handle multiple --target flags with the same value very well (Cargo hangs acquiring the file lock). Perhaps BuildConfig::requested_kinds should be a set? Or maybe there should be some deduplication in from_requested_targets?

src/cargo/core/compiler/compilation.rs Outdated Show resolved Hide resolved
src/bin/cargo/commands/metadata.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/compilation.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_fetch.rs Show resolved Hide resolved
src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved
tests/testsuite/multitarget.rs Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Member Author

I noticed that it does not handle multiple --target flags with the same value very well

Oops I remember thinking I should write a test and handle that and then I must have gotten distracted before implementing a fix for that...

@alexcrichton
Copy link
Member Author

Updated! Thanks for the keen eye

@bors
Copy link
Contributor

bors commented Apr 28, 2020

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

This commit refactors the internals of Cargo to no longer have a
singular `--target` flag (and singular `requested_target` kind throught)
but to instead have a list. The semantics of multiple `--target` flags
is to build the selected targets for each of the input `--target` flag
inputs.

For now this is gated behind `-Zmultitarget` as an unstable features,
since I'm not entirely sure this is the interface we want. In general
it'd be great if we had a way to simply specify `Unit` structures of
what to build on the CLI, but we're in general very far away from that,
so I figured that this is probably sufficient at least for testing for
now.

cc rust-lang#8156
@ehuss
Copy link
Contributor

ehuss commented Apr 29, 2020

👍
@bors r+

@bors
Copy link
Contributor

bors commented Apr 29, 2020

📌 Commit 3fd2814 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 Apr 29, 2020
@bors
Copy link
Contributor

bors commented Apr 29, 2020

⌛ Testing commit 3fd2814 with merge bf1a26d...

@bors
Copy link
Contributor

bors commented Apr 29, 2020

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing bf1a26d to master...

@Mygod
Copy link

Mygod commented Apr 29, 2020

Woah this is unexpectedly efficient.

bors added a commit that referenced this pull request Jun 15, 2020
Fix doctests not running with --target=HOST.

There was a regression in #8167 where `cargo test --target=$HOST` stopped running doctests. This caused doctests to silently stop running in rust-lang/rust (rust-lang/rust#73286).  This PR restores the original behavior where `--target=$HOST` behaves as-if it is a normal host test.

There was a discussion about this at #8167 (review), but I think I let it slip through the cracks.
@alexcrichton alexcrichton deleted the multitarget branch July 29, 2020 17:48
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.

6 participants