Closed Bug 1318200 Opened 8 years ago Closed 8 years ago

Run stylo automation when servo is available

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(5 files, 2 obsolete files)

Bug 1318200 - Mozharness configs for building with stylo;
(deleted), text/x-review-board-request
ted
: review+
Details
Bug 1318200 - TaskCluster CI for Stylo;
(deleted), text/x-review-board-request
dustin
: review+
Details
Bug 1318200 - Obtain target tasks method from parameters;
(deleted), text/x-review-board-request
dustin
: review+
Details
Bug 1318200 - Introduce task graph filtering;
(deleted), text/x-review-board-request
dustin
: review+
Details
Bug 1318200 - Filter Stylo platforms when Servo isn't available;
(deleted), text/x-review-board-request
dustin
: review+
Details
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.
(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.
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)
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 on attachment 8812023 [details] Bug 1318200 - TaskCluster CI for Stylo; https://reviewboard.mozilla.org/r/93928/#review94188
Attachment #8812023 - Flags: review?(dustin) → 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 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 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 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 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 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+
Attachment #8812026 - Attachment is obsolete: true
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: 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)
(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 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-
Attachment #8812029 - Attachment is obsolete: true
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+
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
Blocks: 1330666
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: