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

Add OSMesa headless mode, run WPT against Webrender. #13382

Merged
merged 1 commit into from Sep 28, 2016

Conversation

@glennw
Copy link
Member

glennw commented Sep 22, 2016

This change is Reviewable

@highfive
Copy link

highfive commented Sep 22, 2016

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/Cargo.toml
  • @wafflespeanut: python/servo/command_base.py, python/servo/testing_commands.py, python/servo/post_build_commands.py
  • @jgraham: tests/wpt/harness/wptrunner/browsers/servodriver.py, tests/wpt/harness/wptrunner/wptcommandline.py, tests/wpt/harness/wptrunner/browsers/servo.py, tests/wpt/harness/wptrunner/executors/executorservo.py, tests/wpt/harness/requirements_servo.txt
  • @KiChjang: components/script/Cargo.toml, components/script/dom/webidls/Window.webidl, components/script/lib.rs, components/net/Cargo.toml, components/script/dom/webidls/Element.webidl
@highfive
Copy link

highfive commented Sep 22, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@glennw
Copy link
Member Author

glennw commented Sep 22, 2016

bors-servo added a commit that referenced this pull request Sep 22, 2016
Add OSMesa headless mode, run WPT against Webrender.
@bors-servo
Copy link
Contributor

bors-servo commented Sep 22, 2016

Trying commit 5ad48af with merge 5abb39f...

@glennw
Copy link
Member Author

glennw commented Sep 22, 2016

This is ready for review now.

If anyone interested can take a look at the test expectation changes and let me know if there's anything in there that we don't want to land with updated expectations, I'll look further into the root cause of the differing results.

@emilio r? on the webgl paint thread changes.

@jdm r? on the script / webidl changes.

@larsbergstrom r? on the osmesa build changes.

@emilio / @larsbergstrom / @pcwalton r? on the glutin headless / osmesa changes.

@jgraham / @larsbergstrom r? on the python / wpt changes. Some parts (e.g. the WPT / python changes) might be in the wrong place - please let me know if the case.

@bors-servo
Copy link
Contributor

bors-servo commented Sep 22, 2016

💔 Test failed - linux-dev

@glennw
Copy link
Member Author

glennw commented Sep 23, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Sep 23, 2016

Trying commit aa19938 with merge e31279a...

bors-servo added a commit that referenced this pull request Sep 23, 2016
Add OSMesa headless mode, run WPT against Webrender.

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

bors-servo commented Sep 23, 2016

💔 Test failed - linux-dev

@jdm
Copy link
Member

jdm commented Sep 23, 2016

@glennw Could you verify if all the other new crashes in the test expectations are the same matrix problem?

@glennw
Copy link
Member Author

glennw commented Sep 23, 2016

@jdm I've verified that the four panics in the transform tests are the same bug, and that the three panics in the filters tests are servo/webrender#419.

Both of them can be fixed before landing this if you'd prefer (the filters one is a bit more complicated to fix than the transform one though).

@jdm
Copy link
Member

jdm commented Sep 23, 2016

I'm fine with landing them as is. No need to hold them up as long as we understand what's going on.

@jdm
Copy link
Member

jdm commented Sep 23, 2016

r=me on the script/webidl changes.

height as u32,
chan.send(CanvasData::Pixels(pixel_data)).unwrap();
}
WebGLPaintTaskData::WebRender(ref wr_api, _, Some(image_key)) => {

This comment has been minimized.

@emilio

emilio Sep 23, 2016

Member

hmm... Do we have the context bound at this point in this thread? I think we don't.

This comment has been minimized.

@glennw

glennw Sep 23, 2016

Author Member

Ah, yes. I guess we just need to add the WR(Some(image)) case to the init() method below, right? So that it binds the context in the same way as the non-WR readback case?

This comment has been minimized.

@emilio

emilio Sep 23, 2016

Member

Hm... Yes, though you'd need to reach the actual context, plus apply commands locally in this thread instead of sending them to WebRender.

All in all, I think the best approach is to expose the RendererKind (and probably the wrapper types for the context too) and move the fallback logic here, so that the Readback data contains the correct kind of context.

@@ -215,7 +320,7 @@ impl Window {
#[cfg(not(target_os = "android"))]
fn gl_version() -> GlRequest {
if opts::get().use_webrender {
return GlRequest::Specific(Api::OpenGl, (3, 2));
return GlRequest::Specific(Api::OpenGl, (3, 3));

This comment has been minimized.

@emilio

emilio Sep 23, 2016

Member

Is this completely necessary? (I assume so, but it's kind of unfortunate)

WindowKind::Window(ref window) => {
window.set_inner_size(size.width as u32, size.height as u32)
}
WindowKind::Headless(..) => {}

This comment has been minimized.

@emilio

emilio Sep 23, 2016

Member

I think we can simplify these cases returning early and adding a window() method, or something, but I guess this is fine. r=me on the glutin changes.

@jgraham
Copy link
Contributor

jgraham commented Sep 23, 2016

wptrunner changes look mostly fine, but need to also land upstream.


Reviewed 4 of 4 files at r9, 1 of 1 files at r12.
Review status: 5 of 155 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


python/servo/command_base.py, line 54 at r14 (raw file):

    deps_path = path.join(path.split(bin_path)[0], "build")
    with cd(deps_path):
        print(os.getcwd())

Intentional?


tests/wpt/harness/requirements_servo.txt, line 2 at r14 (raw file):

mozprocess >= 0.19
Mako >= 1.0.4

Hmm, why is this required to run the tests?


Comments from Reviewable

@glennw
Copy link
Member Author

glennw commented Sep 25, 2016

Review status: 5 of 155 files reviewed at latest revision, 5 unresolved discussions.


tests/wpt/harness/requirements_servo.txt, line 2 at r14 (raw file):

Previously, jgraham wrote…

Hmm, why is this required to run the tests?

The osmesa build process requires python + mako. I'm assuming this is the wrong place to put the requirement, although it does work :) Where should it be?

Comments from Reviewable

@glennw
Copy link
Member Author

glennw commented Sep 25, 2016

Review status: 5 of 155 files reviewed at latest revision, 5 unresolved discussions.


ports/glutin/window.rs, line 323 at r14 (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

Is this completely necessary? (I assume so, but it's kind of unfortunate)

Nope, fixed.

python/servo/command_base.py, line 54 at r14 (raw file):

Previously, jgraham wrote…

Intentional?

Fixed

Comments from Reviewable

@glennw
Copy link
Member Author

glennw commented Sep 25, 2016

@jgraham What is the upstreaming process - is it something we do after this all lands? Or does it need to be done beforehand?

@glennw
Copy link
Member Author

glennw commented Sep 26, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2016

📌 Commit cdb69d4 has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2016

Testing commit cdb69d4 with merge c7e1a57...

bors-servo added a commit that referenced this pull request Sep 28, 2016
Add OSMesa headless mode, run WPT against Webrender.

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

bors-servo commented Sep 28, 2016

💔 Test failed - linux-rel

@notriddle
Copy link
Contributor

notriddle commented Sep 28, 2016

/css21_dev/html4/max-width-017.htm failed, and it's not a known intermittent.

@glennw
Copy link
Member Author

glennw commented Sep 28, 2016

@bors-servo retry

  • Testing if intermittent - have never seen that test fail before locally or during all these try runs.
@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2016

Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only linux-rel, mac-rel-wpt...

@notriddle
Copy link
Contributor

notriddle commented Sep 28, 2016

@glennw
Copy link
Member Author

glennw commented Sep 28, 2016

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2016

Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only linux-rel, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2016

@bors-servo bors-servo merged commit cdb69d4 into servo:master Sep 28, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Oct 3, 2016
Move mako out of wptrunner required packages

There was a review comment about this in #13382 (comment) that was never addressed.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13562)
<!-- Reviewable:end -->
@glennw glennw deleted the glennw:headless branch Dec 12, 2016
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 4, 2017
…rom servo:jdm-patch-5); r=jgraham

There was a review comment about this in servo/servo#13382 (comment) that was never addressed.

Source-Repo: https://github.com/servo/servo
Source-Revision: c10de40e4ea4430037ac9156cc2b5d31f92a7cd4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…rom servo:jdm-patch-5); r=jgraham

There was a review comment about this in servo/servo#13382 (comment) that was never addressed.

Source-Repo: https://github.com/servo/servo
Source-Revision: c10de40e4ea4430037ac9156cc2b5d31f92a7cd4

UltraBlame original commit: 3c1adde304c06e01eb22151a5cea65c35651021e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…rom servo:jdm-patch-5); r=jgraham

There was a review comment about this in servo/servo#13382 (comment) that was never addressed.

Source-Repo: https://github.com/servo/servo
Source-Revision: c10de40e4ea4430037ac9156cc2b5d31f92a7cd4

UltraBlame original commit: 3c1adde304c06e01eb22151a5cea65c35651021e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…rom servo:jdm-patch-5); r=jgraham

There was a review comment about this in servo/servo#13382 (comment) that was never addressed.

Source-Repo: https://github.com/servo/servo
Source-Revision: c10de40e4ea4430037ac9156cc2b5d31f92a7cd4

UltraBlame original commit: 3c1adde304c06e01eb22151a5cea65c35651021e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.