Skip to content

Comments

Support multiple --target flags on the CLI#8167

Merged
bors merged 1 commit intorust-lang:masterfrom
alexcrichton:multitarget
Apr 29, 2020
Merged

Support multiple --target flags on the CLI#8167
bors merged 1 commit intorust-lang:masterfrom
alexcrichton:multitarget

Conversation

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Apr 27, 2020

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

rust-highfive commented Apr 27, 2020

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
Closed
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Apr 27, 2020

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

alexcrichton commented Apr 28, 2020

@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.

ehuss
ehuss reviewed Apr 28, 2020
View reviewed changes
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?

@alexcrichton
Copy link
Member Author

alexcrichton commented Apr 28, 2020

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 alexcrichton mentioned this pull request Apr 28, 2020
Closed
@alexcrichton
Copy link
Member Author

alexcrichton commented Apr 28, 2020

Updated! Thanks for the keen eye

ehuss
ehuss reviewed Apr 28, 2020
View reviewed changes
@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.

@alexcrichton
Support multiple --target flags on the CLI
3fd2814 
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...

@bors bors merged commit bf1a26d into rust-lang:master Apr 29, 2020
This was referenced Apr 29, 2020
Merged
Merged
Merged
@Mygod
Copy link

Mygod commented Apr 29, 2020

Woah this is unexpectedly efficient.

This was referenced Jun 14, 2020
Merged
Closed
bors added a commit that referenced this pull request Jun 15, 2020
@bors
Auto merge of #8358 - ehuss:fix-target-host-doctest, r=alexcrichton
4e14cc1 
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
This was referenced Jan 17, 2021
Merged
Closed
@ehuss ehuss added this to the 1.45.0 milestone Feb 6, 2022
@weihanglo weihanglo mentioned this pull request Mar 10, 2022
Closed
@weihanglo weihanglo mentioned this pull request Jun 17, 2022
Merged
@ehuss ehuss mentioned this pull request Mar 6, 2025
Closed
@alexcrichton alexcrichton mentioned this pull request Mar 6, 2025
Merged
alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Mar 6, 2025
@alexcrichton
Don't use $CARGO_BUILD_TARGET in cargo metadata
70dc4d8 
This commit fixes a (five year old!) regression in `cargo metadata`
where if `--filter-platform` isn't explicitly specified it will
accidentally read `$CARGO_BUILD_TARGET` (or `build.target`
configuration) and use that as the default `--filter-platform`. The
reason for this is that the calculation for targets changed in rust-lang#8167
and while the shared function makes sense for other commands such as
`cargo build` the targets have a different meaning in `cargo metadata`
so a slightly different set of functionality is desired.

This commit fixes the issue by introducing a new constructor for the
list of `CompileKind` variants where the fallback of "if nothing is
specified" is explicitly chosen.
github-merge-queue bot pushed a commit that referenced this pull request Mar 6, 2025
@epage
Don't use $CARGO_BUILD_TARGET in cargo metadata (#15271)
1c5ec2b 
This commit fixes a (five year old!) regression in `cargo metadata`
where if `--filter-platform` isn't explicitly specified it will
accidentally read `$CARGO_BUILD_TARGET` (or `build.target`
configuration) and use that as the default `--filter-platform`. The
reason for this is that the calculation for targets changed in #8167 and
while the shared function makes sense for other commands such as `cargo
build` the targets have a different meaning in `cargo metadata` so a
slightly different set of functionality is desired.

This commit fixes the issue by introducing a new constructor for the
list of `CompileKind` variants where the fallback of "if nothing is
specified" is explicitly chosen.

Closes #15270

<!--
Thanks for submitting a pull request 🎉! Here are some tips for you:

* If this is your first contribution, read "Cargo Contribution Guide"
first:
  https://doc.crates.io/contrib/
* Run `cargo fmt --all` to format your code changes.
* Small commits and pull requests are always preferable and easy to
review.
* If your idea is large and needs feedback from the community, read how:
  https://doc.crates.io/contrib/process/#working-on-large-features
* Cargo takes care of compatibility. Read our design principles:
  https://doc.crates.io/contrib/design.html
* When changing help text of cargo commands, follow the steps to
generate docs:

https://github.com/rust-lang/cargo/tree/master/src/doc#building-the-man-pages
* If your PR is not finished, set it as "draft" PR or add "WIP" in its
title.
* It's ok to use the CI resources to test your PR, but please don't
abuse them.

### What does this PR try to resolve?

Explain the motivation behind this change.
A clear overview along with an in-depth explanation are helpful.

You can use `Fixes #<issue number>` to associate this PR to an existing
issue.

### How should we test and review this PR?

Demonstrate how you test this change and guide reviewers through your
PR.
With a smooth review process, a pull request usually gets reviewed
quicker.

If you don't know how to write and run your tests, please read the
guide:
https://doc.crates.io/contrib/tests

### Additional information

Other information you want to mention in this PR, such as prior arts,
future extensions, an unresolved problem, or a TODO list.
-->
@jellllly420 jellllly420 mentioned this pull request Aug 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ehuss ehuss ehuss left review comments

Assignees

@ehuss ehuss

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.45.0

Development

Successfully merging this pull request may close these issues.

6 participants

@alexcrichton @rust-highfive @Mark-Simulacrum @bors @ehuss @Mygod