Enable propagating host rustflags to build scripts #10395
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
PR failures are due to new clap 3.1.0 deprecations combined with |
The tests in question all tail to even build the build script, so the contents of `main` don't matter, and make the tests seem more complex than they really are.
|
/cc @jameshilliard too since you contributed the original feature :) |
Why did you remove the stable format? The only reason you have to specify |
Target configs are always triple specific, so setting the config flag |
There was a problem hiding this comment.
It's intentional that target-applies-to-host is a top level configuration setting as we need that for many cross compiling scenarios since we want to be able to unconditionally set target-applies-to-host = false for all targets being built. We really don't want the target-applies-to-host = false behavior to be gated behind a nightly config option as doing so forces us to set hacks like __CARGO_TEST_CHANNEL_OVERRIDE_DO_NOT_USE_THIS="nightly" when cross compiling with a stable toolchain. The reason for the stable flag as opposed to just changing the default is to allow for gradual transition away from the broken target-applies-to-host = true behavior without causing issues for users that currently depend on that. There's potentially some users that may want to retain target-applies-to-host = true behavior as it would allow for smaller configs in the event they want target and host configs to be the same.
|
I gave the original discussion for the feature another reading, and I agree with you that having the top-level configuration option is probably the way to go. It has a similar flavor to The reason this is hairy is because the behavior of Cargo today is inconsistent. Only I'm personally also not in favor of having |
jonhoo
force-pushed
the
fix-10206
branch
3 times, most recently
from
b9273b3
to
2cc438c
Compare
This patch implements more complete logic for applying rustflags to build scripts and other host artifacts. In the default configuration, it only makes build scripts (and plugins and whatnot) pick up on `rustflags` from `[host]`, which fixes rust-lang#10206 but otherwise preserves existing behavior in all its inconsistent glory. The same is the case if `target-applies-to-host` is explicitly set to `false`. When `target-applies-to-host` is explicitly set to `true`, rustflags will start to be applied in the same way that `linker` and `runner` are today -- namely they'll be applied from `[target.<host triple>]` and from `RUSTFLAGS`/`build.rustflags` if `--target <host triple>` is given.
Yeah, it's def a hairy mess.
Ugh, that's pretty bad, I only tested the linkers myself since that's what was causing build errors for us. That should probably just be fixed outright as it's clearly a bug.
IMO the best behavior when unset would be to apply target triples to host if So defaults like this for cases where
I assume you mean when set to
Well the main reason for having it change the default was mostly to minimize the amount of combinations(branch conditions essentially) to test, I only wanted the host table feature to be tested with the new defaults for |
I don't think we can just fix it, sadly, as that would likely break a bunch of cross-compiling users. Hence the "legacy" mode this PR implements. I think we almost certainly have to preserve backwards compatibility here, or cause a bunch of unnecessary churn for users. Though if the Cargo team is okay with the breakage, I'm okay with removing the legacy mode. I just assumed they wouldn't be.
I really dislike dynamic defaults, since they tend to be hard to discover and hard to debug. I'd much rather (as this PR implements) have
Then, come next edition, we make
Sorry, yes,
The number of combinations should be the same, since |
We're talking about fixing the
I'm highly skeptical changing the But this Build scripts typically don't usually need custom build flags at all to function(we don't even need/use the host-config feature in buildroot, we only use the target-applies-to-host feature to override the existing default buggy Arguably anyone cross compiling who has a config that changing this default breaks has a buggy config already, they just happen to have things working by accident, either way the point of the From my understanding the Cargo team is fine with this approach provided there is a transition period before changing the default(although I personally don't actually think this is necessary since fixing the buggy behavior is only likely to break obscure configurations anyways).
I mean, if the goal is to have
What are you trying to do here? Requiring a config option that's intended to be stabilized in order to enable setting a nightly gate for a feature also intended to be stabilized makes no sense to me. Setting
Either way this seems to be something separate from the
This doesn't make sense since
This sounds fine.
This sounds fine.
This is extremely confusing, AFAIU isn't not even possible to set host artifact specific The report #10206 that this fixes seems to be entirely referencing a bug in the nightly Is there also a different bug with
I think the important part is just changing the default before we stabilize the
We want to test the |
|
Ok I think I understand what you mean by the tri-state behavior, I never fully understood that but seeing the test cases now and how they're working clarifies this for me at least. My main priority in reviewing all of this rustflags/target/host related stuff is to ensure that breakage is quanitified and either mitigated or deemed acceptable. In that sense I'm pretty confident that this PR doesn't change the current behavior, which is good in the sense that it mitigates possible breakage. For the future, though, I think we'll want the tri-state idea but not necessarily super-well-supported. I don't think anyone will really argue that today's behavior is correct and should be codified into Cargo for all time. Today's behavior is the amalgamation of growing over years as PRs touched various portions and we as Cargo reviewers didn't do a great job of seeing how everything fits together. What's done is done though, we move past it. This I think means that the stabilization story for Basically what I'm trying to do is to chart a course from where we are to where we want to be with minimal breakage to the ecosystem. Ideally this would mean that at any time if someone hits a breakage they can flip a switch to go back to some previous behavior or something like that. I've been working under the assumption that this is possible but it may simply not be possible. This may all simply mean that breakage is unavoidable and if the breakage doesn't work for someone all we can say is "sorry you can hold back on upgrading Cargo for a bit while the breakage is worked through". |
|
I completely agree — that matches my thinking on the subject. I think the right question then becomes is "how hard is mitigation". If In any case, I also agree with you that this PR doesn't change current behavior beyond fixing #10206, and so should be possible to safely land. |
|
@bors: r+ Ok well we can get all this sorted out during the stabilization of the features here. I agree this is fine to go for now. Personally I continue to feel that these unstable features are also gated on the fact that |
|
📌 Commit 56db829 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
11 changes in d6cdde584a1f15ea086bae922e20fd27f7165431..3d6970d50e30e797b8e26b2b9b1bdf92dc381f34 2022-02-22 19:55:51 +0000 to 2022-02-28 19:29:07 +0000: - rust-lang/cargo#10395 - rust-lang/cargo#10425 - rust-lang/cargo#10428 - rust-lang/cargo#10388 - rust-lang/cargo#10167 - rust-lang/cargo#10429 - rust-lang/cargo#10426 - rust-lang/cargo#10372 - rust-lang/cargo#10420 - rust-lang/cargo#10416 - rust-lang/cargo#10417
Update cargo 11 changes in d6cdde584a1f15ea086bae922e20fd27f7165431..3d6970d50e30e797b8e26b2b9b1bdf92dc381f34 2022-02-22 19:55:51 +0000 to 2022-02-28 19:29:07 +0000: - rust-lang/cargo#10395 - rust-lang/cargo#10425 - rust-lang/cargo#10428 - rust-lang/cargo#10388 - rust-lang/cargo#10167 - rust-lang/cargo#10429 - rust-lang/cargo#10426 - rust-lang/cargo#10372 - rust-lang/cargo#10420 - rust-lang/cargo#10416 - rust-lang/cargo#10417
|
It's a good point. I wonder if the way to go here is to take a lesson from rust-lang/cc-rs#9 and add support for |
|
Maybe? That's something I think needs discussion outside of just this PR and input from others as well. |
|
Oh, yes, definitely. I just don't see a way around doing that, or something like it, if we want |
In Cargo, it is quite typical for "build scripts" to be written in Rust and therefore they need to be compiled as part of the overall build. In cross-compilation, that means a mixed host and target build. Unfortunately, by default Cargo makes no distinction between the RUSTFLAGS used for the host and the target. There is, however, an unstable feature to make this distinction [1][2]. We already have CARGO_TARGET_APPLIES_TO_HOST="false". This makes sure that any configuration that we make for the target doesn't automatically apply to the host as well. However, this only applies for per-target configuration, for example the setting of "cc" in the config.toml generated by package/rust/rust.mk. Flags that are passed with RUSTFLAGS still apply to both host and target. Therefore, we need to use the CARGO_TARGET_<tuple>_RUSTFLAGS environment variable instead of plain RUSTFLAGS. This, however, doesn't allow us to specify flags that apply only to the host. We could use CARGO_TARGET_<hosttuple>_RUSTFLAGS for that, but that doesn't work in case the host and target tuple are the same. For this, we need another unstable feature, enabled with CARGO_UNSTABLE_HOST_CONFIG="true". With this enabled, we can specify flags that apply only for the host build using CARGO_HOST_RUSTFLAGS. Currently, we don't have any such flags, but we really should: we should pass the proper link flags to point to $(HOST_DIR)/lib. Therefore, add CARGO_HOST_RUSTFLAGS doing exactly that. [1] https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#host-config [2] rust-lang/cargo#10395 Signed-off-by: James Hilliard <james.hilliard1@gmail.com> Signed-off-by: Arnout Vandecappelle <arnout@mind.be>
In Cargo, it is quite typical for "build scripts" to be written in Rust and therefore they need to be compiled as part of the overall build. In cross-compilation, that means a mixed host and target build. Unfortunately, by default Cargo makes no distinction between the RUSTFLAGS used for the host and the target. There is, however, an unstable feature to make this distinction [1][2]. We already have CARGO_TARGET_APPLIES_TO_HOST="false". This makes sure that any configuration that we make for the target doesn't automatically apply to the host as well. However, this only applies for per-target configuration, for example the setting of "cc" in the config.toml generated by package/rust/rust.mk. Flags that are passed with RUSTFLAGS still apply to both host and target. Therefore, we need to use the CARGO_TARGET_<tuple>_RUSTFLAGS environment variable instead of plain RUSTFLAGS. This, however, doesn't allow us to specify flags that apply only to the host. We could use CARGO_TARGET_<hosttuple>_RUSTFLAGS for that, but that doesn't work in case the host and target tuple are the same. For this, we need another unstable feature, enabled with CARGO_UNSTABLE_HOST_CONFIG="true". With this enabled, we can specify flags that apply only for the host build using CARGO_HOST_RUSTFLAGS. Currently, we don't have any such flags, but we really should: we should pass the proper link flags to point to $(HOST_DIR)/lib. Therefore, add CARGO_HOST_RUSTFLAGS doing exactly that. [1] https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#host-config [2] rust-lang/cargo#10395 Signed-off-by: James Hilliard <james.hilliard1@gmail.com> Signed-off-by: Arnout Vandecappelle <arnout@mind.be>
In Cargo, it is quite typical for "build scripts" to be written in Rust and therefore they need to be compiled as part of the overall build. In cross-compilation, that means a mixed host and target build. Unfortunately, by default Cargo makes no distinction between the RUSTFLAGS used for the host and the target. There is, however, an unstable feature to make this distinction [1][2]. We already have CARGO_TARGET_APPLIES_TO_HOST="false". This makes sure that any configuration that we make for the target doesn't automatically apply to the host as well. However, this only applies for per-target configuration, for example the setting of "cc" in the config.toml generated by package/rust/rust.mk. Flags that are passed with RUSTFLAGS still apply to both host and target. Therefore, we need to use the CARGO_TARGET_<tuple>_RUSTFLAGS environment variable instead of plain RUSTFLAGS. This, however, doesn't allow us to specify flags that apply only to the host. We could use CARGO_TARGET_<hosttuple>_RUSTFLAGS for that, but that doesn't work in case the host and target tuple are the same. For this, we need another unstable feature, enabled with CARGO_UNSTABLE_HOST_CONFIG="true". With this enabled, we can specify flags that apply only for the host build using CARGO_HOST_RUSTFLAGS. Currently, we don't have any such flags, but we really should: we should pass the proper link flags to point to $(HOST_DIR)/lib. Therefore, add CARGO_HOST_RUSTFLAGS doing exactly that. [1] https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#host-config [2] rust-lang/cargo#10395 Signed-off-by: James Hilliard <james.hilliard1@gmail.com> Signed-off-by: Arnout Vandecappelle <arnout@mind.be> (cherry picked from commit b40a2cc) Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
In Cargo, it is quite typical for "build scripts" to be written in Rust and therefore they need to be compiled as part of the overall build. In cross-compilation, that means a mixed host and target build. Unfortunately, by default Cargo makes no distinction between the RUSTFLAGS used for the host and the target. There is, however, an unstable feature to make this distinction [1][2]. We already have CARGO_TARGET_APPLIES_TO_HOST="false". This makes sure that any configuration that we make for the target doesn't automatically apply to the host as well. However, this only applies for per-target configuration, for example the setting of "cc" in the config.toml generated by package/rust/rust.mk. Flags that are passed with RUSTFLAGS still apply to both host and target. Therefore, we need to use the CARGO_TARGET_<tuple>_RUSTFLAGS environment variable instead of plain RUSTFLAGS. This, however, doesn't allow us to specify flags that apply only to the host. We could use CARGO_TARGET_<hosttuple>_RUSTFLAGS for that, but that doesn't work in case the host and target tuple are the same. For this, we need another unstable feature, enabled with CARGO_UNSTABLE_HOST_CONFIG="true". With this enabled, we can specify flags that apply only for the host build using CARGO_HOST_RUSTFLAGS. Currently, we don't have any such flags, but we really should: we should pass the proper link flags to point to $(HOST_DIR)/lib. Therefore, add CARGO_HOST_RUSTFLAGS doing exactly that. [1] https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#host-config [2] rust-lang/cargo#10395 Signed-off-by: James Hilliard <james.hilliard1@gmail.com> Signed-off-by: Arnout Vandecappelle <arnout@mind.be>
|
@rustbot label: +Z-target-applies-to-host |
…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)
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-hostfeature (#9453), and adds the unstabletarget-applies-to-host-kindflag to mirrortarget-applies-to-hostfor 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-kindenabled.Additional information
target-applies-to-hostso 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.target-applies-to-host-kindis never behavior we want users to turn on, and that it should therefore simply be removed and hard-coded as beingfalse.target-applies-to-host-kindshould interact with-Zmultitarget. If, for example,requested_kinds = [HostTarget, SomeOtherTarget]andkind.is_host(), shouldRUSTFLAGStake effect or not? For the time being I've just hard-coded the behavior for single targets, and the answer would be "no".