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

Removed references of android-rs-glue from comments and documentation #9549

Merged
merged 1 commit into from Feb 6, 2016
Merged

Removed references of android-rs-glue from comments and documentation #9549

merged 1 commit into from Feb 6, 2016
+4 −6

Conversation

@saurvs
Copy link
Contributor

saurvs commented Feb 5, 2016

Fixes #9507.

Review on Reviewable
@highfive
Copy link

highfive commented Feb 5, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon.
python/servo/command_base.py
@@ -362,11 +362,6 @@ def build_env(self, gonk=False, hosts_file_path=None):
env["OPENSSL_LIB_DIR"] = openssl_dir
env['OPENSSL_INCLUDE_DIR'] = path.join(env["GONKDIR"], "external/openssl/include")

# FIXME: These are set because they are the variable names that

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Feb 5, 2016

Member

This comment is removed, but the code was not changed.

This comment has been minimized.

Sign in to view
@larsbergstrom

larsbergstrom Feb 5, 2016

Contributor

Yeah, sorry I didn't see this PR come in! He does not need to change the code - just the comment. Sorry I wasn't clear!
@saurvs
Copy link
Contributor Author

saurvs commented Feb 5, 2016

Yeah, I was unsure about that because the issue said the references linger on only in comments and documentation, and I didn't want to break anything by removing code I wasn't fully familiar with. But I'll correct it now.
@wafflespeanut
Copy link
Member

wafflespeanut commented Feb 5, 2016

r? @larsbergstrom
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 5, 2016

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

python/servo/command_base.py, line 365 [r2] (raw file):
Please just change the comment to mention build-apk instead. Sorry I didn't make that clear! These variables are still used in the build scripts that were based on the original ones from android-rs-glue.

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

saurvs commented Feb 5, 2016

So should the original comment stay with 'android-rs-glue' replaced by 'build-apk'?
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 5, 2016

@saurvs Yes, please! Also, you can remove the FIXME, since I don't think it's going anywhere now :-)
@saurvs
Copy link
Contributor Author

saurvs commented Feb 5, 2016

Ok!
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 5, 2016

Looks great! Can you please squash the commits, and then I'll land it? Let me know if you need help with the git commands to squash them :-)

Thanks for your patience!
@nox
Copy link
Member

nox commented Feb 5, 2016

@bors-servo rollup
@nox nox added S-needs-squash and removed S-awaiting-review labels Feb 5, 2016
@saurvs
Removed documentation referencing 'android-rs-glue', and updated a si…
Loading status checks…
9801552 
…milar comment to mention 'build-apk'
@saurvs
Copy link
Contributor Author

saurvs commented Feb 5, 2016

Squashing done!
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 5, 2016

@bors-servo r+ rollup
@bors-servo
Copy link
Contributor

bors-servo commented Feb 5, 2016

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

bors-servo commented Feb 6, 2016

⌛ Testing commit 9801552 with merge b2a297c...
bors-servo added a commit that referenced this pull request Feb 6, 2016
@bors-servo
Auto merge of #9549 - saurvs:master, r=larsbergstrom
Loading status checks…
b2a297c 
Removed references of android-rs-glue from comments and documentation

Fixes #9507.

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

bors-servo commented Feb 6, 2016

💔 Test failed - mac-rel-css
@KiChjang
Copy link
Member

KiChjang commented Feb 6, 2016

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

bors-servo commented Feb 6, 2016

⚡ Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-wpt are reusable. Rebuilding only mac-rel-css...
@nox nox added the S-awaiting-merge label Feb 6, 2016
@nox nox removed the S-tests-failed label Feb 6, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Feb 6, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 9801552 into servo:master Feb 6, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@saurvs @highfive @wafflespeanut @larsbergstrom @nox @bors-servo @KiChjang
You can’t perform that action at this time.