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

Tracking Issue for target-applies-to-host #9453

Open
1 task done
jameshilliard opened this issue May 3, 2021 · 3 comments
Open
1 task done

Tracking Issue for target-applies-to-host #9453

jameshilliard opened this issue May 3, 2021 · 3 comments
Labels
C-tracking-issue Category: A tracking issue for something unstable. S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. Z-target-applies-to-host Nightly: target-applies-to-host

Comments

@jameshilliard
Copy link
Contributor

jameshilliard commented May 3, 2021

Summary

Original issue: #3349
Implementation: #9322
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#target-applies-to-host
Issues: Z-target-applies-to-host Nightly: target-applies-to-host

The target-applies-to-host key in a config file can be used set the desired behavior for passing target config flags to build scripts.

Unresolved Issues

@jameshilliard jameshilliard added the C-tracking-issue Category: A tracking issue for something unstable. label May 3, 2021
@ehuss ehuss added the Z-target-applies-to-host Nightly: target-applies-to-host label Jan 17, 2022
bors added a commit that referenced this issue Feb 28, 2022
Enable propagating host rustflags to build scripts

### What does this PR try to resolve?

This PR primarily fixes #10206, but in doing so it also slightly modifies the interface for the unstable `target-applies-to-host` feature (#9453), and adds the unstable `target-applies-to-host-kind` flag to mirror `target-applies-to-host` for build scripts and other host artifacts.

The commit messages have more in-depth discussion.

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

The test case from #10206 now works rather than producing an error. It has also been added a regression test case. A few additional test cases have also been added to handle the expected behavior around rustflags for build scripts with and without `target-applies-to-host-kind` enabled.

### Additional information

1. This changes the interface for `target-applies-to-host` so that it does not need to be specified twice to be used. And it can still be set through configuration files using the `[unstable]` table. However, we may(?) want to pick a stable format for in-file configuration of this setting unless we intend for it to only ever be a command-line flag.
2. It may be that `target-applies-to-host-kind` is never behavior we want users to turn on, and that it should therefore simply be removed and hard-coded as being `false`.
3. It's not entirely clear how `target-applies-to-host-kind` should interact with `-Zmultitarget`. If, for example, `requested_kinds = [HostTarget, SomeOtherTarget]` and `kind.is_host()`, should `RUSTFLAGS` take effect or not? For the time being I've just hard-coded the behavior for single targets, and the answer would be "no".
jameshilliard added a commit to jameshilliard/cargo that referenced this issue Mar 1, 2022
@rich-g
Copy link

rich-g commented Jun 17, 2022

Any chance of stabilizing this?

@rich-g
Copy link

rich-g commented Oct 19, 2022

Just bumping this again. Would really like this stabilized so I can get off of nightly builds. :)

@jameshilliard
Copy link
Contributor Author

Just bumping this again. Would really like this stabilized so I can get off of nightly builds. :)

Not sure what status of stabilizing this is, but if you want to use it on stable just set this in your env.

jameshilliard added a commit to jameshilliard/cargo that referenced this issue Oct 21, 2022
jameshilliard added a commit to jameshilliard/cargo that referenced this issue Dec 3, 2022
jameshilliard added a commit to jameshilliard/cargo that referenced this issue Apr 11, 2023
@ehuss ehuss added S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. labels Apr 25, 2023
@epage epage moved this to Unstable, baking in Cargo Roadmap Sep 5, 2023
bors added a commit that referenced this issue Jul 4, 2024
…ihanglo

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

## 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](https://github.com/gmorenz/target_application_to_host_matrix) 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<br/>Flag passed to shared dep from bin<br/>Linker passed to bin<br/><br/>Flag passed to proc macro<br/>Flag passed to shared dep from proc macro<br/>Linker passed to proc macro<br/><br/>Built with 5 invocations<br/>Without rustflags, built with 5 invocations<br/> | **Flag passed to bin**<br/>**Flag passed to shared dep from bin**<br/>Linker not passed to bin<br/><br/>Flag not passed to proc macro<br/>Flag not passed to shared dep from proc macro<br/>Linker not passed to proc macro<br/><br/>**Built with 6 invocations**<br/>Without rustflags, built with 5 invocations<br/> |
| --target x86_64-unknown-linux-gnu | Flag passed to bin<br/>Flag passed to shared dep from bin<br/>Linker passed to bin<br/><br/>Flag not passed to proc macro<br/>Flag not passed to shared dep from proc macro<br/>Linker passed to proc macro<br/><br/>Built with 6 invocations<br/>Without rustflags, built with 6 invocations<br/> | Flag passed to bin<br/>Flag passed to shared dep from bin<br/>Linker passed to bin<br/><br/>Flag not passed to proc macro<br/>Flag not passed to shared dep from proc macro<br/>Linker not passed to proc macro<br/><br/>Built with 6 invocations<br/>Without rustflags, built with 6 invocations<br/> |
| --target i686-unknown-linux-gnu | Flag passed to bin<br/>Flag passed to shared dep from bin<br/>Linker not passed to bin<br/><br/>Flag not passed to proc macro<br/>Flag not passed to shared dep from proc macro<br/>Linker passed to proc macro<br/><br/>Built with 6 invocations<br/>Without rustflags, built with 6 invocations<br/> | Flag passed to bin<br/>Flag passed to shared dep from bin<br/>Linker not passed to bin<br/><br/>Flag not passed to proc macro<br/>Flag not passed to shared dep from proc macro<br/>Linker not passed to proc macro<br/><br/>Built with 6 invocations<br/>Without rustflags, built with 6 invocations<br/> |

 ## 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](#10395 (comment)) [2](#9753 (comment))

[My own comment describing exactly how I ran into this](#9753 (comment))

[Documentation for the feature](https://doc.rust-lang.org/cargo/reference/unstable.html#target-applies-to-host)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for something unstable. S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. Z-target-applies-to-host Nightly: target-applies-to-host
Projects
Status: Unstable, baking
Development

No branches or pull requests

3 participants