1419182 - compare-mozconfigs cleanup

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

I'm tired of finding dead code in the mozconfig whitelist. I'm going to clean this mess up once and for all.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Remove unused required keys feature;

Nobody passes in the "required" argument. Remove it and
functionality related to it.

Review commit: https://reviewboard.mozilla.org/r/201400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201400/

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Hard code whitelist key;

We only call this function once. Behavior doesn't need to
be parameterized.

Review commit: https://reviewboard.mozilla.org/r/201402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201402/

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Don't use mutable default argument value;

We always passed this argument in so this change doesn't fix
any bugs. I just didn't like the code smell.

Review commit: https://reviewboard.mozilla.org/r/201404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201404/

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Remove functionality for downloading mozconfigs;

Pretty sure this is a relic of running this check in automation
outside the context of a source check. compare-mozconfigs.py is
essentially a source lint. I don't see a need for this feature to
exist.

Review commit: https://reviewboard.mozilla.org/r/201406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201406/

Comment 5

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Clean up value checking;

Review commit: https://reviewboard.mozilla.org/r/201408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201408/

Comment 6

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Pass topsrcdir into compare-mozconfigs.py;

We currently have the logic for compare-mozconfigs spread across
2 Python files. I'd like to move the logic into 1 file so we
can do nicer things more easily. In preparation for this, change
compare-mozconfigs.py to receive an argument that is the path to
the topsrcdir. Also, switch to argparse because it is more modern.

Review commit: https://reviewboard.mozilla.org/r/201410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201410/

Comment 7

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Consolidate mozconfig comparison logic into compare-mozconfigs.py;

Hopefully this makes it more obvious how all of this is hooked
together.

Review commit: https://reviewboard.mozilla.org/r/201412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201412/

Comment 8

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Remove MOZ_MAKE_FLAGS references from mozconfigs whitelists;

We no longer define MOZ_MAKE_FLAGS in the in-repo mozconfigs. Nor do we
pass MOZ_MAKE_FLAGS from any automation configs as far as I can tell.

Review commit: https://reviewboard.mozilla.org/r/201414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201414/

Comment 9

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Remove last references to PROFILE_GEN_SCRIPT;

There are no other references to PROFILE_GEN_SCRIPT outside
of the mozconfig whitelist file. The deleted entries from the
whitelist file were meaningless.

Review commit: https://reviewboard.mozilla.org/r/201416/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201416/

Comment 10

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Remove last reference to CLIENT_PY_ARGS;

This variable isn't consumed anywhere. I have no clue where it
was consumed. It appears to be the product of an old era.

Review commit: https://reviewboard.mozilla.org/r/201418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201418/

Comment 11

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Remove unreferenced entries in nightly whitelist;

None of these are defined in any "nightly" mozconfigs.

Some of these lines may occur and be set by mozconfigs. But the
entire premise of compare-mozconfigs is flawed because it only compares
file content: not the end result of mozconfig evaluation. (mozconfigs
are shell scripts and a lot of logic occurs in other sourced shell
scripts.)

Review commit: https://reviewboard.mozilla.org/r/201420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201420/

Comment 12

[Gregory Szorc [:gps]]

Attachment: Bug 1419182 - Look for extra lines in nightly whitelist;

Before, the whitelist['nightly'] entries could contain lines that
didn't actually occur in the mozconfig. With this commit, we
require that all lines in the whitelist actually exist in the
mozconfig.

Review commit: https://reviewboard.mozilla.org/r/201422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201422/

Comment 13

[Michael Shal [:mshal]]

Comment on attachment 8930274 [details]
Bug 1419182 - Remove unused required keys feature;

https://reviewboard.mozilla.org/r/201400/#review207058

It looks like this readConfig() method is some copy-pasta from build/release/info.py, hence the unused cruft.

Comment 14

[Michael Shal [:mshal]]

Comment on attachment 8930275 [details]
Bug 1419182 - Hard code whitelist key;

https://reviewboard.mozilla.org/r/201402/#review207062

Comment 15

[Michael Shal [:mshal]]

Comment on attachment 8930276 [details]
Bug 1419182 - Don't use mutable default argument value;

https://reviewboard.mozilla.org/r/201404/#review207064

Comment 16

[Michael Shal [:mshal]]

Comment on attachment 8930277 [details]
Bug 1419182 - Remove functionality for downloading mozconfigs;

https://reviewboard.mozilla.org/r/201406/#review207066

Comment 17

[Michael Shal [:mshal]]

Comment on attachment 8930278 [details]
Bug 1419182 - Clean up value checking;

https://reviewboard.mozilla.org/r/201408/#review207068

Comment 18

[Michael Shal [:mshal]]

Comment on attachment 8930279 [details]
Bug 1419182 - Pass topsrcdir into compare-mozconfigs.py;

https://reviewboard.mozilla.org/r/201410/#review207070

::: commit-message-5cd60:7
(Diff revision 1)
> +
> +We currently have the logic for compare-mozconfigs spread across
> +2 Python files. I'd like to move the logic into 1 file so we
> +can do nicer things more easily. In preparation for this, change
> +compare-mozconfigs.py to receive an argument that is the path to
> +the topsrcdir. Also, switch to argparse because it is more modern.

What, splitting the argparse switch out into a separate commit would make the patchset too large? :)

Comment 19

[Michael Shal [:mshal]]

Comment on attachment 8930280 [details]
Bug 1419182 - Consolidate mozconfig comparison logic into compare-mozconfigs.py;

https://reviewboard.mozilla.org/r/201412/#review207072

::: build/compare-mozconfig/compare-mozconfigs-wrapper.py:23
(Diff revision 1)
> -        for platform in PLATFORMS:
> -            log.info('Comparing platform %s' % platform)
> -            python_exe = substs['PYTHON']
> -            topsrcdir = substs['top_srcdir']
> +        topsrcdir = substs['top_srcdir']
>  
> -            # construct paths and args for compare-mozconfig
> +        ret = subprocess.call([

Does the wrapper file actually serve any purpose now? It seems like the compare-mozconfigs.py script could just be a python test.

Comment 20

[Michael Shal [:mshal]]

Comment on attachment 8930281 [details]
Bug 1419182 - Remove MOZ_MAKE_FLAGS references from mozconfigs whitelists;

https://reviewboard.mozilla.org/r/201414/#review207074

Comment 21

[Michael Shal [:mshal]]

Comment on attachment 8930282 [details]
Bug 1419182 - Remove last references to PROFILE_GEN_SCRIPT;

https://reviewboard.mozilla.org/r/201416/#review207076

Comment 22

[Michael Shal [:mshal]]

Comment on attachment 8930283 [details]
Bug 1419182 - Remove last reference to CLIENT_PY_ARGS;

https://reviewboard.mozilla.org/r/201418/#review207078

Comment 23

[Michael Shal [:mshal]]

Comment on attachment 8930284 [details]
Bug 1419182 - Remove unreferenced entries in nightly whitelist;

https://reviewboard.mozilla.org/r/201420/#review207080

Comment 24

[Michael Shal [:mshal]]

Comment on attachment 8930285 [details]
Bug 1419182 - Look for extra lines in nightly whitelist;

https://reviewboard.mozilla.org/r/201422/#review207082

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 8930280 [details]
Bug 1419182 - Consolidate mozconfig comparison logic into compare-mozconfigs.py;

https://reviewboard.mozilla.org/r/201412/#review207072

> Does the wrapper file actually serve any purpose now? It seems like the compare-mozconfigs.py script could just be a python test.

You are correct: it can likely be eliminated. This was marginally beyond my scope bloat tolerance for this series. The next person touching this code can worry about it :)

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23c4884d5316
Remove unused required keys feature; r=mshal
https://hg.mozilla.org/integration/autoland/rev/124bb2bb9b39
Hard code whitelist key; r=mshal
https://hg.mozilla.org/integration/autoland/rev/01ee358a08bf
Don't use mutable default argument value; r=mshal
https://hg.mozilla.org/integration/autoland/rev/3fe392d9fd66
Remove functionality for downloading mozconfigs; r=mshal
https://hg.mozilla.org/integration/autoland/rev/a7a3f0cdc871
Clean up value checking; r=mshal
https://hg.mozilla.org/integration/autoland/rev/489860a7e03a
Pass topsrcdir into compare-mozconfigs.py; r=mshal
https://hg.mozilla.org/integration/autoland/rev/a1040ffa24e2
Consolidate mozconfig comparison logic into compare-mozconfigs.py; r=mshal
https://hg.mozilla.org/integration/autoland/rev/cd573160f87c
Remove MOZ_MAKE_FLAGS references from mozconfigs whitelists; r=mshal
https://hg.mozilla.org/integration/autoland/rev/16aadd824582
Remove last references to PROFILE_GEN_SCRIPT; r=mshal
https://hg.mozilla.org/integration/autoland/rev/728865ad47ef
Remove last reference to CLIENT_PY_ARGS; r=mshal
https://hg.mozilla.org/integration/autoland/rev/5db28a857fdc
Remove unreferenced entries in nightly whitelist; r=mshal
https://hg.mozilla.org/integration/autoland/rev/3c9fb7f0ff92
Look for extra lines in nightly whitelist; r=mshal

Comment 27

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/23c4884d5316
https://hg.mozilla.org/mozilla-central/rev/124bb2bb9b39
https://hg.mozilla.org/mozilla-central/rev/01ee358a08bf
https://hg.mozilla.org/mozilla-central/rev/3fe392d9fd66
https://hg.mozilla.org/mozilla-central/rev/a7a3f0cdc871
https://hg.mozilla.org/mozilla-central/rev/489860a7e03a
https://hg.mozilla.org/mozilla-central/rev/a1040ffa24e2
https://hg.mozilla.org/mozilla-central/rev/cd573160f87c
https://hg.mozilla.org/mozilla-central/rev/16aadd824582
https://hg.mozilla.org/mozilla-central/rev/728865ad47ef
https://hg.mozilla.org/mozilla-central/rev/5db28a857fdc
https://hg.mozilla.org/mozilla-central/rev/3c9fb7f0ff92