Closed
Bug 1374748
Opened 7 years ago
Closed 7 years ago
Run Stylo Linux64 tests using regular builds and stop building linux64-stylo builds
Categories
(Firefox Build System :: Task Configuration, task, P3)
Firefox Build System
Task Configuration
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla56
People
(Reporter: cpeterson, Assigned: jryans)
References
Details
Attachments
(3 files, 4 obsolete files)
After we start building Stylo support in regular Linux64 builds (pref'd off) in bug 1356991, we can: 1. Run Stylo tests using regular builds (with a pref flip or environment variable like WebRender's linux64-qr tests) instead of the linux64-stylo builds. 2. Stop building linux64-stylo. Running Stylo tests on platforms other than Linux64 can happen later.
Comment 3•7 years ago
|
||
patch to disable stylo specific builds and run the stylo-refests with a pref enabled I also disabled the sequential tests please confirm that this is expected
Comment 4•7 years ago
|
||
Builder diff if these changes were implemented on m-c cpeterson please confirm that this is what you are looking for
Flags: needinfo?(cpeterson)
Comment 5•7 years ago
|
||
Also I noticed that there are stylo jobs running on beta now, can these be disabled?
Reporter | ||
Comment 6•7 years ago
|
||
(In reply to Kim Moir [:kmoir] from comment #5) > Also I noticed that there are stylo jobs running on beta now, can these be > disabled? Yes. We don't need to run Stylo jobs on Beta 55 (or Beta 56 next cycle). We will want them for Beta 57 and thereafter, if all goes well.
Flags: needinfo?(cpeterson)
Reporter | ||
Comment 7•7 years ago
|
||
(In reply to Kim Moir [:kmoir] from comment #3) > Created attachment 8880856 [details] [diff] [review] > bug1374748.patch > > patch to disable stylo specific builds and run the stylo-refests with a pref > enabled > > I also disabled the sequential tests please confirm that this is expected We will still want to run Stylo some tests in parallel (default) mode and sequential mode (bug 1356122) and some in e10s and non-e10s mode. I updated our test matrix in the Stylo spreadsheet: https://docs.google.com/spreadsheets/d/1HmBsxrEpFfCgpLBG5Mvj5wEA9oEq5N8m4cxQYsSo4S8/edit#gid=1737640065 (In reply to Kim Moir [:kmoir] from comment #4) > Created attachment 8880859 [details] > m-cbuilder.diff > > Builder diff if these changes were implemented on m-c > > cpeterson please confirm that this is what you are looking for It looks like this diff removes Stylo crashtests, mochitests, Talos, and web-platform-tests and non-e10s mode. We will still want to run all the test suites we're running now.
Comment 8•7 years ago
|
||
So this is a bit tricky because you want the stylo tests to run in parallel with the non-stylo tests but they have all have the same name. Is this an an accurate assessment? For instance Run crashtest against linux64/opt Run crashtest against linux64/opt with '--parallel-stylo-traversal and (enable stylo pref) and layout.css.servo.enabled=true for parallel tests Run crashtest against linux64/opt with '--setperf=layout.css.servo.enabled=true' for sequential tests
Flags: needinfo?(cpeterson)
Reporter | ||
Comment 9•7 years ago
|
||
(In reply to Kim Moir [:kmoir] from comment #8) > So this is a bit tricky because you want the stylo tests to run in parallel > with the non-stylo tests but they have all have the same name. Is this an an > accurate assessment? > > For instance > Run crashtest against linux64/opt > Run crashtest against linux64/opt with '--parallel-stylo-traversal and > (enable stylo pref) and layout.css.servo.enabled=true for parallel tests > Run crashtest against linux64/opt with > '--setperf=layout.css.servo.enabled=true' for sequential tests I think that is correct. Would adding new names for these Stylo tests make configuration easier? It looks like Treeherder has separate test names for linux64-stylo's reftests: opt-reftest-e10s (R1-R8) and opt-reftest-stylo-e10s (Rs1-Rs16).
Flags: needinfo?(cpeterson) → needinfo?(kmoir)
Updated•7 years ago
|
Attachment #8880856 -
Attachment is obsolete: true
Flags: needinfo?(kmoir)
Updated•7 years ago
|
Attachment #8880859 -
Attachment is obsolete: true
Comment 10•7 years ago
|
||
patch to disable stylo jobs from running on beta. will attach diff Some of the tests weren't limited by branch
Attachment #8881284 -
Flags: review?(aki)
Comment 11•7 years ago
|
||
Updated•7 years ago
|
Attachment #8881284 -
Flags: review?(aki) → review+
Comment 13•7 years ago
|
||
Pushed by kmoir@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c923a5e8169f Run Stylo tests using regular builds and stop building linux64-stylo builds r=aki DONTBUILD
Updated•7 years ago
|
Keywords: leave-open
Comment 15•7 years ago
|
||
Given that we need to set a couple of properties and run the regular set of tests against linux64 builds as well as the stylo ones, I think we need to set a property in the same way that they e10s tests run. i.e. setting this flag essentially allows the tests to be duplicated and copied with new properties like in @transforms.add def split_e10s(config, tests): in taskcluster/taskgraph/transforms/tests.py and give them new names as well to distinguish them from non-stylo tests.
Comment 16•7 years ago
|
||
I tried a few permutations of this but wasn't able to get it working before I left for PTO.
Comment 17•7 years ago
|
||
I took a quick look at this, and found one problem that's going to make this hard to do: there's no common way to enable stylo across test suites. It would be nice if --enable-stylo worked for mozharness and the various underlying suites, but that's not the case right now. Another approach would be to enable stylo if an environment variable is set, e.g. MOZ_STYLO=1, but that also is not supported right now.
Reporter | ||
Comment 18•7 years ago
|
||
(In reply to Chris AtLee [:catlee] from comment #17) > It would be nice if --enable-stylo worked for mozharness and the various > underlying suites, but that's not the case right now. Another approach would > be to enable stylo if an environment variable is set, e.g. MOZ_STYLO=1, but > that also is not supported right now. So an environment variable would be adequate? How is Quantum Render enabled for the linux64-qr test runs? Setting an environment variable sounds a lot easier than adding --enable-stylo flags to multiple test suites for a feature that will soon by enabled by default.
Flags: needinfo?(catlee)
Comment 19•7 years ago
|
||
The QR tests use an environment variable MOZ_WEBRENDER, which is set in configs in places like this: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/desktop_unittest.py#709 And then used in the code here: https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#2388 I agree that using an environment various sounds a lot easier right now. In general though, I do think it would be nice if test harnesses all supported setting arbitrary prefs. Some already do, judging by some green test runs on this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9159ff2650470993fc39126ab279589521b03b23
Flags: needinfo?(catlee)
Comment 20•7 years ago
|
||
Given that MOZ_STYLO is already the name of our #define, I think FORCE_ENABLE_STYLO is a better name. Easiest way is to check the env var here: http://searchfox.org/mozilla-central/rev/238406d4c1b3f147522ce0a45a4c6f84a8115781/layout/base/nsLayoutUtils.cpp#7798 If the var is set, we just set sStyloEnabled to true and avoid hooking it up to the pref system.
Comment 22•7 years ago
|
||
How about using STYLO_THREADS as a proxy? If you set it to a positive value, it uses that number of threads, if you set it to 0 (or -1, maybe), it finds a good number of threads, and you don't set it, stylo is not enabled.
Comment 23•7 years ago
|
||
STYLO_THREADS is already overloaded and used in a entirely different part of the code. I don't see any advantage to overloading it more for this very specific and short-lived use-case of overriding the normal pref mechanism.
Assignee | ||
Comment 25•7 years ago
|
||
With bug 1379300 landed, you should be able to use STYLO_FORCE_ENABLED=1 for this purpose.
Reporter | ||
Comment 26•7 years ago
|
||
Now that we have Stylo support in Nightly builds for Linux64, Mac, and Windows, we should run our Stylo tests on those platforms. We should run all the same test configurations as we do for linux64-stylo on inbound, autoland, and central: https://docs.google.com/spreadsheets/d/1HmBsxrEpFfCgpLBG5Mvj5wEA9oEq5N8m4cxQYsSo4S8/edit#gid=1737640065 We don't need to run the Mac and Windows tests in Stylo sequential mode, just parallel mode.
Summary: Run Stylo tests using regular builds and stop building linux64-stylo builds → Run Stylo tests using regular builds (Linux, Mac, and Windows) and stop building linux64-stylo builds
Comment 27•7 years ago
|
||
Simply setting the env var isn't enough. The mozinfo file claims that stylo is off, which means that all of the skip-if=stylo tests run.
Assignee | ||
Comment 28•7 years ago
|
||
:catlee attempted a try run to use the STYLO_FORCE_ENABLED=1 env var, but there's an additional issue of the test annotations. Some tests use mozinfo to decide what tests should run, and this checks MOZ_STYLO_ENABLE from build time when setting the `stylo` field, which will be false if we use only the STYLO_FORCE_ENABLED=1 env var. This means in the current state, Stylo is getting enabled, but the test harness thinks it is disabled, so it tries to run tests that are known to fail with Stylo. :ahal, is there a good place to override mozinfo during test runs to adjust mozinfo based on an env var?
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(ahalberstadt)
Comment 29•7 years ago
|
||
Is this for mochitest? Looks like there's an --extra-mozinfo-json argument that will do what you want: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/mochitest_options.py#356
Flags: needinfo?(ahalberstadt)
Assignee | ||
Comment 30•7 years ago
|
||
Okay, I filed bug 1380082 to update the test harnesses to run the right tests with the env var.
Reporter | ||
Comment 31•7 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #29) > Is this for mochitest? Looks like there's an --extra-mozinfo-json argument > that will do what you want: > https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/ > mochitest_options.py#356 We currently run mochitests, reftests, crashtests, and web-platform-tests with Stylo enabled. Also, we run reftests in two different Stylo modes: comparing Stylo-vs-Stylo (similar to the default Gecko-vs-Gecko mode) and Stylo-vs-Gecko. This spreadsheet describes the various combinations of tests and Stylo/e10s modes we run on the inbound, autoland, and central repos: https://docs.google.com/spreadsheets/d/1HmBsxrEpFfCgpLBG5Mvj5wEA9oEq5N8m4cxQYsSo4S8/edit#gid=1737640065
Comment 32•7 years ago
|
||
Ah ok, it's probably best to handle each of those within the respective harnesses since they use different manifest formats. I think this is most analogous to e10s, if you foresee developers wanting to run this locally a lot, it may be worth adding a --stylo flag to the harnesses or something.
Assignee | ||
Comment 33•7 years ago
|
||
Here's a tweaked version of :catlee's changes to switch back to regular builds using the env var. This depends on other patches in bug 1380082. Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66695eac04fe2aa9e5514c05edd85714a896cf97 I am not sure what other steps might be needed, so I'll leave this part here for someone else to pick up.
Reporter | ||
Comment 34•7 years ago
|
||
Kim, Ryan is configuring the Stylo tests to use regular builds (instead of linux64-stylo) and to run on Mac and Windows. Once he has the tests running, you can turn off the linux64-stylo custom builds. In the meantime, I don't think you need to do anything for this bug.
Assignee: kmoir → jryans
Assignee | ||
Comment 35•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #33) > Created attachment 8885934 [details] [diff] [review] > Use regular builds for Stylo via env var > > Here's a tweaked version of :catlee's changes to switch back to regular > builds using the env var. > > This depends on other patches in bug 1380082. Here's a try run: > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=66695eac04fe2aa9e5514c05edd85714a896cf97 > > I am not sure what other steps might be needed, so I'll leave this part here > for someone else to pick up. Linux64 appears to work in this mode (with just the env var). I am still triaging the state of other desktop platforms in bug 1380053.
Assignee | ||
Comment 36•7 years ago
|
||
After talking on IRC, updating the scope of this bug to focus on the Linux64 part only. For Stylo tests on other desktop platforms, we'll use the separate bug 1380053 (first to check tests, perhaps another is needed for actual task changes). With Linux64, the tests appear green in my last try run from comment 33: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66695eac04fe2aa9e5514c05edd85714a896cf97 However, the special Stylo builds still ran, so it seems more steps are needed to actually remove them. I don't think I am best placed to make this change, or at least I would need some pointers on what remains. :catlee, since you started the WIP patch I used above, do you know what's left here?
Assignee: jryans → nobody
Flags: needinfo?(catlee)
Summary: Run Stylo tests using regular builds (Linux, Mac, and Windows) and stop building linux64-stylo builds → Run Stylo Linux64 tests using regular builds and stop building linux64-stylo builds
Assignee | ||
Comment 37•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #36) > However, the special Stylo builds still ran, so it seems more steps are > needed to actually remove them. Ah, my mistake, the linux64-stylo builds do appear to be gone in the try run. So maybe it's ready to go, but there could be other bits to clean up that I am unaware of.
Assignee | ||
Comment 38•7 years ago
|
||
The try run in comment 33 was tests only, let's verify Talos as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfb836a87a9c17f9b0aeb63b7356ae5e8762b0c2
Assignee | ||
Comment 39•7 years ago
|
||
Talos is driven by Buildbot, so we need a different way to enable Stylo. Trying that based on :catlee's suggestion: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb76d4a7687b282e50a52b95647051ecf707270c
Assignee | ||
Comment 40•7 years ago
|
||
Another attempt to activate Stylo for Talos doesn't seem to be working: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a547800299e604ee7f7810edb044b4b76106dc9
Assignee | ||
Comment 41•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40) > Another attempt to activate Stylo for Talos doesn't seem to be working: > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=9a547800299e604ee7f7810edb044b4b76106dc9 Ah, my mistake, I must have opened the wrong log at first. :( Okay, I'll fold this version into my patch.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcc9acf7d58bbeae8bda44f78798e3947817d263
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Flags: needinfo?(catlee)
Assignee | ||
Updated•7 years ago
|
Attachment #8885934 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 44•7 years ago
|
||
mozreview-review |
Comment on attachment 8887711 [details] Bug 1374748 - Use regular builds for Stylo via env var. https://reviewboard.mozilla.org/r/158616/#review164244
Attachment #8887711 -
Flags: review?(catlee) → review+
Comment hidden (mozreview-request) |
Comment 46•7 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c21ec4c07f82 Use regular builds for Stylo via env var. r=catlee
Backing this out for breaking talos on at least Mac platforms. Regular mac builds: https://treeherder.mozilla.org/logviewer.html#?job_id=115672376&repo=autoland Devedition mac builds: https://treeherder.mozilla.org/logviewer.html#?job_id=115672403&repo=autoland https://hg.mozilla.org/integration/autoland/rev/22e24b1ada9431c21c7f1f8a950d68ec94842c83
Flags: needinfo?(jryans)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 49•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64a679a29253ad346446dce32cda8a59f1477665
Flags: needinfo?(jryans)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 51•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=442aec6e50e0f8e95ace7ad0945a9fbf0cb6eb96
Comment 52•7 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6c77cacaf82d Use regular builds for Stylo via env var. r=catlee
Comment 53•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6c77cacaf82d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Product: TaskCluster → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•