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

rustbuild: avoid trying to inversely cross-compile for build triple from host triples #76415

Merged
merged 3 commits into from
Sep 11, 2020

Conversation

Mark-Simulacrum
Copy link
Member

This changes rustbuild's cross compilation logic to better match what users expect,
particularly, avoiding trying to inverse cross-compile for the build triple from host triples.
That is, if build=A, host=B, target=B, we do not want to try and compile for A from B.
Indeed, the only "known to run" triple when cross-compiling is the build triple A.
When testing for a particular target we need to be able to run binaries compiled for
that target though.

The last commit also modifies the default set of host/target triples to avoid producing
needless artifacts for the build triple:

The new behavior is to respect --host and --target when passed as the only
configured triples (no triples are implicitly added). The default for --host is
the build triple, and the default for --target is the host triple(s), either
configured or the default build triple.

Fixes #76333

r? @alexcrichton if possible, otherwise we'll need to hunt down a reviewer

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2020
@Mark-Simulacrum Mark-Simulacrum force-pushed the bootstrap-cross-compilation branch from 5e1ddf6 to 0353cc8 Compare September 6, 2020 17:19
@jyn514 jyn514 added A-cross Area: Cross compilation T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 6, 2020
@Mark-Simulacrum
Copy link
Member Author

@infinity0 -- would appreciate your thoughts on this change to defaults. I hope that it moves us closer to doing what you expect from a build system :)

@Mark-Simulacrum Mark-Simulacrum force-pushed the bootstrap-cross-compilation branch from 0353cc8 to 537ca9a Compare September 6, 2020 23:19
@infinity0
Copy link
Contributor

Thanks! Yes this sounds great, and hopefully will shorten cross-compile times too.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me! We've trended over time towards just having one target triple specified in most steps which makes sense to me, and the build triple is just implicit anywhere as what you're actually running rustc for to produce things. It might be worthwhile to take a gander at some of the cross-compiled docker and testing builds though and make sure they're still working as expected.

pub target: TargetSelection,
pub path: PathBuf,
}

impl RunConfig<'_> {
pub fn build_triple(&self) -> TargetSelection {
self.builder.build.build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build, build, build, 🔧 🏗️

}
for target in targets {
let run = RunConfig { builder, path: pathset.path(builder), target: *target };
(self.make_run)(run);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this previous logic matches the ancient logic of the makefiles, but I don't think it ever fully made sense. The main thing to worry about here is the impact on all the builders on CI, since some of them may accidentally be relying on the previous behavior. Everything is so specific nowadays though it may be unlikely. In any case, just something to watch out for.

@Mark-Simulacrum
Copy link
Member Author

I did some looking and local builds, and I think we should be good on CI. It's also not super late in the cycle and we have beta too; if issues do come up we can patch them as needed (likely with CI modifications rather than changes to bootstrap). This should also essentially not affect anyone doing local development, as that's usually not passing any target/host flags. I expect that the typical use cases for distros are either already expecting the new behavior or would quickly add the relevant targets as needed (the main change that might be needed is adding the build triple to the target/host triple lists). I suspect that it is quite rare to expect the build triple to just happen to also be dist'd though.

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Sep 10, 2020

📌 Commit 537ca9acfa3bb9382f5bc553b3bc8e5d5c922b59 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2020
@Mark-Simulacrum
Copy link
Member Author

er, @bors r- r=alexcrichton

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 10, 2020
@bors
Copy link
Contributor

bors commented Sep 10, 2020

📌 Commit 537ca9acfa3bb9382f5bc553b3bc8e5d5c922b59 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 10, 2020
@bors
Copy link
Contributor

bors commented Sep 10, 2020

⌛ Testing commit 537ca9acfa3bb9382f5bc553b3bc8e5d5c922b59 with merge 44bf268cf9ca8a9740e763358ae78c2e17c83677...

@tmandry
Copy link
Member

tmandry commented Sep 10, 2020

@bors retry
Yielding to rollup

@bors
Copy link
Contributor

bors commented Sep 11, 2020

⌛ Testing commit 537ca9acfa3bb9382f5bc553b3bc8e5d5c922b59 with merge e04bb31ce647390be4b450f0657c244dbca5316d...

@bors
Copy link
Contributor

bors commented Sep 11, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 11, 2020
rustc is a natively cross-compiling compiler, and generally none of our steps
should care whether they are using a compiler built of triple A or B, just the
--target directive being passed to the running compiler. e.g., when building for
some target C, you don't generally want to build two stds: one with a host A
compiler and the other with a host B compiler. Just one std is sufficient.
Previously, the CLI --target/--host definitions and configured options differed
in their effect: when setting these on the CLI, only the passed triples would be
compiled for, while in config.toml we would also compile for the build triple
and any host triples. This is needlessly confusing; users expect --target and
--host to be identical to editing the configuration file.

The new behavior is to respect --host and --target when passed as the *only*
configured triples (no triples are implicitly added). The default for --host is
the build triple, and the default for --target is the host triple(s), either
configured or the default build triple.
@Mark-Simulacrum Mark-Simulacrum force-pushed the bootstrap-cross-compilation branch from 537ca9a to 78125ec Compare September 11, 2020 12:59
@Mark-Simulacrum
Copy link
Member Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Sep 11, 2020

📌 Commit 78125ec has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2020
@bors
Copy link
Contributor

bors commented Sep 11, 2020

⌛ Testing commit 78125ec with merge 141bb23...

@bors
Copy link
Contributor

bors commented Sep 11, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: alexcrichton
Pushing 141bb23 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 11, 2020
@bors bors merged commit 141bb23 into rust-lang:master Sep 11, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 11, 2020
@Mark-Simulacrum Mark-Simulacrum deleted the bootstrap-cross-compilation branch September 11, 2020 20:54
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2020
Add host triples to CI builders

This is a follow-up to rust-lang#76415, which changed how x.py interprets cross-compilation target/host flags. This should fix the known cases, but I'm still working through CI logs before/after that PR to identify if anything else is missing.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2020
…oalbini

Add host triples to target lists

This PR is primarily intended to fix rust-lang/rustup#2494, which is the second commit. That bug was introduced by rust-lang#76415, and incompletely fixed by rust-lang#76639. (rust-lang#76639 added host triples, which gave us compilers, but missed that we also need documentation and other target-only things). However, it also removes duplicate macOS CI builders.

r? `@pietroalbini`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cross Area: Cross compilation merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.47 beta cross-compiling broken: tries to run the reverse cross-compile
8 participants