-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Stabilize target-applies-to-host feature. #9753
base: master
Are you sure you want to change the base?
Stabilize target-applies-to-host feature. #9753
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
0a3c506
to
d5ebab6
Compare
#9322 is quite sprawling and I may not have also made this clear enough, but I personally do not think that this is a candidate for stabilization until the issue fixed in #9634 is resolved. Otherwise this flag I don't believe works with |
I think it works well enough for cross builds since the host build always ends up being used with a non-triple target directory and the target with the triple target directory, they shouldn't really conflict there at least. |
FYI I've integrated the |
I, uh, stand by my previous comment. I don't dispute that the feature works for |
What issue are we hitting there? Shared dependencies conflicting? |
☔ The latest upstream changes (presumably #9613) made this pull request unmergeable. Please resolve the merge conflicts. |
9c61466
to
e2b82bf
Compare
Hi, thank you to everyone here for working on this feature. What is the planned behavior for |
I think it eventually was supposed to be to have the target config only to apply to target artifact builds, currently they apply to It seems progress has stalled a bit here however since it seems nobody doing review really has all that good of an understanding of this area of the codebase.
Are you setting both
No, I don't think that makes much sense, the target |
Any progress on this? We rely on this feature for cross compiling and I do not feel comfortable with having to set |
IMO setting __CARGO_TEST_CHANNEL_OVERRIDE_DO_NOT_USE_THIS is probably safer, since it's only picked up/used in essentially one place(which is sufficient to enable this one nightly only feature we need) while This seems to have fixed things for us: Be aware that hacks to enable nightly features like this are likely to break, we can manage this easily since we pin our rust toolchain versions(we do this with our non-rust toolchains as well to various degrees) and have auto-builders that would likely catch any related regressions quickly, we just really don't want to be pinning rust/cargo toolchains to nightly toolchains since we really only want to enable stable features(this one is just not really optional in practice as even basic x86_64 cross compilation breaks without it). |
FWIW, I also ran out of workarounds a month ago after I wanted to upgrade something (either Rust/Cargo or one of the couple Rust libraries I still haven't ported away from... I don't remember the situation, but it involved yet another place where flags were being incorrectly applied to the wrong part of the build process) and have been able to switch to this feature and it is working great (and I will note that even using __CARGO_TEST_CHANNEL_OVERRIDE_DO_NOT_USE_THIS this is way more stable and less likely to break than all of my prior hacks that were required to make Rust even semi-usable for serious compilation; I also, though, have somewhat controlled toolchain versions). OrchidTechnologies/orchid@3ced403 |
☔ The latest upstream changes (presumably #10395) made this pull request unmergeable. Please resolve the merge conflicts. |
c3f3ef8
to
a5f2401
Compare
rebased |
Thanks for the PR, but I'm going to be stepping down from the Cargo team so I'm going to un-assign myself from this. The Cargo team will help review this when they get a chance. |
☔ The latest upstream changes (presumably #10868) made this pull request unmergeable. Please resolve the merge conflicts. |
a5f2401
to
0cc2206
Compare
rebased |
0cc2206
to
3c7652c
Compare
@joshtriplett Does this look good to merge? It would be nice if cross compilation build systems could drop hacks like Having unstable env variables as a hard requirement for a functional cross compilation environment isn't exactly ideal. |
3c7652c
to
6b0988c
Compare
☔ The latest upstream changes (presumably #13409) made this pull request unmergeable. Please resolve the merge conflicts. |
The |
I am building a static binary (not interpreted by I believe that if this is stabilized before fixing this issue, it would be backwards incompatible to change. Moreover it would move the situation from we have two ways things are built ( |
@rustbot label: +Z-target-applies-to-host |
There's some important discussion about eventual stabilization in this review comment, and the discussion on the main thread of that PR after the review. To summarize: Should Because of complexity Alex Crichton (not tagging as they have chosen to step down from this) and @jonhoo appear to have been leaning towards "stabilize directly to |
@gmorenz It isn't clear to me, in the three-state idea, why anyone would ever use =true: new code should use =false, and some older scripts might temporarily need =mixed, but no one should ever be using =true? I can see renaming the flag, but it seems kind of silly and will just cause even more churn, given that I would expect that no one should ever opt into that behavior: at least with this name, the few people who might care are likely to find these issues and the now years of discussion on various forums with people talking about this flag as fixing the current broken behavior and explaining why you might want to turn this legacy behavior back on. I would then document the flag, explaining merely that =true opts into quickly/legacy behavior. I frankly would not try to document that behavior, as it was, is, and will continue to be nonsensical and the goal of keeping it in the codebase only makes sense for people whose code already happened to work before, and those people do not need documentation of the behavior that is already confusing: they just need to be told to stop relying on it. I would also state that the flag might go away in the future, and so no one should start writing new code that uses =true or ever consider turning it on unless they are doing so to temporarily work around an issue. At that point, the name seems almost irrelevant. As far as I can tell, all of this discussion about a tri-state flag happened due to a misunderstanding of the intended future of the flag, not due to any real interest in actually having =true be a supported configuration. As such, it seems silly to implement the tri-state feature merely to make the name make more sense, but it also seems silly to lose sleep over the name given that the flag is only to be used in a way where the name can be confusing in exceptional scenarios by people who already rely on whatever that behavior might be, and the flag is coming into existence with the goal of then rapidly phasing out. Remember: in its current state, cargo does not work! The most common way for serious integrators to build software is using host==target cross-compiling, and so an increasingly-large number of projects are turning to the crazy workaround of using the DO_NOT_USE_THIS nightly override env and are already pushing this flag into production as without it cargo simply doesn't work. It would behoove everyone to push this flag--whatever the default--sooner rather than later, as it isn't clear to me that the people whose code right now could break even if the default were changed should matter more than the people who are using this flag to fix the bug. |
@saurik I'm sympathetic to that argument with regards to As currently implemented it seems to me that I've started poking at the cargo code in an attempt to fix the flag, but no promises. Edit: Apparently rust-lang/rust wanted |
@saurik - Correcting my previous comment, since that build script is always passing a What is the difference in behavior that you see by enabling the flag in that context? Edit: It's that linker arguments isn't used to build host artifacts |
…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)
☔ The latest upstream changes (possibly 081d7ba) made this pull request unmergeable. Please resolve the merge conflicts. |
Details: #9453, #9322