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

Configure hosts separately from targets when --target is specified. #9322

Merged
merged 12 commits into from Jun 1, 2021

Conversation

jameshilliard
Copy link
Contributor

@jameshilliard jameshilliard commented Apr 2, 2021

This prevents target configs from accidentally being picked up when cross compiling from hosts that have the same architecture as their targets.

closes #3349

@rust-highfive
Copy link

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) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 2, 2021
@jameshilliard jameshilliard force-pushed the split-host branch 4 times, most recently from 4875137 to 60f721b Compare April 2, 2021 05:21
@jameshilliard jameshilliard force-pushed the split-host branch 2 times, most recently from 83b5f50 to 1597f84 Compare April 2, 2021 15:47
@jameshilliard jameshilliard changed the title Allow configuration of hosts separately from targets. Configure hosts separately from targets when --target is specified. Apr 2, 2021
@joshtriplett
Copy link
Member

joshtriplett commented Apr 6, 2021

One quick bit of feedback: I don't think this should just be host. This should be host.(name of host's target triple), so that you can specify different flags/etc for different host architectures or operating systems.

If there's something that really does apply to all hosts, it could use host.* or similar.

@jameshilliard
Copy link
Contributor Author

This should be host.(name of host's target triple), so that you can specify different flags/etc for different host architectures or operating systems.

I don't see a realistic use case for having host target triples since each build host should only need one host configuration for all targets as the build scripts must run on the build host and shouldn't change depending on targets, using target triples here seems to complicate things unnecessarily.

@joshtriplett
Copy link
Member

@jameshilliard I have several use cases for that. Projects often check in .cargo/config.toml files, and users may do something similar to share their personal .cargo/config.toml files. Things like flags or linkers may vary by platform; for instance, you may want one setting for Windows hosts and another for Linux, or one setting for x86-64 and another for ARM64.

@alexcrichton
Copy link
Member

Thanks for the PR @jameshilliard! We talked about this in the Cargo meeting today for quite awhile and I'm going to try to summarize our thoughts all down in writing. As you've discovered this is a particularly large thorn in Cargo's side, and it's been there for quite some time!

Overall we generally favored this design, but one thing we're worried about is backwards compatibility. This will break any usage of [target] configuration that would apply to both the host and the target (when they're the same). Another concern that I have is that this is close but not quite to fixing some surprising behavior with cargo build. From our discussion, however, I think we've got a way forward on all this, and I'm curious to get your thoughts on it!

The changes we were thinking for this PR are:

  • Instead of host.linker we'd have host.$target.linker in the same manner as the target table today (to future-proof config used simultaneously for different hosts, e.g. on Windows).
  • Add a new flag to the target table, something like target.$target.applies-to-host which is default true (Cargo's historical default). If this flag is set to false, though, then the [host] table is consulted for host builds.
  • This lends itself well to a fix for the cargo build situation as well. With these configuration knobs in place cargo build would configure the target/host build separately if host configuration is present or target configuration which doesn't apply to the host (e.g. applies-to-host is false).
  • Once all this support has been stabilized and has sat on the stable channel for a release or so, then we'd switch the default of applies-to-host from true to false.

How does that sound? This is definitely a more long-term plan to get this fixed but the hope is that we can do this with minimal breakage and if something breaks there's recourse for users to fix it. Additionally I think this should fix all if not almost all of the issue about cargo build and cross compilation and such.

The points about "fixing" cargo build and updating its behavior with respect to the host table is a little more involved than just this PR itself. It's fine for that to be a separate PR or implemented by someone else later. Until that's all implemented though I would personally prefer to land this support unstable in Cargo because it's so closely related to those changes as well (and it seems good to evaluate/stabilize all at once).

@jameshilliard
Copy link
Contributor Author

Instead of host.linker we'd have host.$target.linker in the same manner as the target table today (to future-proof config used simultaneously for different hosts, e.g. on Windows).

I think this is unnecessarily complex, these host config flag settings aren't exactly portable in most cases so having multiple sections for different host build script targets doesn't appear to be useful.

This will break any usage of [target] configuration that would apply to both the host and the target (when they're the same).

This should behave as before as long as one doesn't manually set the target(which one generally only does when cross compiling AFAIK). The only thing I could see this breaking are some obscure edge-cases(obscure enough they aren't even tested for) where one is manually setting a target that also needs custom flags for the build script(most of the time build scripts don't need custom flags so this generally shouldn't cause any issues).

Add a new flag to the target table, something like target.$target.applies-to-host which is default true (Cargo's historical default). If this flag is set to false, though, then the [host] table is consulted for host builds.

Well it doesn't exactly unconditionally default to true here but rather it inconsistently chooses what to do based on the host and target architectures being the same, in most cases when cross compiling the default will be false as long as the target arch is different from the host. Relying on setting target.$target.applies-to-host to false would result in the existing highly inconsistent/unexpected behavior as applying target configs to host scripts is wrong/unnecessary in almost all cases, the only case where applying target flags to host build scripts is generally safe is when the host is the target(which I have retained the existing behavior for). There's no good reason IMO that the config for cross compiling for a x86_64 target from a x86_64 host should require special casing vs a config for compiling for a aarch64 target from the same x86_64 host.

Once all this support has been stabilized and has sat on the stable channel for a release or so, then we'd switch the default of applies-to-host from true to false.

I really don't think this is a good idea, currently the behavior of cargo arbitrarily applying target configurations to host scripts is objectively wrong but only breaks in a fatal way for specific configurations, and by providing a host table we ensure that in the rare case build scripts need custom flags that there is a way for them to be provided still. Since this change should not break any common use cases I think we should just default to doing the sane thing and not apply target flags to build scripts except in cases where the host is the target(target not set).

This is definitely a more long-term plan to get this fixed but the hope is that we can do this with minimal breakage and if something breaks there's recourse for users to fix it.

While this does change a default it doesn't appear likely to cause significant breakage, and with a [host] section it should be trivial to fix for those where this breaks.

The points about "fixing" cargo build and updating its behavior with respect to the host table is a little more involved than just this PR itself.

I'm not really seeing a scenario that's not covered by my approach, is there some use case I've missed here?

@jameshilliard
Copy link
Contributor Author

jameshilliard commented Apr 7, 2021

Projects often check in .cargo/config.toml files, and users may do something similar to share their personal .cargo/config.toml files. Things like flags or linkers may vary by platform; for instance, you may want one setting for Windows hosts and another for Linux, or one setting for x86-64 and another for ARM64.

Do you have an example of what one of those checked in cargo configs would look like? I'd like to see one that this change would break ideally.

Custom build script flags tied to the host architecture just seems like a bad idea for multiple reasons, it means the build output gets affected by both the build host as well as the target which I don't see as desirable in general.

Normally the build output should only be affected by the target settings, the main use case for the [host] table IMO is to give the user a way to fix any unexpected build script compile failures, I wouldn't expect that section to be used at all when cross compiling except to workaround unanticipated issues.

@saurik
Copy link

saurik commented Apr 7, 2021

I have done a lot of "complicated" criss-crossed compilation--mostly in the world of C/C++--and am one of the people who has been long affected by this Cargo issue, whilst slowly drowning in failing workarounds ;P, and I am also not sure I see the use case for host-triple specific build rules? The only thing I ever feel I need to pass that is special on the host is super high-level language confirmation flags (the only one I can think of being -std=, but I could see a use case for -funsigned-char). I generally just need my host compiler to "work at all", not be configured in some extra special way (and I thereby try to get away if at all possible with just running "cc" and passing -I/c/o, though in practice I often "care" a bit and run "gcc" or even "clang"). It could be that people are doing something very different in Rust-land with their host build steps, or there is an entire level of complexity far past where I have ever gone?

@joshtriplett
Copy link
Member

@jameshilliard
A few examples of things I'd actively like to do that would require different options depending on the target triple the host uses:

  • Use lld for performance when building on hosts where it works reliably, but not on hosts where it doesn't.
  • Use target-feature flags for host code, for build scripts that do CPU-intensive work (such as building complex data structures) and would benefit from optimization. (These are the same kinds of build scripts that motivate turning on optimizations even in debug mode.)
  • Use a specific compiler, but only on targets where that path makes sense (e.g. using a specific compiler on Windows and a different specific compiler on Linux).

Also, to clarify, I don't object to having [host] for the case of options identical on every host; I just want to make sure, when we're adding this, that it's possible to specify options specific to the architecture or OS of the host.

@saurik
Copy link

saurik commented Apr 7, 2021

@joshtriplett The first and third bullets don't actually seem like they should be checked in as files to your project as part of your cargo configuration, as that is probably going to make them attempt to get applied to a build of your project even if you are just some embedded dependency; at a "high-level", I'd argue that the person doing the build should get to make these determinations if at all possible, as these are both like, toolchain issues and not target issues. Like, to the extent to which there are faster ways of doing these builds, shouldn't that just be defaults built into cargo, or decisions made by the system toolchain distributor? (I'd even argue that is mostly true of the second one as well, though I can appreciate some stored guidance on math flags or something, but even there I find it a bit sketchy and I'm highly concerned that projects using it will become hard for me to compile.)

@bors
Copy link
Collaborator

bors commented Jun 1, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 0cecbd6 to master...

@bors bors merged commit 0cecbd6 into rust-lang:master Jun 1, 2021
@jameshilliard jameshilliard deleted the split-host branch June 1, 2021 21:03
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 3, 2021
Update cargo

10 commits in e931e4796b61de593aa1097649445e535c9c7ee0..0cecbd67323ca14a7eb6505900d0d7307b00355b
2021-05-24 16:17:27 +0000 to 2021-06-01 20:09:13 +0000
- Configure hosts separately from targets when --target is specified. (rust-lang/cargo#9322)
- Add some validation to rustc-link-arg (rust-lang/cargo#9523)
- Implement suggestions for unknown features in workspace (rust-lang/cargo#9420)
- Extract common `make_dep_path` to cargo_util (rust-lang/cargo#9529)
- Add a note about rustflags compatibility. (rust-lang/cargo#9524)
- Consolidate doc collision detection. (rust-lang/cargo#9526)
- Add `--depth` option for `cargo-tree` (rust-lang/cargo#9499)
- `cargo tree -e no-proc-macro` to hide procedural macro dependencies (rust-lang/cargo#9488)
- Update to semver 1.0.0 (rust-lang/cargo#9508)
- Update tar dependency to 0.4.35 (rust-lang/cargo#9517)
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Jun 11, 2021
buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this pull request Aug 3, 2021
To fix this bug we have to use an undocumented test variable to enable
the nightly target-applies-to-host feature so that the target linker
doesn't get used for host binaries like the build-script.

Fixes:
error: failed to run custom build command for `ripgrep v0.8.1 (/home/buildroot/buildroot/output/build/ripgrep-0.8.1)`

Caused by:
  process didn't exit successfully: `/home/buildroot/buildroot/output/build/ripgrep-0.8.1/target/release/build/ripgrep-59eeb7069534e1ef/build-script-build` (exit status: 1)
  --- stderr
  /home/buildroot/buildroot/output/build/ripgrep-0.8.1/target/release/build/ripgrep-59eeb7069534e1ef/build-script-build: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.33' not found (required by /home/buildroot/buildroot/output/build/ripgrep-0.8.1/target/release/build/ripgrep-59eeb7069534e1ef/build-script-build)

Details:
rust-lang/cargo#3349
rust-lang/cargo#9322

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
@ehuss ehuss added this to the 1.54.0 milestone Feb 6, 2022
jameshilliard added a commit to jameshilliard/cargo that referenced this pull request Mar 1, 2022
jameshilliard added a commit to jameshilliard/cargo that referenced this pull request Oct 21, 2022
jameshilliard added a commit to jameshilliard/cargo that referenced this pull request Dec 3, 2022
jameshilliard added a commit to jameshilliard/cargo that referenced this pull request Apr 11, 2023
@vithalsm
Copy link

vithalsm commented Feb 6, 2024

Hello, I am currently facing this issue with target==host==x86_64. Cargo version is 1.75. Is this version has the fix?

I tried with following settings in the config.toml, but I don't see it taking effect:

[host]
linker="/usr/bin/gcc"

@weihanglo
Copy link
Member

@jameshilliard
Copy link
Contributor Author

Cargo version is 1.75. Is this version has the fix?

Yes but it's a bit annoying to enable since for unclear reasons #9753 has yet to be merged, for buildroot we use these env variables.

@vithalsm
Copy link

vithalsm commented Feb 7, 2024

Thanks for the pointers. However -Z expects cargo from 'nightly' channel or use the env variables as being used by buildroot. Using nightly is not feasible for us, so will try with env variables.

But, would it be possible to merge and make the options available regular stable release please? That will be very helpful for many

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to provide build flags for the build-script-build
10 participants