Abstract platform-specific workflows into separate files#29534
Merged
bors-servo merged 2 commits intoservo:masterfrom Mar 31, 2023
Merged
Abstract platform-specific workflows into separate files#29534bors-servo merged 2 commits intoservo:masterfrom
bors-servo merged 2 commits intoservo:masterfrom
Conversation
sagudev
force-pushed
the
multiflows
branch
2 times, most recently
from
a2b131d to
87ee46d
Compare
mrobinson
reviewed
Mar 22, 2023
Contributor
|
☔ The latest upstream changes (presumably #29529) made this pull request unmergeable. Please resolve the merge conflicts. |
Contributor
|
☔ The latest upstream changes (presumably #29528) made this pull request unmergeable. Please resolve the merge conflicts. |
Member
Author
|
@mrobinson what is the status with running WPT test on layout-2020? Do we need separate trackers for or how it goes. Currently I am testing modular workflows with inputs. The goal is to make testing layout-2020 vs 2013 just matter of workflow flags. |
Member
The first step, which is almost complete, is to ensure that Layout 2020 builds properly when landing new changes. After that we need to generate results for Layout 2020 during WPT import. Once that's complete, we should be able to run tests for Layout 2020 during the main workflow. The remaining question for me about the final part is if it should wait until the project has decided on a layout engine. |
sagudev
force-pushed
the
multiflows
branch
2 times, most recently
from
07df44f to
18aaf8e
Compare
Member
Author
|
Okay, so I enabled layout2020 checks in PR and in main check. Both have unit test disabled as it has the following failures: Compiling layout_2020 v0.0.1 (/home/runner/work/servo/servo/components/layout_2020)
error[E0404]: expected trait, found struct `Gen`
--> components/layout_2020/tests/floats.rs:62:12
|
62 | G: Gen,
| ^^^ not a trait
error[E0404]: expected trait, found struct `Gen`
--> components/layout_2020/tests/floats.rs:86:12
|
86 | G: Gen,
| ^^^ not a trait
error[E0404]: expected trait, found struct `Gen`
--> components/layout_2020/tests/floats.rs:349:12
|
349 | G: Gen,
| ^^^ not a trait |
Contributor
|
☔ The latest upstream changes (presumably #29538) made this pull request unmergeable. Please resolve the merge conflicts. |
Member
Author
|
@mrobinson Are Intermittent errors dependent on layout and should both layouts have separate report_agregate_results handling? There is still some space left for unification (nightly-rust, nightly, wpt-nightly) but that would require some additional work and I would rather put it off until this lands as this PR is big enough already. |
mrobinson
reviewed
Mar 27, 2023
Member
mrobinson
left a comment
mrobinson
left a comment
There was a problem hiding this comment.
Thanks for working on this. A couple thoughts:
- I think it might be good to split this change up into multiple pull requests, as there are a few unrelated things going on here. For instance, turning on sccache seems like an interesting idea, but if something goes wrong and we need to revert it, we'll end up reverting the split of the platform-specific workflows as well. Maybe this change can simply handle splitting the workflows?
- I think it's useful that the test results show up in a GitHub comment. It makes them easier to find and the idea is that we can gradually work on reducing the amount of tests that are flaky because the results are right there.
- By splitting the Layout 2013 and Layout 2020 build jobs, we duplicate more effort whereas before Layout 2020 added about 7 minutes to the total build and used one less builder. I suppose there is a tradeoff there, as total job time might be less now, but what was your thought process here?
Member
Author
That was actually my initial plan, but it was useful to have cache while testing. Will remove sccache and make it in new PR. Should WPT github chacks also be split into separate PR?
I am actually just adding WPT to github checks on top of github comments. What I was asking was if different layout should have different comments and WPT checks or do we only show layout 2013 results as we are now.
It just felt more natural to have separate builds for them. The faster the linux built finishes the faster WPT checks starts, so WPT tests profit from building per layout in separate builds. Also I believe 7 min is only accounting for building as we currently do not run unit-tests for layout-2020, but in near future we certainly will, and then separate builds will make more sense in quick-check to. |
Member
Yes, that would be really useful if it's not too much work. Thank you!
Oh, thanks for the clarification. I think that's fine then. For some background, we are actually in a very interesting moment in regards to Layout 2013 and Layout 2020. We are in the process of choosing one of these layout engines to focus on. One possibility, is that the other layout engine will be removed from the tree in order to focus development efforts. It's unclear to me if we will turn on tests for Layout 2020 before or after this moment, and obviously it depends on what the result of the choice is. Really, this just needs a wider discussion to figure out what the best path forward is and what order to do things in.
Hopefully, the context above sheds some light here as well. That said, unit tests might be a good place to start with this process and then WPT tests can follow. That could avoid overwhelming the builders right away. Right now dedicated 40 builders to a single WPT run might be too much to ask though. |
Member
Author
|
Also what is good with this modular approach is that, that any test (ex. wpt on layout-2013) can be easily switched on or off, which is very useful in this times.
We could run layout-2020 WPT test only on PRs that change any layout-2020 files, this should be pretty trivial change in main.yml after this PR. |
mrobinson
requested changes
Mar 28, 2023
mrobinson
changed the title
sagudev
changed the title
sagudev
changed the title
|
Results from try job (#4571674861): Flaky unexpected result (24)
Stable unexpected results that are known to be intermittent (32)
Stable unexpected results (1)
|
Member
|
@bors-servo retry |
Contributor
bors-servo
added a commit
that referenced
this pull request
Mar 31, 2023
Abstract platform-specific workflows into separate files
Changes:
- split main workflow into multiple modular files & merged linux and quickcheck
- easily allow testing both `layout-2013` and `layout-2020` (currently layout-2020 is build on both quick-check and main workflows, but no actual tests are run)
- workflow_dispatch on any try run
Future work:
- sccache for caching
- linux (release, layout-2013): 30min -> 15min
- mac: 1h 30min -> 50min
- windows: not working yet:
```console
sccache: encountered fatal error
sccache: error: Invalid timestamp field in entry header ("-1 ")
sccache: caused by: Invalid timestamp field in entry header ("-1 ")
error: could not compile `openssl-sys`
Caused by:
process didn't exit successfully: `sccache rustc --crate-name openssl_sys --edition=2018 C:\Users\runneradmin\.cargo\registry\src\github.com-1ecc6299db9ec823\openssl-sys-0.9.81\src\lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -C metadata=15e5a7651b4bbdb6 -C extra-filename=-15e5a7651b4bbdb6 --out-dir C:\a\servo\servo\target\release\deps -C linker=lld-link.exe -L dependency=C:\a\servo\servo\target\release\deps --extern libc=C:\a\servo\servo\target\release\deps\liblibc-1fe4b5632b2d1333.rmeta --cap-lints allow -W unused-extern-crates -L native=C:\a\servo\servo\.servo\msvc-dependencies\openssl\111.3.0+1.1.1c-vs2017-2019-09-18\x64-windows\lib -l static=libssl -l static=libcrypto -l dylib=gdi32 -l dylib=user32 -l dylib=crypt32 -l dylib=ws2_32 -l dylib=advapi32 --cfg const_fn --cfg openssl --cfg "osslconf=\"OPENSSL_NO_COMP\"" --cfg "osslconf=\"OPENSSL_NO_SSL3_METHOD\"" --cfg ossl101 --cfg ossl102 --cfg ossl102f --cfg ossl102h --cfg ossl110 --cfg ossl110f --cfg ossl110g --cfg ossl110h --cfg ossl111 --cfg ossl111b --cfg ossl111c` (exit code: 0xfffffffe)
warning: build failed, waiting for other jobs to finish...
```
- report WPT test results also using GitHub checks (so we get those sexy results even when there is no associated PR) [example](https://github.com/sagudev/servo/runs/12193772513)
- run `WPT export` only when PR changes files in `tests/wpt/**`
Member
There are also the nightly builds, which can run concurrently with auto/try builds, but hopefully there won’t be too much delay caused by contention with those. |
Contributor
|
💡 This pull request was already approved, no need to approve it again.
|
Contributor
|
📌 Commit 8959c57 has been approved by |
Contributor
bors-servo
added a commit
that referenced
this pull request
Mar 31, 2023
Abstract platform-specific workflows into separate files
Changes:
- split main workflow into multiple modular files & merged linux and quickcheck
- easily allow testing both `layout-2013` and `layout-2020` (currently layout-2020 is build on both quick-check and main workflows, but no actual tests are run)
- workflow_dispatch on any try run
Future work:
- sccache for caching
- linux (release, layout-2013): 30min -> 15min
- mac: 1h 30min -> 50min
- windows: not working yet:
```console
sccache: encountered fatal error
sccache: error: Invalid timestamp field in entry header ("-1 ")
sccache: caused by: Invalid timestamp field in entry header ("-1 ")
error: could not compile `openssl-sys`
Caused by:
process didn't exit successfully: `sccache rustc --crate-name openssl_sys --edition=2018 C:\Users\runneradmin\.cargo\registry\src\github.com-1ecc6299db9ec823\openssl-sys-0.9.81\src\lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -C metadata=15e5a7651b4bbdb6 -C extra-filename=-15e5a7651b4bbdb6 --out-dir C:\a\servo\servo\target\release\deps -C linker=lld-link.exe -L dependency=C:\a\servo\servo\target\release\deps --extern libc=C:\a\servo\servo\target\release\deps\liblibc-1fe4b5632b2d1333.rmeta --cap-lints allow -W unused-extern-crates -L native=C:\a\servo\servo\.servo\msvc-dependencies\openssl\111.3.0+1.1.1c-vs2017-2019-09-18\x64-windows\lib -l static=libssl -l static=libcrypto -l dylib=gdi32 -l dylib=user32 -l dylib=crypt32 -l dylib=ws2_32 -l dylib=advapi32 --cfg const_fn --cfg openssl --cfg "osslconf=\"OPENSSL_NO_COMP\"" --cfg "osslconf=\"OPENSSL_NO_SSL3_METHOD\"" --cfg ossl101 --cfg ossl102 --cfg ossl102f --cfg ossl102h --cfg ossl110 --cfg ossl110f --cfg ossl110g --cfg ossl110h --cfg ossl111 --cfg ossl111b --cfg ossl111c` (exit code: 0xfffffffe)
warning: build failed, waiting for other jobs to finish...
```
- report WPT test results also using GitHub checks (so we get those sexy results even when there is no associated PR) [example](https://github.com/sagudev/servo/runs/12193772513)
- run `WPT export` only when PR changes files in `tests/wpt/**`
|
Results from try job (#4572276108): Flaky unexpected result (17)
Stable unexpected results that are known to be intermittent (34)
Stable unexpected results (1)
|
|
Results from try job (#4572252457): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (36)
|
Contributor
|
💔 Test failed - checks-github |
Member
|
@bors-servo retry |
Member
Author
|
@bors-servo ping |
Contributor
|
😪 I'm awake I'm awake |
Member
|
I think @bors-servo is running tests for another PR now so it will take some time to run tests for this PR. |
Contributor
|
Results from try job (#4574242172): Flaky unexpected result (27)
Stable unexpected results that are known to be intermittent (23)
|
Contributor
|
☀️ Test successful - checks-github |
bors-servo
added a commit
that referenced
this pull request
Apr 1, 2023
Update wpt-nightly-update.sh with right paths from workflows Fixup to #29534 as wpt-logs-linux are separated per folders not per file.
bors-servo
added a commit
that referenced
this pull request
Apr 2, 2023
Sccache in actions (linux & mac) Coming from #29534. - linux (release, layout-2013): 30min -> 15min - mac: 1h 30min -> 50min - windows: not working yet Windows problems are very weird, when using `RUSTC_WRAPPER: sccache` we get: ```console sccache: encountered fatal error sccache: error: Invalid timestamp field in entry header ("-1 ") sccache: caused by: Invalid timestamp field in entry header ("-1 ") error: could not compile `openssl-sys` Caused by: process didn't exit successfully: `sccache rustc --crate-name openssl_sys --edition=2018 C:\Users\runneradmin\.cargo\registry\src\github.com-1ecc6299db9ec823\openssl-sys-0.9.81\src\lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -C metadata=15e5a7651b4bbdb6 -C extra-filename=-15e5a7651b4bbdb6 --out-dir C:\a\servo\servo\target\release\deps -C linker=lld-link.exe -L dependency=C:\a\servo\servo\target\release\deps --extern libc=C:\a\servo\servo\target\release\deps\liblibc-1fe4b5632b2d1333.rmeta --cap-lints allow -W unused-extern-crates -L native=C:\a\servo\servo\.servo\msvc-dependencies\openssl\111.3.0+1.1.1c-vs2017-2019-09-18\x64-windows\lib -l static=libssl -l static=libcrypto -l dylib=gdi32 -l dylib=user32 -l dylib=crypt32 -l dylib=ws2_32 -l dylib=advapi32 --cfg const_fn --cfg openssl --cfg "osslconf=\"OPENSSL_NO_COMP\"" --cfg "osslconf=\"OPENSSL_NO_SSL3_METHOD\"" --cfg ossl101 --cfg ossl102 --cfg ossl102f --cfg ossl102h --cfg ossl110 --cfg ossl110f --cfg ossl110g --cfg ossl110h --cfg ossl111 --cfg ossl111b --cfg ossl111c` (exit code: 0xfffffffe) warning: build failed, waiting for other jobs to finish... ``` and when using only `CCACHE: sccache` we get [0 cache hits/misses which is weird as at least mozjs is using CCACHE variable and should have same sccache requests](https://github.com/sagudev/servo/actions/runs/4587126100/jobs/8100424378).
bors-servo
added a commit
that referenced
this pull request
Apr 4, 2023
WPT aggregated results also as github check Report WPT aggregated results also using GitHub check (so we get those sexy results even when there is no associated PR). [example](https://github.com/sagudev/servo/runs/12193772513) Last missing component coming from #29534.
bors-servo
added a commit
that referenced
this pull request
Apr 4, 2023
WPT aggregated results also as github check Report WPT aggregated results also using GitHub check (so we get those sexy results even when there is no associated PR). [example](https://github.com/sagudev/servo/runs/12193772513) Last missing component coming from #29534.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes:
layout-2013andlayout-2020(currently layout-2020 is build on both quick-check and main workflows, but no actual tests are run)Future work:
WPT exportonly when PR changes files intests/wpt/**