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

Use the same build environment and features for CEF, Servo, Gonk, Geckolib #11122

Merged
merged 2 commits into from
May 11, 2016

Conversation

mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented May 10, 2016

  • Remove unnecessary dependencies and features from top-level Cargo.tomls. The features for each crate will be computed based on the union of features specified in the dependency graph. Specifying the same ones again just adds more ways for them to get out of sync.
  • Move all cargo build environment variables into CommandBase

Fixes #11112. r? @metajack

(Not included: CI test to make sure #11112 doesn't regress again.)


This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/servo/command_base.py, python/servo/build_commands.py
  • @KiChjang: components/script/Cargo.toml

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 10, 2016
@highfive
Copy link

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@metajack
Copy link
Contributor

@bors-servo r+

The CommandBase stuff is a really nice cleanup.

Previously, highfive wrote…

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

Reviewed 2 of 2 files at r1, 11 of 11 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bors-servo
Copy link
Contributor

📌 Commit 2d24f48 has been approved by metajack

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 11, 2016
@mbrubeck
Copy link
Contributor Author

@bors-servo p=1

@bors-servo
Copy link
Contributor

⌛ Testing commit 2d24f48 with merge e6efe7c...

bors-servo pushed a commit that referenced this pull request May 11, 2016
Use the same build environment and features for CEF, Servo, Gonk, Geckolib

* Remove unnecessary dependencies and features from top-level Cargo.tomls.  The features for each crate will be computed based on the union of features specified in the dependency graph.  Specifying the same ones again just adds more ways for them to get out of sync.
* Move all cargo build environment variables into CommandBase

Fixes #11112. r? @metajack

(Not included: CI test to make sure #11112 doesn't regress again.)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11122)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - arm32

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels May 11, 2016
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels May 11, 2016
@highfive
Copy link

New code was committed to pull request.

@mbrubeck
Copy link
Contributor Author

@bors-servo r=metajack

  • Fixed a Python error that broke the arm32 build

@bors-servo
Copy link
Contributor

📌 Commit 0573d12 has been approved by metajack

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 11, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 0573d12 with merge 17b2909...

bors-servo pushed a commit that referenced this pull request May 11, 2016
Use the same build environment and features for CEF, Servo, Gonk, Geckolib

* Remove unnecessary dependencies and features from top-level Cargo.tomls.  The features for each crate will be computed based on the union of features specified in the dependency graph.  Specifying the same ones again just adds more ways for them to get out of sync.
* Move all cargo build environment variables into CommandBase

Fixes #11112. r? @metajack

(Not included: CI test to make sure #11112 doesn't regress again.)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11122)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - android

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels May 11, 2016
@jdm
Copy link
Member

jdm commented May 11, 2016

main.rs:22:1: 22:27 error: can't find crate for `android_glue` [E0463]
main.rs:22 extern crate android_glue;
           ^~~~~~~~~~~~~~~~~~~~~~~~~~

The features for each crate will be computed based on the union of features
specified in the dependency graph.  Specifying the same ones again just adds
more ways for them to get out of sync.
@highfive
Copy link

New code was committed to pull request.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels May 11, 2016
@mbrubeck
Copy link
Contributor Author

@bors-servo r=larsbergstrom p=0

  • Fix the android build

@bors-servo
Copy link
Contributor

📌 Commit b2e874e has been approved by larsbergstrom

@highfive highfive assigned larsbergstrom and unassigned metajack May 11, 2016
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 11, 2016
@mbrubeck
Copy link
Contributor Author

I notice with this change, build-cef on buildbot is no longer rebuilding the whole dependency graph, but it is still rebuilding the style crate and everything that depends on it:

http://build.servo.org/builders/mac-rel-wpt/builds/947/steps/compile_1/logs/stdio

I suspect this is because some file timestamp is causing Cargo to re-run style's build script (maybe a .pyc file). If so, we could fix this by having the build script print an appropriate cargo:rerun-if-changed line.

@metajack
Copy link
Contributor

Doesn't cargo look at .gitignore? Perhaps we're just missing a gitignore rule?

@mbrubeck
Copy link
Contributor Author

mbrubeck commented May 11, 2016

Doesn't cargo look at .gitignore? Perhaps we're just missing a gitignore rule?

Cargo doesn’t use git to walk crates within the servo repo, because it doesn’t find the .git directory, because it’s not at the same level as any Cargo.toml file. When Cargo workspaces (RFC 1525) are implemented and we start using them I think that will fix this, and things like this will Just Work™.

@mbrubeck
Copy link
Contributor Author

@bors-servo p=1

@bors-servo
Copy link
Contributor

⌛ Testing commit b2e874e with merge 7f76e3b...

bors-servo pushed a commit that referenced this pull request May 11, 2016
Use the same build environment and features for CEF, Servo, Gonk, Geckolib

* Remove unnecessary dependencies and features from top-level Cargo.tomls.  The features for each crate will be computed based on the union of features specified in the dependency graph.  Specifying the same ones again just adds more ways for them to get out of sync.
* Move all cargo build environment variables into CommandBase

Fixes #11112. r? @metajack

(Not included: CI test to make sure #11112 doesn't regress again.)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11122)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

@bors-servo bors-servo merged commit b2e874e into servo:master May 11, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 11, 2016
@mbrubeck mbrubeck deleted the unify-builds branch May 17, 2016 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants