Status

task
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: kmoir, Assigned: kmoir)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
email from cpeterson
--
hi Kim, I just wanted to give you a heads up that, in a couple weeks, the Quantum Style team would like to add a new build config (if that is the correct term?) to Treeherder running on mozilla-central (and inbound and autoland) pushes. We would want this temporary build config ("linux64-stylo") to run the usual build and test jobs (with some Stylo flags enabled). We could then retire the tests running in the incubator/stylo repo.

We're tracking the dependencies in meta bug 1330414 (alias "stylo-central"): Run linux64-stylo tests in mozilla-central.

https://bugzilla.mozilla.org/showdependencytree.cgi?id=1330414&hide_resolved=1

--

questions from me

Is there a mozconfig concurrently defined?
Is branding different?
please define list of tests that need to run
talos required or not?
Does this build need to run on every commit or could it run periodically (to reduce AWS costs)
(Assignee)

Updated

2 years ago
Assignee: nobody → kmoir
(Assignee)

Comment 1

2 years ago
Another question, will these additional builds + tests ride the trains and be uplifted to aurora or will the preferences just be merged into regular firefox builds at some point and we'll be able to disable the style specific builds and tests.
Flags: needinfo?(cpeterson)
(In reply to Kim Moir [:kmoir] from comment #1)
> Another question, will these additional builds + tests ride the trains and
> be uplifted to aurora or will the preferences just be merged into regular
> firefox builds at some point and we'll be able to disable the style specific
> builds and tests.

I don't think we've fully sorted that out yet. Does it have a significant impact on what we do now?

At present, the aim for this configuration is just to get the stylo builds green on CI (incrementally turning more tests on), and then flip the switch on Nightly. Once that's happened, the proposed linux64-stylo configuration will have served its purpose.

After that, we'll need to sort out exactly what kind of rollout we want for Quantum, in particular whether all the quantum projects ride the trains together, and what kind of CI we want to keep running on non-quantum builds in case we need to flip parts of quantum off.

Ideally we could punt on that discussion until later. But if there are reasons why it affects our decision-making now, I'd like to understand them.
Flags: needinfo?(cpeterson)
(In reply to Kim Moir [:kmoir] from comment #1)
> Another question, will these additional builds + tests ride the trains and
> be uplifted to aurora or will the preferences just be merged into regular
> firefox builds at some point and we'll be able to disable the style specific
> builds and tests.

We won't need these linux64-stylo builds + tests to ride the trains.

When Stylo is ready to ride the trains, it will be enabled in regular Firefox builds (perhaps in just one one platform initially to test the waters). At that time, we might want a "linux64-nostylo" or whatever build config to ride the trains to ensure we have don't break the non-Stylo code, in case we need to disable Stylo at the last minute. We did something similar for e10s.
Priority: -- → P2
(Assignee)

Comment 4

2 years ago
> I don't think we've fully sorted that out yet. Does it have a significant impact on what we do now?

No, that's fine, thanks

> from email from cpeterson

    Does this build need to run on every commit or could it run periodically (to reduce AWS costs)

As a first step, I think we can just focus on running it on m-c, which should make the AWS cost a non issue. We can weight the rest of the tradeoffs later (unless we need to now for budgeting reasons).


kmoir> Thanks this is good to know, I will chat with sheriffs to see if they see any issues with this (they probably don't want failures on this build on merges if this code is not exercised on m-i etc)
(In reply to Kim Moir [:kmoir] from comment #4)
> kmoir> Thanks this is good to know, I will chat with sheriffs to see if they
> see any issues with this (they probably don't want failures on this build on
> merges if this code is not exercised on m-i etc)

In the first phase I'm talking about, the builds would be hidden by default. We would certainly need the builds on inbound if they were going to be tier 1.
(Assignee)

Comment 6

2 years ago
So to consolidate the list of requirements
this is a tier 2 build 
will be initially hidden on m-c only
no branding changes
once it is now longer required and code is merged into the regular nightly it will be deleted
perhaps a need for a no-stylo build at this time, to verify that tests still work there
small set of tests initially, this will expand
mozonfig is the same as the existing linux64 build but with --enable-stylo
(In reply to Kim Moir [:kmoir] from comment #6)
> So to consolidate the list of requirements
> this is a tier 2 build
> will be initially hidden on m-c only
> no branding changes
> once it is now longer required and code is merged into the regular nightly
> it will be deleted
> perhaps a need for a no-stylo build at this time, to verify that tests still
> work there
> small set of tests initially, this will expand
> mozonfig is the same as the existing linux64 build but with --enable-stylo

That all sounds right.
(Assignee)

Comment 8

2 years ago
Posted patch wip patch (obsolete) — Splinter Review
(Assignee)

Updated

2 years ago
Depends on: 1318200
(Assignee)

Comment 9

2 years ago
Posted patch stylo-2.patch (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f26a8223817ed1dba643db1d8381c13798db159

task graph with builds and tests enabled
https://public-artifacts.taskcluster.net/b65JrIfvTU6jTwiQuJ-v0w/0/public/full-task-graph.json

I'll setup a hook or use dustin's new in tree cron stuff once the other dependent bugs are resolved
Attachment #8828537 - Attachment is obsolete: true
Attachment #8828883 - Flags: review?(mtabara)
Comment on attachment 8828883 [details] [diff] [review]
stylo-2.patch

Review of attachment 8828883 [details] [diff] [review]:
-----------------------------------------------------------------

::: taskcluster/ci/build/linux.yml
@@ +354,5 @@
> +        product: firefox
> +        job-name: linux64-stylo-opt
> +    treeherder:
> +        platform: linux64/stylo
> +        symbol: tc(Bo)

Not sure I'm right here, but isn't this conflicting with ASan `Bo` builds symbols in the tree?

@@ +380,5 @@
> +        product: firefox
> +        job-name: linux64-stylo-debug
> +    treeherder:
> +        platform: linux64/stylo
> +        symbol: tc(Bd)

Isn't this conflicting with ASan `Bd` builds symbols in the tree?
Attachment #8828883 - Flags: review?(mtabara) → review+
(Assignee)

Comment 11

2 years ago
Posted patch stylo-3.patchSplinter Review
Attachment #8828883 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Depends on: 1322769
There are now 2 linux64-stylo/opt and linux64-stylo/debug sections in taskcluster/ci/build/linux.yml. These need to be reconciled. Needinfo kmoir for that.

Also, needinfo dustin so he is aware that duplicate keys can exist in YAML files. I'm, uh, kinda surprised that's not a parse error in YAML. IMO it should be caught in the taskgraph layer.
Flags: needinfo?(kmoir)
Flags: needinfo?(dustin)
(Assignee)

Comment 15

2 years ago
I just removed the extra stanzas
Flags: needinfo?(kmoir)

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f05517eee951
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Flags: needinfo?(dustin)
I have noticed that the Python YAML parser doesn't have an issue with duplicate keys, although the JS parser we use does.  Without regexes or a different parser, I don't know how we could catch that, sadly.
(Assignee)

Comment 19

2 years ago
reopening because I need to enable the tc hook once the issues in bug 1322769 are resolved
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I guess we are close to working on mozilla-central, but it looks like this patch has stopped the reftest-stylo jobs running on the stylo repo.  Can we have them continue running until we're ready to move over fully?  https://treeherder.mozilla.org/#/jobs?repo=stylo&revision=f8e2a5d85f4777a43bf8cf2c50ee04e02eb37ae7&filter-searchStr=linux64-stylo
Flags: needinfo?(kmoir)
(Assignee)

Comment 21

2 years ago
okay, I'm investigating a patch for that now
Flags: needinfo?(kmoir)
(Assignee)

Comment 22

2 years ago
Posted patch stylo3.patchSplinter Review
I think this will fix it
(Assignee)

Updated

2 years ago
Attachment #8831738 - Flags: checked-in+
Thanks Kim.  I've landed that on the stylo repo now.
Depends on: 1338871
(Assignee)

Comment 24

2 years ago
Going to close this even though the talos dependant bug 1338871  is not complete, it has still been assigned to me
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Component: Platform Support → Buildduty
Product: Release Engineering → Infrastructure & Operations
You need to log in before you can comment on or make changes to this bug.