If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clean up B2G desktop core configs in mozharness

RESOLVED FIXED

Status

Release Engineering
Mozharness
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mshal, Assigned: mshal)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
The B2G desktop configs in mozharness have a bit of cargo-culting. We should pull out any unnecessary/redundant config settings. Depends on 1146892, since that will probably help some of the non-default PATH settings, at least on Windows.

Comment 1

3 years ago
(In reply to Michael Shal [:mshal] from comment #0)
> The B2G desktop configs in mozharness have a bit of cargo-culting. We should
> pull out any unnecessary/redundant config settings. Depends on 1146892,
> since that will probably help some of the non-default PATH settings, at
> least on Windows.

+1 to this
(Assignee)

Comment 2

3 years ago
Looking at this further, I think the primary cargo-culting are the unused STAGE_USERNAME and STAGE_SSH_KEY variables. They can be removed from all the B2G desktop configs and fx desktop configs, as far as I can tell.

The rest of the config appears to be necessary, except for some of the variables set to default values. Do we want to remove those since they're the default, or leave them in to make them easier to change in the future? Here is the set of variables in the mac config that I think can be removed without affecting compilation (untested so far - just from looking at how they're used) -

-    # decides whether we want to use moz_sign_cmd in env
-    'enable_signing': False,
-    'purge_skip': ['info', 'rel-*:45d', 'tb-rel-*:45d'],
-    'purge_basedirs':  [],
-    'enable_count_ctors': False,
-    'enable_talos_sendchange': False,
-    'enable_unittest_sendchange': False,
-    'enable_max_vsize': False,

So do we want to leave eg: enable_unittest_sendchange in the config to make it obvious that it's available but disabled?
Flags: needinfo?(jlund)

Comment 3

3 years ago
(In reply to Michael Shal [:mshal] from comment #2)
> Looking at this further, I think the primary cargo-culting are the unused
> STAGE_USERNAME and STAGE_SSH_KEY variables. They can be removed from all the
> B2G desktop configs and fx desktop configs, as far as I can tell.

cool, let's remove those for sure.

> -    # decides whether we want to use moz_sign_cmd in env
> -    'enable_signing': False,
> -    'purge_skip': ['info', 'rel-*:45d', 'tb-rel-*:45d'],
> -    'purge_basedirs':  [],
> -    'enable_count_ctors': False,
> -    'enable_talos_sendchange': False,
> -    'enable_unittest_sendchange': False,
> -    'enable_max_vsize': False,
> 
> So do we want to leave eg: enable_unittest_sendchange in the config to make
> it obvious that it's available but disabled?

the way I have it right now is:

if the config item is the same across all platforms of fx desktop builds, I put it in the default config:
   - http://mxr.mozilla.org/build/source/mozharness/scripts/fx_desktop_build.py#41

if an item differs on a per platform basis, I put the item in each platform config just to be explicit, even if not having it at all in certain platforms yields the same default effect (e.g. enable_max_vsize):
   - http://mxr.mozilla.org/build/source/mozharness/configs/b2g/desktop_macosx64.py#47
   - http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildbase.py#1596

iow - the items that have values but yield no affect on the job (e.g. ones that are False) but still have at least one True in another config, we should leave them in. there are pros and cons with this approach but I think it is nice to be explicit and declare items within each config at their respective config levels (script, platform, variant, branch, and build pool).

Maybe it be easier to have a quick chat if that's not making sense.
Flags: needinfo?(jlund)
(Assignee)

Comment 4

3 years ago
I think this would bring the list of things to be removed down to:

'enable_signing': False,
'enable_count_ctors': False,
'enable_talos_sendchange': False, (though enable_unittest_sendchange differs among the b2g desktop platforms, so it might make sense to leave this in too)

Though note that B2GDesktopBuild inherits from FxDesktopBuild, so I don't know if there's a good way to overwrite the default config there. Any suggestions?
(Assignee)

Comment 5

3 years ago
Created attachment 8590461 [details] [diff] [review]
cleanup-configs
Attachment #8590461 - Flags: review?(jlund)

Comment 6

3 years ago
> Though note that B2GDesktopBuild inherits from FxDesktopBuild, so I don't
> know if there's a good way to overwrite the default config there. Any
> suggestions?

re-write the __init__ method?

Updated

3 years ago
Attachment #8590461 - Flags: review?(jlund) → review+
(Assignee)

Comment 7

3 years ago
Comment on attachment 8590461 [details] [diff] [review]
cleanup-configs

https://hg.mozilla.org/build/mozharness/rev/9d4b7c66dcbd
Attachment #8590461 - Flags: checked-in+
mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/production
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.