Pass rustflags to artifacts built with implicit targets when using target-applies-to-host #13900

gmorenz · 2024-05-11T03:29:42Z

What this PR does

This fixes #10744, a long-standing bug with target-applies-to-host=false where RUSTFLAGS are not being passed to artifact-units when built with cargo build (as opposed to cargo build --target <host>).

It doesn't fix a similar problem with linker and links. If the architecture in this PR is accepted, I expect I will be able to fix linker and links in the same way in a subsequent PR.

Below is a hopefully useful table of what flags are passed when, with new behavior bolded (without these changes the flag is not passed). I've only changed values in the top right cell, I've included the whole table for completeness and to hopefully make clear what exactly this feature is doing (which took me awhile to understand).

The table was generated with a host of x86_64-unknown-linux-gnu. "Flag" refers to values in RUSTFLAGS. "Linker" refers to the value of --config target.<host>.linker . The table was built with this repo and then hand modify to bold changed values.

	target_applies_to_host=true	target_applies_to_host=false
no --target flag	Flag passed to bin Flag passed to shared dep from bin Linker passed to bin Flag passed to proc macro Flag passed to shared dep from proc macro Linker passed to proc macro Built with 5 invocations Without rustflags, built with 5 invocations	Flag passed to bin Flag passed to shared dep from bin Linker not passed to bin Flag not passed to proc macro Flag not passed to shared dep from proc macro Linker not passed to proc macro Built with 6 invocations Without rustflags, built with 5 invocations
--target x86_64-unknown-linux-gnu	Flag passed to bin Flag passed to shared dep from bin Linker passed to bin Flag not passed to proc macro Flag not passed to shared dep from proc macro Linker passed to proc macro Built with 6 invocations Without rustflags, built with 6 invocations	Flag passed to bin Flag passed to shared dep from bin Linker passed to bin Flag not passed to proc macro Flag not passed to shared dep from proc macro Linker not passed to proc macro Built with 6 invocations Without rustflags, built with 6 invocations
--target i686-unknown-linux-gnu	Flag passed to bin Flag passed to shared dep from bin Linker not passed to bin Flag not passed to proc macro Flag not passed to shared dep from proc macro Linker passed to proc macro Built with 6 invocations Without rustflags, built with 6 invocations	Flag passed to bin Flag passed to shared dep from bin Linker not passed to bin Flag not passed to proc macro Flag not passed to shared dep from proc macro Linker not passed to proc macro Built with 6 invocations Without rustflags, built with 6 invocations

How it is implemented

There are two stages in the UnitGraph's life. It gets built, with correct CompileKind: CompileKind::Host for host units, CompileKind::Target(HostTriple) for artifact units. Then it gets rebuilt with artifact units that are intended for the host having their kind changed to CompileKind::Host.

This rebuilding step serves to de-duplicate host and artifact units. A step that we want to maintain where possible. Because de-duplicating host and artifact dependencies is only possible if their rustflags don't differ we must calculate rustflags for units before this step, and this step necessarily throws away the information necessary to calculate them.

The possible rustflags have already been determined before creating units. "Calculating rustflags for units" just means determining which set of rustflags go with a particular unit, and storing that somewhere. The obvious place to do that is in the unit itself, so that's what this PR does, which has the convenient side affect of also handing the de-duplication logic for us. If the rustflags are the same, two units can be merged, if they differ, they cannot.

In the above table that's why it takes 5 invocations to build the repo without rustflags, but 6 with them. The shared_dependency merges when it is built without rustflags, and not when it is built with rustflags.

Related PRs, comments and issues

fixes #10744

#9453 - Tracking issue

#9753 - Stabilization PR

#9634 - I believe this was another attempt (going down another implementation route) to fix the same issue

Comments from Alex Crichton noting that this must be fixed 1 2

My own comment describing exactly how I ran into this

Documentation for the feature

rustbot · 2024-05-11T03:29:48Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

gmorenz · 2024-05-11T03:38:55Z

@rustbot label: Z-target-applies-to-host

The original work for this feature was done under Alex Crichton, and no team member has seemingly picked up the feature after he stepped down. Since the original bug isn't marked as "S-accepted" I'm rather ignoring the contributor documentation by submitting this PR in the first place (sorry).

If this isn't the direction whatever team member picks this up would like to go in, I'm happy to do something else. If no team member picks this up (which is what the contributing docs suggest should happen)... I guess the PR sits here harmlessly ¯\_(ツ)_/¯ (I'll probably show up at office hours in that case and see if I can prompt some movement).

gmorenz · 2024-05-11T04:15:16Z

If I'm reading the test error correctly, it's a formatting change in the benchmark harness completely unrelated to this PR

It's expecting

test bin_bench ... bench:           0 ns/iter (+/- 0)

It's finding

test bin_bench ... bench:           0.00 ns/iter (+/- 0.00)

With extra .00s

Fixed in #13901

ehuss · 2024-05-13T19:20:29Z

If you rebase on latest master, the test issue should be fixed.

gmorenz · 2024-05-13T23:27:58Z

Rebased on master. Also merged the commit fixing the doc links into the commits moving the the docs around, since I noticed comments on other PRs asking that every commit pass tests.

weihanglo

Thanks and sorry for the long wait.

Could you follow the "atomic commit" pattern? That is, the first commit show the current buggy behavior and pass all tests, followed by the other commits fix both tests and the bug.

weihanglo · 2024-07-01T22:24:14Z

src/cargo/core/compiler/build_context/target_info.rs

-            let ct = CompileTarget::new(&rustc.host)?;
-            target_info.insert(ct, host_info.clone());
-            target_config.insert(ct, gctx.target_cfg_triple(&rustc.host)?);
+            target_config.insert(host_target, gctx.target_cfg_triple(&rustc.host)?);


This looks correct.

Would you mind sharing why we need the entire infra change (Arc and move out rustflags from targetinfo), instead of just the change here?

This question is basically what "How it is implemented" tries to answer in the original comment on this PR. To take another stab at explaining it here:

The CompileKind for artifact Unit starts out as CompileKind::Target(host_target) (assuming we're not cross compiling). The CompileKind for host Unit starts out as CompileKind::Host

This change sets up target_info so that lookups for CompileKind::Target(host_target) get the correct rustflags (as well as units with CompileKind::Host, which need different rustflags). Which is great provided we lookup artifact Unit's with their original CompileKind.

rebuild_unit_graph_shared changes artifact Units with CompileKind::Target(host_target) to have CompileKind::Host. It does this so that we can deduplicate identical otherwise identical units and only compile them once*.

The current infrastructure attempts to look up rustflags via kind after rebuild_unit_graph_shared. Thus even though we have correctly set up the target_info for CompileKind::Target(host_target) we're going to look up the flags in host_info, which are wrong. The infrastructure change is to push this lookup forwards to before we rebuild the unit with a different kind. It is also necessary because we deduplicate Units based on their hash, so even if we could somehow lookup the correct rustflags later, there wouldn't necessarily be a unique set of them (in the case where host and target artifacts have different rustflags, and a shared dependency).

* To draw your attention to the table at the top. This is why no --target flag, target_applies_to_host=false has "Without rustflags, built with 5 invocations" rather than 6. A dependency shared between the host artifacts and the target artifacts only has to be built once because it is built the exact same way for both of them.

However this question caused me to double check that my understanding was correct by... trying it. I believe it is, however it turns out my test is broken and will (incorrectly) pass (fail to compile) because passing an invalid rustflag will cause TargetInfo::new(host_target) to fail because that command calls rustc with rustflags to query file-names/sysroot/... even though we never build a target with those rustflags.

I'll figure out how to write a version of that test without the false-pass on the partial change and push it later tonight.

Thanks. I like the idea that Unit holds the exact rustflags it is going to invoke. It opens a new door that we might be able to use this information to create the acutal “build plans” for external systems.

There's only one nit left and I'll merge this :)

src/cargo/ops/cargo_compile/unit_generator.rs

gmorenz · 2024-07-02T00:56:32Z

Could you follow the "atomic commit" pattern? That is, the first commit show the current buggy behavior and pass all tests, followed by the other commits fix both tests and the bug.

I've done this as requested, as well as incorporation the suggested 1 line change, rebasing on master, and fixing the use of a now-deprecated testing function.

I owe you fixing the test so that it doesn't pass on the partial-fix you asked about in the other review commit.

gmorenz · 2024-07-02T02:06:09Z

Alright, pushed an updated test that passes --cfg feature="flag" in RUSTFLAGS instead of an unexpected cfg. This doesn't change the behavior of anything in this branch at all, but it means that partial fix now passes the current-behavior version of the test and doesn't pass the fixed behavior version of the test, because it doesn't error when querying rustc for information prior to building anything.

The partial fix branch is here, just for illustrative purposes.

gmorenz · 2024-07-02T02:15:48Z

Oops, pushing again with fixed formatting in the test in the first commit, and a fixed comment in the test in the second commit (just sloppy rebasing on my part)

weihanglo · 2024-07-04T05:01:21Z

tests/testsuite/rustflags.rs

@@ -1587,3 +1587,29 @@ fn host_config_rustflags_with_target() {
        .arg("host.rustflags=[\"--cfg=foo\"]")
        .run();
 }
+
+#[cargo_test]
+fn target_applies_to_host_rustflags_works() {


Should we have a similar test for rustdocflags to prevent regressions?

Sure, just pushed this

weihanglo · 2024-07-04T13:19:44Z

src/cargo/core/compiler/build_context/target_info.rs

-            let ct = CompileTarget::new(&rustc.host)?;
-            target_info.insert(ct, host_info.clone());
-            target_config.insert(ct, gctx.target_cfg_triple(&rustc.host)?);
+            target_config.insert(host_target, gctx.target_cfg_triple(&rustc.host)?);


Thanks. I like the idea that Unit holds the exact rustflags it is going to invoke. It opens a new door that we might be able to use this information to create the acutal “build plans” for external systems.

There's only one nit left and I'll merge this :)

…rget-applies-to-host

weihanglo · 2024-07-04T19:50:57Z

Thanks a lot!

@bors r+

bors · 2024-07-04T19:51:00Z

📌 Commit 5be8044 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-07-04T19:52:08Z

⌛ Testing commit 5be8044 with merge 8457356...

bors · 2024-07-04T20:24:33Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 8457356 to master...

Update cargo 13 commits in a515d463427b3912ec0365d106791f88c1c14e1b..f86ce56396168edf5d0e1c412ddda0b2edba7d46 2024-07-02 20:53:36 +0000 to 2024-07-05 17:52:05 +0000 - test: Migrate jobserver to snapbox (rust-lang/cargo#14191) - chore(deps): update msrv (3 versions) to v1.77 (rust-lang/cargo#14186) - test: migrate build_plan and build_script to snapbox (rust-lang/cargo#14193) - test: migrate cfg and check to snapbox (rust-lang/cargo#14185) - test: migrate install* and inheritable_workspace_fields to snapbox (rust-lang/cargo#14170) - Pass rustflags to artifacts built with implicit targets when using target-applies-to-host (rust-lang/cargo#13900) - test: Migrate network tests to snapbox (rust-lang/cargo#14187) - test: migrate some files to snapbox (rust-lang/cargo#14113) - test: Auto-redact `... after last build at ...`; Migrate `freshness` to Snapbox (rust-lang/cargo#14161) - chore: fix some typos (rust-lang/cargo#14182) - fix: improve message for inactive weak optional feature with edition2024 through unused dep collection (rust-lang/cargo#14026) - test：migrate `doc/directory/docscrape` to snapbox (rust-lang/cargo#14171) - test: Migrate git_auth to snapbox (rust-lang/cargo#14172)

Add rustdocflags to Unit's Debug impl ### What does this PR try to resolve? Not adding this was an oversight in #13900, `rustflags` was added but not `rustdocflags`. Sorry about not getting this right the first time. ### How should we test and review this PR? Read the code and compare to the original PR where the matching line was added for rustflags. r? `@weihanglo`

Update cargo 20 commits in a515d463427b3912ec0365d106791f88c1c14e1b..154fdac39ae9629954e19e9986fd2cf2cdd8d964 2024-07-02 20:53:36 +0000 to 2024-07-07 01:28:23 +0000 - test: relax redactions for rust-lang/rust (rust-lang/cargo#14203) - use "bootstrap" instead of "rustbuild" (rust-lang/cargo#14207) - test: migrate serveral files to snapbox (rust-lang/cargo#14180) - Add rustdocflags to Unit's Debug impl (rust-lang/cargo#14201) - Allow enabling `config-include` feature in config (rust-lang/cargo#14196) - fix(test): Restore `does_not_contain` for check (rust-lang/cargo#14198) - test: migrate patch, pkgid, proc_macro and progress to snapbox (rust-lang/cargo#14181) - test: Migrate jobserver to snapbox (rust-lang/cargo#14191) - chore(deps): update msrv (3 versions) to v1.77 (rust-lang/cargo#14186) - test: migrate build_plan and build_script to snapbox (rust-lang/cargo#14193) - test: migrate cfg and check to snapbox (rust-lang/cargo#14185) - test: migrate install* and inheritable_workspace_fields to snapbox (rust-lang/cargo#14170) - Pass rustflags to artifacts built with implicit targets when using target-applies-to-host (rust-lang/cargo#13900) - test: Migrate network tests to snapbox (rust-lang/cargo#14187) - test: migrate some files to snapbox (rust-lang/cargo#14113) - test: Auto-redact `... after last build at ...`; Migrate `freshness` to Snapbox (rust-lang/cargo#14161) - chore: fix some typos (rust-lang/cargo#14182) - fix: improve message for inactive weak optional feature with edition2024 through unused dep collection (rust-lang/cargo#14026) - test：migrate `doc/directory/docscrape` to snapbox (rust-lang/cargo#14171) - test: Migrate git_auth to snapbox (rust-lang/cargo#14172)

Fix passing of links-overrides with target-applies-to-host and an implicit target ### What does this PR try to resolve? This fixes the link-overrides half of #14195, both the panic, and the fact that the field is being discarded, the latter of which caused the former as discussed in [the issue](#14195 (comment)). It does so following the blueprint laid out in #13900 - which is also in my opinion the current best summary of the broader context. ### How should we test and review this PR? For reviewing, comparing to the changes in #13900 might be useful. ### Additional information I'm pushing a PR for the other half of #14195 simultaneously. I thought it better to keep the PRs small since they're independent, though if merged simultaneously there will be a conflict over the ordering of fields in `Unit`.

Use `Rc` instead of `Arc` for storing rustflags # What does this PR try to resolve? There's no reason that these reference counted pointers (which I introduced in #13900) need to be atomic, so let's use non-atomic pointers. # Additional information First noticed by `@weihanglo` [here](#14205 (comment)). r? `@weihanglo`

rustbot assigned ehuss May 11, 2024

rustbot added the Z-target-applies-to-host Nightly: target-applies-to-host label May 11, 2024

gmorenz force-pushed the target_applies_to_host_rustflags branch from 7494b08 to ba2c5c2 Compare May 13, 2024 23:26

weihanglo reviewed Jul 1, 2024

View reviewed changes

gmorenz force-pushed the target_applies_to_host_rustflags branch from ba2c5c2 to b8ce338 Compare July 1, 2024 23:44

gmorenz force-pushed the target_applies_to_host_rustflags branch from b8ce338 to 2d693d5 Compare July 2, 2024 02:00

gmorenz force-pushed the target_applies_to_host_rustflags branch from 2d693d5 to 1f6de2e Compare July 2, 2024 02:15

weihanglo approved these changes Jul 4, 2024

View reviewed changes

gmorenz added 3 commits July 4, 2024 14:30

Add test verifying behavior in rust-lang#10744

a547d19

Pass rustflags to artifacts built with implicit targets when using ta…

07c9f77

…rget-applies-to-host

Pass rustdocflags like rustflags

5be8044

gmorenz force-pushed the target_applies_to_host_rustflags branch from 1f6de2e to 5be8044 Compare July 4, 2024 18:33

weihanglo approved these changes Jul 4, 2024

View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2024

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

bors merged commit 8457356 into rust-lang:master Jul 4, 2024
22 checks passed

bors mentioned this pull request Jul 4, 2024

feat: implement RFC 3553 to add SBOM support #13709

Open

gmorenz mentioned this pull request Jul 5, 2024

Linker and links are ignored with target-applies-to-host and no --target flag #14195

Open

gmorenz deleted the target_applies_to_host_rustflags branch July 5, 2024 18:37

weihanglo mentioned this pull request Jul 5, 2024

Update cargo rust-lang/rust#127385

Merged

gmorenz mentioned this pull request Jul 6, 2024

Add rustdocflags to Unit's Debug impl #14201

Merged

This was referenced Jul 6, 2024

Fix passing of links-overrides with target-applies-to-host and an implicit target #14205

Merged

Fix passing of linker with target-applies-to-host and an implicit target #14206

Open

rustbot added this to the 1.81.0 milestone Jul 7, 2024

jameshilliard mentioned this pull request Jul 19, 2024

support generic target tables and env variables #9603

Closed

gmorenz mentioned this pull request Jul 20, 2024

Use Rc instead of Arc for storing rustflags #14273

Merged

weihanglo mentioned this pull request Aug 1, 2024

warning: output filename collision. #14253

Closed

erikjohnston mentioned this pull request Aug 13, 2024

Regression build script output collision when using unstable target-applies-to-host and debug=0 #14394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass rustflags to artifacts built with implicit targets when using target-applies-to-host #13900

Pass rustflags to artifacts built with implicit targets when using target-applies-to-host #13900

gmorenz commented May 11, 2024 •

edited

Loading

rustbot commented May 11, 2024

gmorenz commented May 11, 2024 •

edited

Loading

gmorenz commented May 11, 2024 •

edited

Loading

ehuss commented May 13, 2024

gmorenz commented May 13, 2024

weihanglo left a comment

weihanglo Jul 1, 2024

gmorenz Jul 2, 2024

weihanglo Jul 4, 2024

gmorenz commented Jul 2, 2024

gmorenz commented Jul 2, 2024

gmorenz commented Jul 2, 2024

weihanglo Jul 4, 2024

gmorenz Jul 4, 2024

weihanglo Jul 4, 2024

weihanglo commented Jul 4, 2024

bors commented Jul 4, 2024

bors commented Jul 4, 2024

bors commented Jul 4, 2024

Pass rustflags to artifacts built with implicit targets when using target-applies-to-host #13900

Pass rustflags to artifacts built with implicit targets when using target-applies-to-host #13900

Conversation

gmorenz commented May 11, 2024 • edited Loading

What this PR does

How it is implemented

Related PRs, comments and issues

rustbot commented May 11, 2024

gmorenz commented May 11, 2024 • edited Loading

gmorenz commented May 11, 2024 • edited Loading

ehuss commented May 13, 2024

gmorenz commented May 13, 2024

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Jul 1, 2024

Choose a reason for hiding this comment

gmorenz Jul 2, 2024

Choose a reason for hiding this comment

weihanglo Jul 4, 2024

Choose a reason for hiding this comment

gmorenz commented Jul 2, 2024

gmorenz commented Jul 2, 2024

gmorenz commented Jul 2, 2024

weihanglo Jul 4, 2024

Choose a reason for hiding this comment

gmorenz Jul 4, 2024

Choose a reason for hiding this comment

weihanglo Jul 4, 2024

Choose a reason for hiding this comment

weihanglo commented Jul 4, 2024

bors commented Jul 4, 2024

bors commented Jul 4, 2024

bors commented Jul 4, 2024

gmorenz commented May 11, 2024 •

edited

Loading

gmorenz commented May 11, 2024 •

edited

Loading

gmorenz commented May 11, 2024 •

edited

Loading