Skip to content

/ servo

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

Download extra stdlib only when required: #9557 #9611

Merged
merged 1 commit into from Feb 13, 2016
Merged

Download extra stdlib only when required: #9557 #9611

merged 1 commit into from Feb 13, 2016

Conversation

@dlrobertson
Copy link
Contributor

dlrobertson commented Feb 12, 2016

Split ensure_bootstrap into two phases including a phase checking the compiler, and a phase checking for target libraries. E.g.

    # should download the stdlib for "i686-unknown-linux-gnu", "arm-linux-androideabi"
    # and the hosts target
    ./mach build -d --target i686-unknown-linux-gnu --android
    # should only download the stdlib for the hosts target
    ./mach build -d

Let me know if I missed anything! There are a few parts of this patch in its current state that I'm not a huge fan of, but I couldn't think of a better way in the moment.

Still new to working on servo, so any comments or critiques are welcome!

Fix #9557

Review on Reviewable
@highfive
Copy link

highfive commented Feb 12, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Ms2ger (or someone else) soon.
@dlrobertson dlrobertson force-pushed the dlrobertson:i9557 branch from f597b5c to 7cb4c21 Feb 12, 2016
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 12, 2016

Awesome! This is very exciting - thanks for doing this. It will save folks quite a bit of bandwidth (and disk space).
@dlrobertson dlrobertson force-pushed the dlrobertson:i9557 branch from 7cb4c21 to 3cfce59 Feb 12, 2016
@dlrobertson
Copy link
Contributor Author

dlrobertson commented Feb 12, 2016

Thanks, no problem! Let me know if you have any pro tips for a servo noobie at drobertson on #servo 😄
@jdm
Copy link
Member

jdm commented Feb 12, 2016

r? @larsbergstrom
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 12, 2016

I'm not sure how I feel about the split of bootstrap-rust and bootstrap-rust-libs. I feel like it would be nice if we still just had a single bootstrap-rust that could handle the --target argument, but I assume you factored it this way to make it easier to split out check for whether all of the stdlibs are present in command_base.py?

If so, I think this is OK, with just the few nits and comment cleanup addressed. Thanks!

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

python/servo/bootstrap_commands.py, line 151 [r1] (raw file):
Remove this TODO :-)

python/servo/bootstrap_commands.py, line 164 [r1] (raw file):
nit: bootstrap-rust-libs appears to be the command name used.

python/servo/command_base.py, line 401 [r1] (raw file):
nit: either host target or host's target

Comments from the review on Reviewable.io
@dlrobertson
Copy link
Contributor Author

dlrobertson commented Feb 12, 2016

I'm not sure how I feel about the split of bootstrap-rust and bootstrap-rust-libs. I feel like it would be nice if we still just had a single bootstrap-rust that could handle the --target argument

Agreed, the update will reflect this. Good suggestion!
@dlrobertson
Download extra std lib when cross-compiling #9557
Loading status checks…
0df4118 
Split ensure_bootstrap into two phases including a phase checking the
compiler, and a phase checking for target libraries.
@dlrobertson dlrobertson force-pushed the dlrobertson:i9557 branch from 3cfce59 to 0df4118 Feb 12, 2016
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 12, 2016

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 12, 2016

@bors-servo r+

Great work!
@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

📌 Commit 0df4118 has been approved by larsbergstrom
@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

⌛ Testing commit 0df4118 with merge 088963f...
bors-servo added a commit that referenced this pull request Feb 12, 2016
@bors-servo
Auto merge of #9611 - danlrobertson:i9557, r=larsbergstrom
Loading status checks…
088963f 
Download extra stdlib only when required: #9557

Split [`ensure_bootstrap`](https://github.com/danlrobertson/servo/blob/i9557/python/servo/command_base.py#L397-L422) into two phases including a phase checking the compiler, and a phase checking for target libraries. E.g.

```
    # should download the stdlib for "i686-unknown-linux-gnu", "arm-linux-androideabi"
    # and the hosts target
    ./mach build -d --target i686-unknown-linux-gnu --android
    # should only download the stdlib for the hosts target
    ./mach build -d
```

Let me know if I missed anything! There are a few parts of this patch in its current state that I'm not a huge fan of, but I couldn't think of a better way in the moment.

Still new to working on servo, so any comments or critiques are welcome!

Fix #9557

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9611)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

💔 Test failed - mac-rel-wpt
@jdm
Copy link
Member

jdm commented Feb 12, 2016

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

⚡ Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css are reusable. Rebuilding only mac-rel-wpt...
@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

💔 Test failed - mac-rel-wpt
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 13, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Feb 13, 2016

⚡ Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css are reusable. Rebuilding only mac-rel-wpt...
@bors-servo
Copy link
Contributor

bors-servo commented Feb 13, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 0df4118 into servo:master Feb 13, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@dlrobertson dlrobertson deleted the dlrobertson:i9557 branch Feb 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@larsbergstrom larsbergstrom

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@dlrobertson @highfive @larsbergstrom @jdm @bors-servo
You can’t perform that action at this time.