bootstrap: Always build for host, even when target is given #77133

tmandry · 2020-09-24T01:18:34Z

This changes the behavior from not building for host whenever an
explicit target is specified. I find this much less confusing.

You can still disable host steps by passing an explicit empty list for
host.

Fixes #76990.

r? @Mark-Simulacrum

Mark-Simulacrum · 2020-09-24T01:56:38Z

src/bootstrap/config.rs

        config.hosts = if let Some(arg_host) = flags.host {
            arg_host
        } else if let Some(file_host) = build.host {
            file_host.iter().map(|h| TargetSelection::from_user(h)).collect()
        } else {
            vec![config.build]
        };
+        // If host was explicitly given an empty list, don't run any host-only steps.
+        config.skip_only_host_steps = config.hosts.is_empty();


This shouldn't be necessary anymore, the field should be removable with no effect.

I tried that at first but it changed the dist behavior in the test: it causes us to distribute a rustc for the build target ("A" in the test) as well as sources. Is that desirable?

I think that's probably because the tests are always configuring the build triple as part of the host triples, whereas here that's not the case -- https://github.com/Mark-Simulacrum/rust/blob/4de836e2141c418d37e556bc80c4ad0127d74b71/src/bootstrap/builder/tests.rs#L23-L26.

We should, I think, remove this field and update the tests to avoid that buggy behavior (probably by explicitly adding the build triple to all of the tests that need it, i.e., those that don't explicitly override skip_only_host_steps.

Ah that makes sense, done. I also made them list all their targets explicitly for consistency.

Mark-Simulacrum · 2020-09-25T00:48:21Z

Could you also add a note to the CHANGELOG? https://github.com/rust-lang/rust/blob/master/src/bootstrap/CHANGELOG.md

No need to add a new version, just append to the list of changes.

r=me with that done.

Mark-Simulacrum · 2020-09-25T00:52:06Z

Hm, actually, no -- I think this probably also means that we need to go and update a bunch of our builders to explicitly pass --host '' or --host '[]' or something like that if they're only passing target right now, otherwise we'll bump build times and all up. I'm not sure what the best way to do that is, though -- maybe just rg '\-\-target' src/ci/ and trying to catch all of those cases?

I'm not sure if this is a sign we're "doing the wrong thing" or not -- I think probably no, because in most cases you do want to test or distribute for both targets and hosts, just in our CI the way we parallelize things we want to avoid that because we're taking care of the host part in one place and then spreading the targets out.

tmandry · 2020-09-25T00:58:05Z

No need to add a new version, just append to the list of changes.

Should we consider it a breaking change though?

Mark-Simulacrum · 2020-09-25T01:02:46Z

Hm, it's a good question. I guess the answer is probably yes. It should be relatively easy to bump the version number -- I think just https://github.com/rust-lang/rust/blob/master/src/bootstrap/bin/main.rs#L30 needs to change but not 100% sure.

cc @jyn514 as well

jyn514 · 2020-09-25T01:07:29Z

You also need to add it to the changelog itself: https://github.com/rust-lang/rust/blob/master/src/bootstrap/CHANGELOG.md. Those should be the only two places I think.

jyn514 · 2020-09-25T01:08:04Z

Hm, actually, no -- I think this probably also means that we need to go and update a bunch of our builders to explicitly pass --host '' or --host '[]' or something like that if they're only passing target right now, otherwise we'll bump build times and all up. I'm not sure what the best way to do that is, though -- maybe just rg '--target' src/ci/ and trying to catch all of those cases?

Another use case for https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/x.2Epy.20CI.20defaults ;)

Mark-Simulacrum · 2020-09-25T01:15:49Z

I don't think defaults would be a good fit here, they would probably confuse things more than help - better to have both overriden in the same place.

tmandry · 2020-09-25T01:28:39Z

Hmm, thinking about this more and I'm pretty on the fence about what to do.

For config.toml it seems like we're reverting back to what we had 1-2 weeks ago, so having people bump changelog-seen feels like overkill.

For the command line we could consider it a breaking change, but we could also say that building more than we used to in some cases isn't actually breaking. Still, we do want people who might be impacted to know about it.

jyn514 · 2020-09-25T01:30:04Z

Would it help if instead of a binary breaking/not-breaking you printed a short description of the change?

tmandry · 2020-09-25T01:33:47Z

Do you mean in the changelog? Or in check_version()?

A changelog entry would probably look something like:

host now defaults to the value of build in all cases. Previously we defaulted host to an empty list when target was overridden and to build otherwise.

jyn514 · 2020-09-25T01:36:17Z

I meant in check_version.

In general I think nagging more is better than nagging less. The thing to avoid is people changing the number without reading the changelog, and I don't think we've had enough changes for that yet.

tmandry · 2020-09-25T01:55:44Z

Right, and because these changes are "live" as soon as they merge, I think we're always going to have the issue of there being only a single change for each version. Which means we might want to tweak the changelog format a bit. Maybe grouping changes by month would make sense?

I do like the idea of sticking the change text in check_version(), i.e. a programmatic changelog to match the text one. One other idea: it would be nice if there were a flag or command to bump the version in your config.toml for you =)

tmandry · 2020-09-26T00:05:40Z

Okay, updated the changelog and bumped the version. Outputting breaking changes from x.py and adding an "ack" feature would be good for a follow up change.

tmandry · 2020-09-26T00:07:49Z

@Mark-Simulacrum I'll let you take a final look since I updated all the CI scripts

bors · 2020-09-26T20:22:00Z

☔ The latest upstream changes (presumably #77224) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

jyn514 · 2020-09-29T05:34:14Z

Looks like you need --host="" instead of --host somewhere.

Trying to fix a problem in CI. Maybe some version of Docker is not passing '' args correctly?

tmandry · 2020-09-29T20:16:09Z

Strange, maybe some version of Docker wasn't passing through empty '' args? I changed everything to use --host='' to see if that fixes things.

@bors r=Mark-Simulacrum rollup=never

bors · 2020-09-29T20:16:10Z

📌 Commit 63eaa60 has been approved by Mark-Simulacrum

bors · 2020-09-29T21:44:30Z

⌛ Testing commit 63eaa60 with merge cd6acf7a460fd4dd80f83443261995d6907c07b7...

bors · 2020-09-29T22:13:32Z

💔 Test failed - checks-actions

tmandry · 2020-09-29T22:28:53Z

thread 'main' panicked at '

couldn't find required command: "arm-linux-gnueabihf-g++"

', src/bootstrap/sanity.rs:59:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test --host= --target arm-unknown-linux-gnueabihf

I want this to be a spurious error but it probably isn't. Maybe there's a subtle change in the behavior that we haven't accounted for.

Mark-Simulacrum · 2020-09-29T22:42:45Z

Well, one immediate problem is that --host='' appears to configure an empty string as the triple, at least based on some quick dbg!ing locally.

[src/bootstrap/flags.rs:577] split(&matches.opt_strs("host")).into_iter().map(|x|
                                                     TargetSelection::from_user(&x)).collect::<Vec<_>>() = [
    TargetSelection {
        triple: "",
        file: None,
    },
]

We should probably just filter out empty strings in the split function in flags.rs, that seems to have fixed that failure locally for me at least.

tmandry · 2020-09-29T22:51:30Z

Nice catch! I seem to have forgotten that I can reproduce these locally with Docker.

tmandry · 2020-09-29T22:51:49Z

Pushed a change with your suggestion.

Mark-Simulacrum · 2020-09-29T22:52:45Z

That said the build still fails for me locally with zlib.h header missing, but not sure if that's related to this PR. I think it might be changes to the docker environment or something like that, because adding zlib1g-dev to the armhf-gnu docker container does seem to fix things. Maybe an upstream change to 16.04? Seems unlikely, but not entirely implausible, and I'm fine with hitting CI to find out.

Mark-Simulacrum · 2020-09-29T22:55:04Z

TIL about carp streamer, https://codepoints.net/U+1F38F, though how that relates to "flags" beyond some visual similarity I'm not sure.

Anyway, @bors r+ rollup=never

bors · 2020-09-29T22:55:05Z

📌 Commit bf7aeaa has been approved by Mark-Simulacrum

bors · 2020-09-30T00:59:18Z

⌛ Testing commit bf7aeaa with merge 0d9afb6...

bors · 2020-09-30T03:10:40Z

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 0d9afb6 to master...

@Mark-Simulacrum

…orino Update `changelog-seen` in config.toml.example This got out of sync when the version was bumped last time in rust-lang#77133 Long-term we may want to find an easier way to maintain this that doesn't require bumping the version in three different places. Off the top of my head I can't think of anything, though. It _is_ documented in src/bootstrap/README.md, although I don't know how many people read that. r? @Mark-Simulacrum cc @spastorino

@Mark-Simulacrum

…orino Update `changelog-seen` in config.toml.example This got out of sync when the version was bumped last time in rust-lang#77133 Long-term we may want to find an easier way to maintain this that doesn't require bumping the version in three different places. Off the top of my head I can't think of anything, though. It _is_ documented in src/bootstrap/README.md, although I don't know how many people read that. r? @Mark-Simulacrum cc @spastorino

rust-highfive assigned Mark-Simulacrum Sep 24, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2020

Mark-Simulacrum reviewed Sep 24, 2020

View reviewed changes

Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2020

tmandry added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 25, 2020

Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2020

jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 25, 2020

tmandry force-pushed the bootstrap-host branch from 114c806 to abee146 Compare September 25, 2020 01:29

tmandry force-pushed the bootstrap-host branch from abee146 to 631bd06 Compare September 25, 2020 01:50

tmandry force-pushed the bootstrap-host branch from 631bd06 to 2826235 Compare September 25, 2020 02:33

tmandry added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 26, 2020

Use --host='' instead of --host ''

63eaa60

Trying to fix a problem in CI. Maybe some version of Docker is not passing '' args correctly?

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 29, 2020

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 29, 2020

Filter out empty items in bootstrap::flags::split

bf7aeaa

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 29, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 30, 2020

bors merged commit 0d9afb6 into rust-lang:master Sep 30, 2020

rustbot added this to the 1.48.0 milestone Sep 30, 2020

tmandry deleted the bootstrap-host branch September 30, 2020 19:00

jyn514 mentioned this pull request Oct 10, 2020

Update changelog-seen in config.toml.example #77787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootstrap: Always build for host, even when target is given #77133

bootstrap: Always build for host, even when target is given #77133

tmandry commented Sep 24, 2020

Mark-Simulacrum Sep 24, 2020

tmandry Sep 24, 2020

Mark-Simulacrum Sep 24, 2020

tmandry Sep 25, 2020

Mark-Simulacrum commented Sep 25, 2020

Mark-Simulacrum commented Sep 25, 2020

tmandry commented Sep 25, 2020

Mark-Simulacrum commented Sep 25, 2020

jyn514 commented Sep 25, 2020

jyn514 commented Sep 25, 2020

Mark-Simulacrum commented Sep 25, 2020

tmandry commented Sep 25, 2020

jyn514 commented Sep 25, 2020

tmandry commented Sep 25, 2020

jyn514 commented Sep 25, 2020 •

edited

Loading

tmandry commented Sep 25, 2020 •

edited

Loading

tmandry commented Sep 26, 2020

tmandry commented Sep 26, 2020

bors commented Sep 26, 2020

jyn514 commented Sep 29, 2020

tmandry commented Sep 29, 2020

bors commented Sep 29, 2020

bors commented Sep 29, 2020

bors commented Sep 29, 2020

tmandry commented Sep 29, 2020

Mark-Simulacrum commented Sep 29, 2020

tmandry commented Sep 29, 2020

tmandry commented Sep 29, 2020

Mark-Simulacrum commented Sep 29, 2020

Mark-Simulacrum commented Sep 29, 2020

bors commented Sep 29, 2020

bors commented Sep 30, 2020

bors commented Sep 30, 2020

bootstrap: Always build for host, even when target is given #77133

bootstrap: Always build for host, even when target is given #77133

Conversation

tmandry commented Sep 24, 2020

Mark-Simulacrum Sep 24, 2020

Choose a reason for hiding this comment

tmandry Sep 24, 2020

Choose a reason for hiding this comment

Mark-Simulacrum Sep 24, 2020

Choose a reason for hiding this comment

tmandry Sep 25, 2020

Choose a reason for hiding this comment

Mark-Simulacrum commented Sep 25, 2020

Mark-Simulacrum commented Sep 25, 2020

tmandry commented Sep 25, 2020

Mark-Simulacrum commented Sep 25, 2020

jyn514 commented Sep 25, 2020

jyn514 commented Sep 25, 2020

Mark-Simulacrum commented Sep 25, 2020

tmandry commented Sep 25, 2020

jyn514 commented Sep 25, 2020

tmandry commented Sep 25, 2020

jyn514 commented Sep 25, 2020 • edited Loading

tmandry commented Sep 25, 2020 • edited Loading

tmandry commented Sep 26, 2020

tmandry commented Sep 26, 2020

bors commented Sep 26, 2020

jyn514 commented Sep 29, 2020

tmandry commented Sep 29, 2020

bors commented Sep 29, 2020

bors commented Sep 29, 2020

bors commented Sep 29, 2020

tmandry commented Sep 29, 2020

Mark-Simulacrum commented Sep 29, 2020

tmandry commented Sep 29, 2020

tmandry commented Sep 29, 2020

Mark-Simulacrum commented Sep 29, 2020

Mark-Simulacrum commented Sep 29, 2020

bors commented Sep 29, 2020

bors commented Sep 30, 2020

bors commented Sep 30, 2020

jyn514 commented Sep 25, 2020 •

edited

Loading

tmandry commented Sep 25, 2020 •

edited

Loading