taskcluster e10s jobs are not using --disable-e10s

NEW
Unassigned

Status

2 years ago
6 months ago

People

(Reporter: jmaher, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified

Firefox Tracking Flags

(e10s-)

Details

(Reporter)

Description

2 years ago
we should ensure that all non-e10s jobs in taskcluster are using --disable-e10s and remove --e10s from the .yml files for e10s related jobs.
Assignee: nobody → gbrown
desktop_unittest.py defines a --e10s option, with a default of False.

https://hg.mozilla.org/mozilla-central/annotate/d0be57e84807/testing/mozharness/scripts/desktop_unittest.py#l113

it also performs this handy trick:

            if suite_category in SUITE_DEFAULT_E10S and not c['e10s']:
                base_cmd.append('--disable-e10s')
            elif suite_category not in SUITE_DEFAULT_E10S and c['e10s']:
                base_cmd.append('--e10s')

https://hg.mozilla.org/mozilla-central/annotate/d0be57e84807/testing/mozharness/scripts/desktop_unittest.py#l332

where

            SUITE_DEFAULT_E10S = ['mochitest', 'reftest']

https://hg.mozilla.org/mozilla-central/annotate/d0be57e84807/testing/mozharness/scripts/desktop_unittest.py#l36
(Reporter)

Comment 2

2 years ago
got it- that would explain why buildbot jobs with --disable-e10s passed to mozharness are failing.
(Reporter)

Comment 3

2 years ago
I looked into adding a --disable-e10s flag to mozharness, that seems more wide sweeping than we would like.  

if we change buildbot, all branches need to support it- but we can leave buildbot alone and get mozharness working properly with --disable-e10s.

For the harnesses, we got rid of --e10s and added --disable-e10s; effectively --disable-e10s toggled the e10s option.  Given that we would need to support both, I imagine we would need to add --disable-e10s to desktop_unittest.py and store it in e10s, then change e10s to store a value in e10s_deprecated- then do a check to see which is set and get the e10s variable properly set.

possibly there are other ways to solve this- but without support for --disable-e10s at least on trunk we only make things more confusing.

Updated

2 years ago
Blocks: 984139
tracking-e10s: --- → +
(In reply to Joel Maher (:jmaher) from comment #3)

Thanks for looking at this Joel.

> Given that we would
> need to support both, I imagine we would need to add --disable-e10s to
> desktop_unittest.py and store it in e10s, then change e10s to store a value
> in e10s_deprecated- then do a check to see which is set and get the e10s
> variable properly set.

I think there would still be a problem for the case of a job that specified neither --e10s nor --disable-e10s; in the existing world, no option means disable-e10s, but in future we want no option to mean e10s.

My plan would be:
 - update desktop_unittest.py to accept both --disable-e10s and --e10s (but not at the same time); if neither is specified, default is still disable-e10s; if both are specified, fail
 - update buildbot and tc such that all jobs specify either --disable-e10s or --e10s
 - update desktop_unittest.py so that if neither is specified, default is e10s
 - update buildbot and tc to remove all --e10s flags
 - remove --e10s option from desktop_unittest.py

Each step would need to be deployed and verified before moving to the next step.

Is there an easier way?

Given this complexity, I have to wonder if it is worth the effort. Are you sure we want to do this?
tracking-e10s: + → ---
Flags: needinfo?(jmaher)
(Reporter)

Comment 5

2 years ago
yeah, that is a good point- this is a lot of work.  Assuming we didn't have buildbot in the mix (as in we wait until we are fully on taskcluster), do you think this would be a simpler path forward?
Flags: needinfo?(jmaher)
I'm not 100% sure, but I think it would be much simpler: one landing that changes desktop_unittest.py and all the yml files at the same time should be okay.
(Reporter)

Comment 7

2 years ago
the only motivation to do this earlier would be to fix existing .yml files so we are not copying a bunch of bad habits.  I am fine punting this or doing part of it now and finishing it later.
Let's put it aside for now, and re-visit in the taskcluster-homogeneous utopia of tomorrow.
Assignee: gbrown → nobody

Comment 9

2 years ago
Hey glob, can we get a tracking:+ setting here?
Flags: needinfo?(glob)
(In reply to Jim Mathies [:jimm] from comment #9)
> Hey glob, can we get a tracking:+ setting here?

hi jimn.

tracking-e10s enabled.

in future please file a bug in bugzilla.mozilla.org::administration (i've moved out of the bmo team, and a bug ensures a speedy response in cases where a single person is on pto).

thanks!
Flags: needinfo?(glob)

Comment 11

2 years ago
(In reply to Byron Jones ‹:glob› from comment #10)
> (In reply to Jim Mathies [:jimm] from comment #9)
> > Hey glob, can we get a tracking:+ setting here?
> 
> hi jimn.
> 
> tracking-e10s enabled.
> 
> in future please file a bug in bugzilla.mozilla.org::administration (i've
> moved out of the bmo team, and a bug ensures a speedy response in cases
> where a single person is on pto).
> 
> thanks!

will do, thanks!
tracking-e10s: --- → -
Is this ready to go now?  Still relevant?
Flags: needinfo?(jmaher)
(Reporter)

Comment 13

2 years ago
the work here would be fixing mozharness commands and related commands in buildbot/taskcluster to switch to --disable-e10s instead of passing in e10s.  This will be much easier when we are not running on buildbot, I think that will be the ideal time to revisit this.
Flags: needinfo?(jmaher)

Updated

6 months ago
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.