Use cross compile style target/host isolation for all builds. #9634
Conversation
|
r? @Eh2406 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
jameshilliard
force-pushed
the
all-cross
branch
7 times, most recently
from
ac876ec to
892635c
Compare
| "\ | ||
| [COMPILING] foo v0.5.0 ([..]) | ||
| [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] | ||
| [RUNNING] `target/debug/foo[EXE]` | ||
| [RUNNING] `target/{}/debug/foo[EXE]` |
There was a problem hiding this comment.
Does it still get copied to target/debug/foo? That is where pretty much everyone expects it to be in case of not cross-compiling. I think the examples and tests dir should also be copied. The deps and build dirs are more implementation details I think, so they don't have to be copied.
There was a problem hiding this comment.
Does it still get copied to
target/debug/foo?
No
That is where pretty much everyone expects it to be in case of not cross-compiling. I think the examples and tests dir should also be copied.
Based on previous discussions the goal is to ensure that non-cross compilation cases are effectively identical to cross compilation cases, so not sure this is desirable.
The deps and build dirs are more implementation details I think, so they don't have to be copied.
Isn't that also the case here? This isn't really an installation directory from my understanding so a specific path here generally shouldn't be important AFAIU.
There was a problem hiding this comment.
When invoking cargo through a different build system or even when automatically building, you have to rely on this. cargo install can't be used for staticlibs and dylibs. It can be used for executables, but I don't think many people do in the context of building programs to be used outside of the current system.
There was a problem hiding this comment.
When invoking cargo through a different build system or even when automatically building, you have to rely on this.
Shouldn't one set an installation directory there instead? That way it should still work either way. Here's an example in buildroot of how this sort of change(still a WIP) is being made.
There was a problem hiding this comment.
That doesn't work for libraries (staticlib/cdylib). Those can't be installed.
There was a problem hiding this comment.
Those can't be installed.
Hmm, seems that's a known cargo bug, we probably should fix that. Normally meta cross build systems(like buildroot) use standard install features from the packages specific build systems(such as autotools or meson) to install libraries to a staging directory for this sort of thing, cargo really should provide an equivalent feature there.
There was a problem hiding this comment.
Generally "staticlib/cdylib" libraries like this are also expected to provide pkg-config .pc files when installed as well so that other software knows how to link against them properly, so cargo probably needs a way to generate those automatically as well when installing.
|
Thanks for the PR for this, but this is unfortunately a pretty tricky change to make. As you've noticed lots of paths need to change and this isn't something we can do lightly. We, fundamentally, cannot simply change Cargo's output paths to be different from This is a pretty difficult change to make and one that we've been meaning to get around to for a very long time but haven't had the chance to do so yet because of how involved it is. There's a lot of issues with the way Cargo stores files today we'd like to fix, and many issues are inter-related with changing output paths like this as well. Unfortunately at this time I don't think any of us on the Cargo team have created a plan about how to fix the issue you're fixing here. We have various thoughts here and there but no one's put together a comprehensive plan about how to do this. I don't think that we can do this off-hand though through a large-ish PR, this is a change that needs careful planning to ensure we don't break the ecosystem and we can migrate existing use cases accordingly. |
|
Maybe a first step would be to move everything that needs to be compiled for the host even when cross-compiling to a new |
Maybe we should just create a symlink or something to the cross style paths for backwards compat?
This def ended up being a bit complex, seems a lot of internal logic was relying on the host target hack.
Yeah, not sure how best to handle this. |
Well I split the host side into target specific directories as well in order to handle some potential edge cases, for instance if one needs to use qemu wrappers or something for multiple different host targets. |
|
The host triple is included in the |
In the target folder arch path or the end file itself as well? Not familiar with how all that works myself but using different filenames like that doesn't seem to make much sense. |
|
|
|
@jameshilliard if you're interested in going the whole 9 yards to fix this issue, I would recommend that we schedule some time for interested folks on the Cargo team to give a brain dump of the various issues. The conclusion of such a brain dump may also be that this isn't the right time and there's other things to fix first, but we can try to enumerate thoughs. Designing through iteration and comments on a PR I don't think is the right way to approach this though, the underyling change here is big enough I think we'll want to plan this with others to make sure that everyone's on board first. |
Yeah, probably a good idea.
Yeah, the best ordering of these changes isn't all that clear, there's a lot of assumptions the code seems to make due to the host target hacks for native build.
Yeah, this first round was mostly an experiment to see what is involved and what parts of the code-base this sort of change would impact. It probably needs a few refactoring rounds at a minimum beforehand. |
|
Ok! Would you be with a video meeting to give a brain dump about this space? |
sure |
|
Ok we've got a regular Cargo meeting scheduled for tomorrow so I'll ask folks to see if there's other interest, and we can schedule afterwards. |
|
☔ The latest upstream changes (presumably #9677) made this pull request unmergeable. Please resolve the merge conflicts. |
Ok, roughly what time would that be? |
|
Oh I just want to gather who's interested today, we'll schedule a time afterwards. |
|
Can you ping me on zulip so we can schedule a time? |
|
☔ The latest upstream changes (presumably #9675) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@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)
|
☔ The latest upstream changes (possibly 081d7ba) made this pull request unmergeable. Please resolve the merge conflicts. |
Note: I disabled a few tests that I wasn't sure how to fix but I think I got things working for the most part. This is still rather rough and needs a good bit of cleanup.