Closed
Bug 1146894
Opened 9 years ago
Closed 9 years ago
Clean up B2G desktop core configs in mozharness
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Release Engineering
Applications: MozharnessCore
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mshal, Assigned: mshal)
References
Details
Attachments
(1 file)
10.71 KB,
patch
|
jlund
:
review+
mshal
:
checked-in+
|
Details | Diff | Splinter Review |
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•9 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•9 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•9 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•9 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•9 years ago
|
||
Attachment #8590461 -
Flags: review?(jlund)
Comment 6•9 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•9 years ago
|
Attachment #8590461 -
Flags: review?(jlund) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8590461 [details] [diff] [review] cleanup-configs https://hg.mozilla.org/build/mozharness/rev/9d4b7c66dcbd
Attachment #8590461 -
Flags: checked-in+
Comment 8•9 years ago
|
||
mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/production
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•