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: Fix the --build argument to bootstrap.py #43384

Merged
merged 1 commit into from
Jul 25, 2017

Conversation

segevfiner
Copy link
Contributor

This makes the --build argument also apply for the downloading of the stage0 toolchain and building rustbuild.

Fixes #42116

This makes the --build argument also apply for the downloading of the
stage0 toolchain and building rustbuild.

Fixes rust-lang#42116
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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.

@shepmaster
Copy link
Member

Thanks for the hard work, @segevfiner! We'll get someone top-notch to review this shortly!

/cc @Mark-Simulacrum who is also working in rustbuild world

@@ -669,7 +670,7 @@ def bootstrap():
rb.update_submodules()

# Fetch/build the bootstrap
rb.build = rb.build_triple()
rb.build = args.build or rb.build_triple()
Copy link
Member

Choose a reason for hiding this comment

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

It feels like this should be inside build_triple itself, but presumably it might not have access to args today? Not sure.

cc @aidanhs, this affects python bootstrap

Copy link
Contributor Author

@segevfiner segevfiner Jul 21, 2017

Choose a reason for hiding this comment

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

It doesn't have access. The command line is handled in the bootstrap() function. The parsed args, or stuff derived from them, are set as attributes of the RustBuild object.

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 21, 2017
@aidanhs
Copy link
Member

aidanhs commented Jul 21, 2017

The fix looks reasonable, but I'm tempted to instead propose that we just remove that piece of documentation and make it configured by the config file. Seems a little odd for --build to be a special case of an argument understood for reconfiguration.

@segevfiner, what do you think about just altering the toml? Or, alternatively, I assume you can pass an argument to configure to set this? (or if you can't, you should be able to!)

Just want to understand the use-case.

@segevfiner
Copy link
Contributor Author

segevfiner commented Jul 21, 2017

@aidanhs
I think all the arguments that bootstrap.py parses are like this. They are passed to the underlying bootstrap executable (rustbuild itself) too.

build is configurable via the config file, that's what I did when I wanted to build using a non-default build. Using it from the config is fine, I see there is even a flag so you can use multiple config files, which is useful when building using multiple build compilers in the same source tree. But using --build will be kinda convenient for this too, which I guess is why it exists in the first place. You will than only need one config.toml and there is even a target specific section in config.toml to help with this.

If it's decided this should not be supported, it should probably removed entirely from rustbuild (It appears in --help). But note that we also have --target and --host, and that begs the question if they should be removed than too...

@aidanhs
Copy link
Member

aidanhs commented Jul 24, 2017

Gotcha, so supporting it here is required if we want to support that flag on rustbuild. Thanks for explaining :) I'm suspicious that there are more edge cases here that aren't handled correctly, but clearly this is a step forward so I'm happy.

Any comments from a rustbuild perspective @Mark-Simulacrum?

@Mark-Simulacrum
Copy link
Member

I don't think so. It seems good to handle --build in the python script so that it can work for more cases; I don't think there are any other flags that we'd need to handle like this, but I could be wrong. If there are, we probably should do something about it. I've wondered if we should have a "minimal" bootstrap in Rust which takes over some of the more complicated logic in the python script, maybe, but that's likely not possible today (and certainly out of scope for this PR). Overall, I think this is good.

@aidanhs
Copy link
Member

aidanhs commented Jul 25, 2017

Thinking about it briefly, I agree - no flags spring to mind as additional cases that should be considered, but I'll look through all the possible flags tomorrow. In the meantime...

@bors r+

@bors
Copy link
Contributor

bors commented Jul 25, 2017

📌 Commit c41ac2e has been approved by aidanhs

@bors
Copy link
Contributor

bors commented Jul 25, 2017

⌛ Testing commit c41ac2e with merge 5c126bd...

bors added a commit that referenced this pull request Jul 25, 2017
…anhs

rustbuild: Fix the --build argument to bootstrap.py

This makes the --build argument also apply for the downloading of the stage0 toolchain and building rustbuild.

Fixes #42116
@bors
Copy link
Contributor

bors commented Jul 25, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: aidanhs
Pushing 5c126bd to master...

@bors bors merged commit c41ac2e into rust-lang:master Jul 25, 2017
@segevfiner segevfiner deleted the bootstrap-build-argument-fix branch July 25, 2017 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants