Support `-Zmultitarget` in cargo config #10473

weihanglo · 2022-03-11T18:33:49Z

What does this PR try to resolve?

Support -Zmultitarget in cargo config. This patch also takes care of backward compatibility, so basically it means that build.target accepts input from both string and array.

#8176 (comment)

[unstable]
multitarget = true

[build]
target = ["x86_64-unknown-linux-gnu", "i686-unknown-linux-gnu"]

How should we test and review this PR?

Several new tests in multitarget.

I didn't add tests for all compile modes, since doing that would produce too many duplications. If anyone thinks it necessary to avoid potential regression please tell me.

Unresolved questions

How to handle CARGO_BUILD_TARGET environment variable? Should it remain accepting a single target value only and hope advanced-env save us?
This unstable feature on CLI has landed for a while. What are other difficulties blocking it from stabilizing?

For backward compatible reason, `build.target` starts accepting both array of string and a single string.

rust-highfive · 2022-03-11T18:33:53Z

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

edwincoronado · 2022-03-11T20:02:14Z

thanks for getting to this so fast @weihanglo ! I have one quick question, it looks like I would still have to do cargo build -Zmultitarget correct? Would adding the

[unstable]
multitarget = true

basically do the same so that I only have to do cargo build?

weihanglo · 2022-03-12T00:11:18Z

@edwincoronado that's correct. You might need on nightly channel at this moment to turn on -Zmultitarget feature.

Anything which can be configured with a -Z flag can also be set in the cargo config file (.cargo/config.toml) in the unstable table. For example:

Ref: https://doc.rust-lang.org/1.59.0/cargo/reference/unstable.html#unstable-features

egdeeno · 2022-03-13T19:08:19Z

maybe use word "targets" instead of "target" ?
targets = ["x86_64-unknown-linux-gnu", "i686-unknown-linux-gnu"]
vs
target = "x86_64-unknown-linux-gnu"

weihanglo · 2022-03-14T00:21:52Z

@edgeone89 I've considered the solution and it's indeed more intuitive. However, when we want to maintain the backwards compatibility we need to preserve build.target. Then it ends up becoming two configs build.target and build.targets, as well as their counterparts CARGO_BUILD_TARGET and CARGO_BUILD_TARGETS environment variables. To make things simple, I prefer to reuse build.target and accept both string and array values.

egdeeno · 2022-03-14T07:36:13Z

@weihanglo ok, thanks.
In case of target = ["x86_64-unknown-linux-gnu", "i686-unknown-linux-gnu"] what will be default target? 0-indexed ?

weihanglo · 2022-03-14T08:27:22Z

In case of target = ["x86_64-unknown-linux-gnu", "i686-unknown-linux-gnu"] what will be default target? 0-indexed ?

Every target listed in build.target will be built by default. If this behavior is so-called a default target, I think all of them are default targets.

Note that when using build.target config or --target cli arg, the build cache and artifacts will be placed under target/$TRIPLE/ for each target platform instead of the root target/.

edwincoronado · 2022-03-25T03:00:37Z

any update on this?

tests/testsuite/multitarget.rs

src/cargo/util/config/mod.rs

src/cargo/core/compiler/compile_kind.rs

weihanglo · 2022-03-29T06:22:34Z

All review comments are resolved.

I was trying to avoid extra allocation, but it turns that the relevant code is not in the hot path during compilations. Your suggestion indeed reduces the code complexity. Thank you!

The relevant is not in the hot path for a cargo build, so it acceptable to allocation String in order to reduce code coomplexity

ehuss · 2022-03-30T20:39:21Z

src/cargo/util/config/mod.rs

+        let values = match &self.inner.val {
+            BuildTargetConfigInner::One(s) => vec![map(s)],
+            BuildTargetConfigInner::Many(v) => {
+                if v.len() > 1 && !config.cli_unstable().multitarget {


Can this skip the length check, just to prevent target = ["foo"], which would be new syntax?

Suggested change

if v.len() > 1 && !config.cli_unstable().multitarget {

if !config.cli_unstable().multitarget {

Ahh, missed that one. Thanks!
Should be fixed now and I tweak the error message a bit.

ehuss · 2022-03-31T00:17:13Z

Thanks!

@bors r+

bors · 2022-03-31T00:17:14Z

📌 Commit c18275b has been approved by ehuss

bors · 2022-03-31T00:17:22Z

⌛ Testing commit c18275b with merge 1ef1e0a...

bors · 2022-03-31T00:53:59Z

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

Update cargo 13 commits in 109bfbd055325ef87a6e7f63d67da7e838f8300b..1ef1e0a12723ce9548d7da2b63119de9002bead8 2022-03-17 21:43:09 +0000 to 2022-03-31 00:17:18 +0000 - Support `-Zmultitarget` in cargo config (rust-lang/cargo#10473) - doc: Fix document url for libcurl format (rust-lang/cargo#10515) - Fix wrong info in "Environment variables" docs (rust-lang/cargo#10513) - Use the correct flag in --locked --offline error message (rust-lang/cargo#10512) - Don't treat host/target duplicates as duplicates (rust-lang/cargo#10466) - Unstable --keep-going flag (rust-lang/cargo#10383) - Part 1 of RFC2906 - Packages can inherit fields from their root workspace (rust-lang/cargo#10497) - Remove unused profile support for -Zpanic-abort-tests (rust-lang/cargo#10495) - HTTP registry implementation (rust-lang/cargo#10470) - Add a notice about review capacity. (rust-lang/cargo#10501) - Add tests for ignoring symlinks (rust-lang/cargo#10047) - Update doc string for deps_of/compute_deps. (rust-lang/cargo#10494) - Consistently use crate::display_error on errors during drain (rust-lang/cargo#10394)

weihanglo added 4 commits March 12, 2022 01:53

Tests for setting multitarget in config

20d96f3

Support multitarget in .cargo/config.toml

76055d2

For backward compatible reason, `build.target` starts accepting both array of string and a single string.

Doc for support multitarget in config

069b884

Derive debug from BuildTargetConfigValue

fcc8816

rust-highfive assigned ehuss Mar 11, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 11, 2022

ehuss reviewed Mar 29, 2022

View reviewed changes

tests/testsuite/multitarget.rs Outdated Show resolved Hide resolved

src/cargo/util/config/mod.rs Outdated Show resolved Hide resolved

src/cargo/util/config/mod.rs Outdated Show resolved Hide resolved

src/cargo/core/compiler/compile_kind.rs Outdated Show resolved Hide resolved

weihanglo added 3 commits March 29, 2022 14:25

Remove unnecessary nightly masquerade

40e1360

Remove unnecessary enum to reduce code complexity

086145e

The relevant is not in the hot path for a cargo build, so it acceptable to allocation String in order to reduce code coomplexity

test: fix to error message for multitarget

3c8ba9a

weihanglo force-pushed the multitarget-config branch from ba13a1e to 3c8ba9a Compare March 29, 2022 06:25

ehuss reviewed Mar 30, 2022

View reviewed changes

Guard that array value of build.target should be in nightly

c18275b

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 Mar 31, 2022

bors merged commit 1ef1e0a into rust-lang:master Mar 31, 2022

ehuss mentioned this pull request Mar 31, 2022

Update cargo rust-lang/rust#95507

Merged

weihanglo deleted the multitarget-config branch March 31, 2022 11:52

ehuss added this to the 1.61.0 milestone Apr 7, 2022

weihanglo mentioned this pull request Jun 18, 2022

Tracking issues for the -Zmultitarget feature #8176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `-Zmultitarget` in cargo config #10473

Support `-Zmultitarget` in cargo config #10473

weihanglo commented Mar 11, 2022 •

edited

Loading

rust-highfive commented Mar 11, 2022

edwincoronado commented Mar 11, 2022

weihanglo commented Mar 12, 2022 •

edited

Loading

egdeeno commented Mar 13, 2022 •

edited

Loading

weihanglo commented Mar 14, 2022

egdeeno commented Mar 14, 2022

weihanglo commented Mar 14, 2022

edwincoronado commented Mar 25, 2022

weihanglo commented Mar 29, 2022

ehuss Mar 30, 2022

weihanglo Mar 30, 2022

ehuss commented Mar 31, 2022

bors commented Mar 31, 2022

bors commented Mar 31, 2022

bors commented Mar 31, 2022

	if v.len() > 1 && !config.cli_unstable().multitarget {
	if !config.cli_unstable().multitarget {

Support -Zmultitarget in cargo config #10473

Support -Zmultitarget in cargo config #10473

Conversation

weihanglo commented Mar 11, 2022 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Unresolved questions

rust-highfive commented Mar 11, 2022

edwincoronado commented Mar 11, 2022

weihanglo commented Mar 12, 2022 • edited Loading

egdeeno commented Mar 13, 2022 • edited Loading

weihanglo commented Mar 14, 2022

egdeeno commented Mar 14, 2022

weihanglo commented Mar 14, 2022

edwincoronado commented Mar 25, 2022

weihanglo commented Mar 29, 2022

ehuss Mar 30, 2022

Choose a reason for hiding this comment

weihanglo Mar 30, 2022

Choose a reason for hiding this comment

ehuss commented Mar 31, 2022

bors commented Mar 31, 2022

bors commented Mar 31, 2022

bors commented Mar 31, 2022

Support `-Zmultitarget` in cargo config #10473

Support `-Zmultitarget` in cargo config #10473

weihanglo commented Mar 11, 2022 •

edited

Loading

weihanglo commented Mar 12, 2022 •

edited

Loading

egdeeno commented Mar 13, 2022 •

edited

Loading