Run stylo automation when servo is available

RESOLVED FIXED in Firefox 53

Status

()

Core
Build Config
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
The "stylo" repository currently has some minimally hacked up automation configs to enable stylo. These changes aren't merged into mozilla-central.

In this bug, I'd like to get those configs added to mozilla-central.

The proposal is pretty simple to describe. We'll add a new build configuration for "stylo" (similar to how e10s was done). This build configuration essentially features --enable-stylo into configure.

We'll automatically schedule this build configuration in the decision task if the current checkout has a "servo" directory. This will currently limit scheduling to commits in the "stylo" repository.

This approach is opposite of how automation is hacked up in stylo today. Currently, they have "nostylo" build configuration. While I'd like to preserve their work, I think having "nostylo" on the "stylo" repository == "normal" on every other repository will confuse things like the intermittent classifier. So we should be consistent across repositories and introduce a new build configuration for "stylo" (like we did for e10s).

The changes in this bug should not impact the automation running on mozilla-central or any other non-stylo repositories in any way.
Having stylo as a separate config (rather than nostylo) sounds fine to me.
Testing both with and without e10s has, roughly, doubled our testing load.  Testing is expensive, so if we are considering doing the same thing with stylo, we'll need to consider that from a budget perspective.
(Assignee)

Comment 3

a year ago
(In reply to Dustin J. Mitchell [:dustin] from comment #2)
> Testing both with and without e10s has, roughly, doubled our testing load. 
> Testing is expensive, so if we are considering doing the same thing with
> stylo, we'll need to consider that from a budget perspective.

My understanding is this is being discussed at the management layer. As far as we're concerned in this bug, we just need to support running a +stylo variation on only the stylo repository. We'll cross the major capacity/cost implications bridge when stylo needs to escape from its repository.
Sounds good.  We've had a few other "OMG why are we spending 2x our daily budget?!" moments, so I just want to avoid surprise.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

a year ago
bholley, et al: what automation do you need on the Stylo repository? Put another way, is there anything running today that we can turn off without it impacting Stylo development?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 18

a year ago
Try push w/ vendored Servo is up at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e01f952c84fd42b64e4fb585dfdcb3c090f9a1a0. Please let me know what you don't like about it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 26

a year ago
mozreview-review
Comment on attachment 8812023 [details]
Bug 1318200 - TaskCluster CI for Stylo;

https://reviewboard.mozilla.org/r/93928/#review94188
Attachment #8812023 - Flags: review?(dustin) → review+

Comment 27

a year ago
mozreview-review
Comment on attachment 8812024 [details]
Bug 1318200 - Obtain target tasks method from parameters;

https://reviewboard.mozilla.org/r/93930/#review94190
Attachment #8812024 - Flags: review?(dustin) → review+

Comment 28

a year ago
mozreview-review
Comment on attachment 8812025 [details]
Bug 1318200 - Introduce task graph filtering;

https://reviewboard.mozilla.org/r/93932/#review94192

r+ with minor language issues

::: taskcluster/docs/parameters.rst:94
(Diff revision 2)
>  specified programmatically using one of a variety of methods (e.g., parsing try
>  syntax or reading a project-specific configuration file).
>  
> +``filters``
> +    List of filter functions (from ``taskcluster/taskgraph/filter_tasks.py`` to
> +    apply). This is usually defined internally, as filters are typically

misplaced `)`

::: taskcluster/docs/taskgraph.rst:95
(Diff revision 2)
>  
>  #. For all kinds, generate all tasks.  The result is the "full task set"
>  #. Create dependency links between tasks using kind-specific mechanisms.  The
>     result is the "full task graph".
> -#. Select the target tasks (based on try syntax or a tree-specific
> -   specification).  The result is the "target task set".
> +#. Filter the target tasks (based on a series of filters, such as try syntax,
> +   tree-specific specifications, etc). The result is the *target task set*.

*..* isn't a bad idea here, but please change the other list items to match

::: taskcluster/taskgraph/generator.py:201
(Diff revision 2)
> +            old_len = len(target_task_set.graph.nodes)
> +            target_tasks = set(fltr(target_task_set, self.parameters))
> -        target_task_set = TaskGraph(
> +            target_task_set = TaskGraph(
> -            {l: all_tasks[l] for l in target_tasks},
> +                {l: all_tasks[l] for l in target_tasks},
> -            Graph(target_tasks, set()))
> +                Graph(target_tasks, set()))
> +            logger.info('Filter %s pruned %d nodes (%d remain)' % (

I would prefer that this used "tasks" and "dependencies" -- I think those will be clearer to those reading the logs without much knowledge of the internals.
Attachment #8812025 - Flags: review?(dustin) → review+

Comment 29

a year ago
mozreview-review
Comment on attachment 8812029 [details]
Bug 1318200 - Pass topsrcdir into decision parameters;

https://reviewboard.mozilla.org/r/93944/#review94194

I think that assumption was based on your assurances -- it's likely made in a few other places, too..
Attachment #8812029 - Flags: review?(dustin) → review+

Comment 30

a year ago
mozreview-review
Comment on attachment 8812026 [details]
Bug 1318200 - Populate parameter indicating whether Servo is present;

https://reviewboard.mozilla.org/r/93934/#review94198

Most of the examinations of the "current" tree occur at the time they are needed -- for example, the hashes of the docker contexts.  There are issues with that pattern (the current tree may not be that specified by `parameters['head_rev']`), but I'd rather be consistent about it.  Whether the servo directory exists is easy enough to check when needed, and doesn't really constitute a parameter.
Attachment #8812026 - Flags: review?(dustin) → review-

Comment 31

a year ago
mozreview-review
Comment on attachment 8812027 [details]
Bug 1318200 - Filter Stylo platforms when Servo isn't available;

https://reviewboard.mozilla.org/r/93936/#review94202

::: taskcluster/taskgraph/filter_tasks.py:38
(Diff revision 4)
> +
> +
> +@filter_task('check_servo')
> +def filter_servo(graph, parameters):
> +    """Filters out tasks requiring Servo if Servo isn't present."""
> +    if parameters.get('have_servo', False):

This is the place to call `os.path.isdir('servo')`.

::: taskcluster/taskgraph/filter_tasks.py:43
(Diff revision 4)
> +    if parameters.get('have_servo', False):
> +        return graph.tasks.keys()
> +
> +    def fltr(task):
> +        # All Stylo platforms require Servo.
> +        if 'stylo' in task.attributes.get('build_platform', ''):

Maybe it's years of seeing it abused, but the idea of checking substrings of identifiers makes me cringe.  It's susceptible to false positives and surprises, and also hard to grep for.

Can you change this to a whitelist of platforms?
Attachment #8812027 - Flags: review?(dustin) → review-

Comment 32

a year ago
mozreview-review
Comment on attachment 8812022 [details]
Bug 1318200 - Mozharness configs for building with stylo;

https://reviewboard.mozilla.org/r/93926/#review94228

::: testing/mozharness/configs/builds/releng_sub_linux_configs/64_stylo.py:15
(Diff revision 2)
> +        'setup-mock',
> +        'build',
> +        'upload-files',
> +        'sendchange',
> +        'check-test',
> +        # 'generate-build-stats',

Did you want to make this work while you're here?
Attachment #8812022 - Flags: review?(ted) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8812026 - Attachment is obsolete: true
(Assignee)

Comment 36

a year ago
mozreview-review-reply
Comment on attachment 8812022 [details]
Bug 1318200 - Mozharness configs for building with stylo;

https://reviewboard.mozilla.org/r/93926/#review94228

> Did you want to make this work while you're here?

I'll handle this in another bug. Possibly today.
(Assignee)

Updated

a year ago
Assignee: nobody → gps
Status: NEW → ASSIGNED
(In reply to Gregory Szorc [:gps] from comment #17)
> bholley, et al: what automation do you need on the Stylo repository? Put
> another way, is there anything running today that we can turn off without it
> impacting Stylo development?

We want reftests and crashtests in the stylo configuration (a special flag to the reftest runner which compares stylo rendering against non-stylo). We will want a subset of mochitests too in the not-too-distant future, but we're not there yet.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 38

a year ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #37)
> We want reftests and crashtests in the stylo configuration (a special flag
> to the reftest runner which compares stylo rendering against non-stylo). We
> will want a subset of mochitests too in the not-too-distant future, but
> we're not there yet.

Cool. Pretty sure that's what you had in the stylo repository and what cargo culted in this series. Worst case we need to make some adjustments after landing. Scheduling for TaskCluster is all in tree, so it shouldn't be difficult.

Word of warning: my commits will incur a few merge conflicts with the stylo repo. If you ping me when you want to merge mozilla-central into stylo, I can help resolve those. (Hint: basically you can drop all the automation customizations from stylo and take what's in m-c.)

Comment 39

a year ago
mozreview-review
Comment on attachment 8812029 [details]
Bug 1318200 - Pass topsrcdir into decision parameters;

https://reviewboard.mozilla.org/r/93944/#review94300

So much churn over topsrcdir, sorry..

::: taskcluster/docs/parameters.rst:72
(Diff revision 4)
> +``topsrcdir``
> +   Path to source checkout. (Should be set automatically.)

This will cause issues when users download `parameters.yml` from a decision task (where it ran in `/home/worker/checkouts/gecko` or whatever) and try to run it at home (in another directory).

I think it will be easiest to assume cwd = topsrcdir.  It wouldn't be the first, or last, place that's assumed.  For example, `docker_image.py` has

    GECKO = os.path.realpath(os.path.join(__file__, '..', '..', '..', '..'))

Fixing that is probably another bug.  Maybe it makes sense to stick it in a constant in `taskgraph.util`?
Attachment #8812029 - Flags: review+ → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8812029 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 42

a year ago
mozreview-review
Comment on attachment 8812027 [details]
Bug 1318200 - Filter Stylo platforms when Servo isn't available;

https://reviewboard.mozilla.org/r/93936/#review94302
Attachment #8812027 - Flags: review?(dustin) → review+

Comment 43

a year ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76318becc109
Mozharness configs for building with stylo; r=ted
https://hg.mozilla.org/integration/autoland/rev/734ec3d6b7d0
TaskCluster CI for Stylo; r=dustin
https://hg.mozilla.org/integration/autoland/rev/1fbab277e984
Obtain target tasks method from parameters; r=dustin
https://hg.mozilla.org/integration/autoland/rev/a34c214911ad
Introduce task graph filtering; r=dustin
https://hg.mozilla.org/integration/autoland/rev/3077666395e0
Filter Stylo platforms when Servo isn't available; r=dustin

Updated

a year ago
Blocks: 1330666
Depends on: 1348754
You need to log in before you can comment on or make changes to this bug.