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.

Sign up for GitHub

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

Jump to bottom

Remove ONLY_BUILD and ONLY_BUILD_TARGETS #48599

Merged
merged 5 commits into from
Mar 11, 2018
Merged

Remove ONLY_BUILD and ONLY_BUILD_TARGETS #48599

merged 5 commits into from
Mar 11, 2018

Conversation

Mark-Simulacrum
Copy link
Member

Primarily removes ONLY_BUILD and ONLY_BUILD_TARGETS. These aren't actually needed in the new system since we can simply not take the relevant host and target fields if we don't want to run with them in Step::make_run.

This PR also includes a few other commits which generally clean up the state of rustbuild, but are not related to the Step changes.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2018
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Feb 28, 2018
Closed
@alexcrichton
Copy link
Member

This seems plausible to me! I think you know more about these variables now than I do, but for me the correctness of the PR is mostly determined by what happens on Travis. Could you run some of the containers locally like a cross dist and cross tests to make sure everything is working? Historically these changes tend to cause an extra step or two to get run on CI which goes undiagnosed for awhile :(

@Mark-Simulacrum
Copy link
Member Author

I've edited the first two commits and rechecked, and I feel that both the ONLY_BUILD_TARGETS and ONLY_BUILD changes are logically true -- that is, given the way we de-duplicate builds, there is no way for them not to work correctly.

I probably won't have time to run dist/cross jobs locally and compare them until at least this weekend, and to an extent have less confidence in my ability to do so correctly than what I've already done here -- but will get back to you with results afterwards.

@pietroalbini pietroalbini 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 Mar 5, 2018
@Mark-Simulacrum
Remove ONLY_BUILD_TARGETS.
1191510 
All cases where it is used can be replaced by substituing run.host for
run.builder.build.build; that is its only effect. As such, it is
removable.
@Mark-Simulacrum
Remove ONLY_BUILD.
1c8f3b0 
All uses are replaced with not accessing run.target/run.host, and
instead directly using run.builder.build.build.
@Mark-Simulacrum
Print out the sysroot and libdir on verbose builds.
c8edb36
@Mark-Simulacrum
Deny warnings
9cfc73c
@Mark-Simulacrum
Refactor run_host_only to have the proper effect.
29a8529 
Previously it was set to true when we didn't run HOSTS steps.
@Mark-Simulacrum Mark-Simulacrum changed the title Cleanup rustbuild Remove ONLY_BUILD and ONLY_BUILD_TARGETS Mar 9, 2018
@Mark-Simulacrum Mark-Simulacrum force-pushed the rustbuild-updates-step-1 branch 2 times, most recently from 761fb27 to 29a8529 Compare March 9, 2018 13:26
@Mark-Simulacrum
Copy link
Member Author

I've manually verified and I feel pretty good about this -- I couldn't find any differences, and the current PR state is intended to be logically sound (even more so than before). r? @alexcrichton

alexcrichton
alexcrichton reviewed Mar 9, 2018
View reviewed changes
src/bootstrap/install.rs
impl Step for Src {
type Output = ();
const DEFAULT: bool = true;
const ONLY_HOSTS: bool = true;
Copy link
Member

Choose a reason for hiding this comment

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

Hm doesn't this mean that we'll be producing the dist-src package on all dist builders now?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the install step; we don't run install on CI. The dist::Src step hasn't changed the conditions it runs on. Neither has this step; I just pulled it out because the macro creates the struct with too many fields for Src... I can go into more detail if necessary, it's somewhat complicated.

Copy link
Member

Choose a reason for hiding this comment

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

Er right sorry I mixed this up with dist::Src, my bad!

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 9, 2018

📌 Commit 29a8529 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 Mar 9, 2018
@Mark-Simulacrum
Copy link
Member Author

@bors rollup

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Mar 9, 2018
@Mark-Simulacrum
Rollup merge of rust-lang#48599 - Mark-Simulacrum:rustbuild-updates-s…
af94aa3 
…tep-1, r=alexcrichton

Remove ONLY_BUILD and ONLY_BUILD_TARGETS

Primarily removes `ONLY_BUILD` and `ONLY_BUILD_TARGETS`. These aren't actually needed in the new system since we can simply not take the relevant `host` and `target` fields if we don't want to run with them in `Step::make_run`.

This PR also includes a few other commits which generally clean up the state of rustbuild, but are not related to the `Step` changes.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Mar 9, 2018
Closed
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Mar 10, 2018
@Mark-Simulacrum
Rollup merge of rust-lang#48599 - Mark-Simulacrum:rustbuild-updates-s…
181424a 
…tep-1, r=alexcrichton

Remove ONLY_BUILD and ONLY_BUILD_TARGETS

Primarily removes `ONLY_BUILD` and `ONLY_BUILD_TARGETS`. These aren't actually needed in the new system since we can simply not take the relevant `host` and `target` fields if we don't want to run with them in `Step::make_run`.

This PR also includes a few other commits which generally clean up the state of rustbuild, but are not related to the `Step` changes.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Mar 10, 2018
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Mar 10, 2018
@kennytm
Rollup merge of rust-lang#48599 - Mark-Simulacrum:rustbuild-updates-s…
c7af197 
…tep-1, r=alexcrichton

Remove ONLY_BUILD and ONLY_BUILD_TARGETS

Primarily removes `ONLY_BUILD` and `ONLY_BUILD_TARGETS`. These aren't actually needed in the new system since we can simply not take the relevant `host` and `target` fields if we don't want to run with them in `Step::make_run`.

This PR also includes a few other commits which generally clean up the state of rustbuild, but are not related to the `Step` changes.
@kennytm kennytm mentioned this pull request Mar 10, 2018
Closed
@bors
Copy link
Contributor

bors commented Mar 11, 2018

⌛ Testing commit 29a8529 with merge 6c70cd1...

bors added a commit that referenced this pull request Mar 11, 2018
@bors
Auto merge of #48599 - Mark-Simulacrum:rustbuild-updates-step-1, r=al…
6c70cd1 
…excrichton

Remove ONLY_BUILD and ONLY_BUILD_TARGETS

Primarily removes `ONLY_BUILD` and `ONLY_BUILD_TARGETS`. These aren't actually needed in the new system since we can simply not take the relevant `host` and `target` fields if we don't want to run with them in `Step::make_run`.

This PR also includes a few other commits which generally clean up the state of rustbuild, but are not related to the `Step` changes.
@bors
Copy link
Contributor

bors commented Mar 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 6c70cd1 to master...

@bors bors merged commit 29a8529 into rust-lang:master Mar 11, 2018
@bors bors mentioned this pull request Mar 11, 2018
Merged
@Mark-Simulacrum Mark-Simulacrum deleted the rustbuild-updates-step-1 branch June 8, 2019 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mark-Simulacrum @alexcrichton @bors @pietroalbini