763903 - regularly run mozconfig comparisons

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[[e-mail] (:bhearsum)]

We run a mozconfig comparison as part of release sanity that compares the nightly mozconfig vs. the release one, factoring in some whitelisted items. This has been great for making sure we don't miss any changes, but it's problematic that it only runs *just* before you start a release. We should run this regularly so that we catch & fix these in advance of starting releases. Possible options:
- as part of 'make check', if we move the tool/whitelists into the source repo
- as part of preproduction

Comment 1

[[e-mail] (:bhearsum)]

Ted, Catlee, and I all agreed that running the comparisons as part of 'make check' would be good. This makes this primarily build config issue. Roughly, we need to:
- Create whitelist (based on https://github.com/mozilla/build-tools/blob/master/buildbot-helpers/mozconfig_whitelist, but not encompassing all repos)
- Move the mozconfig comparison logic (https://github.com/mozilla/build-tools/blob/master/buildbot-helpers/release_sanity.py#L117) to its own script
- Add build system glue to make it happen as part of 'make check'.

We'll also need to backport to mozilla-aurora, beta, and esr. Mostly that should be simple whitelist tweaks.

Comment 3

[[e-mail] (:bhearsum)]

I wrote this thing awhile ago, which would make this easier: https://github.com/mozilla/build-tools/blob/master/scripts/release/compare-mozconfigs.py

Might even make it possible to do this without moving the whitelist in tree.

Comment 4

[[e-mail] (:bhearsum)]

Here's a more precise plan of how to do this (focuses on Firefox, needs to be applied to Fennec & Thunderbird too, though):
* Create "nightly-whitelist" and "release-whitelist" files in "linux32", "linux64", "macosx-universal", and "win32" directories from https://github.com/mozilla/mozilla-central/tree/master/browser/config/mozconfigs
* The "nightly-whitelist" file will contain the lines that are OK to be in the "nightly" mozconfig but not beta/release. Eg, the contents of the existing whitelist["nightly"][$platform].
* The "release-whitelist" will contain the opposite (ok in release + not in nightly).
* Create a standalone version of https://github.com/mozilla/build-tools/blob/master/scripts/release/compare-mozconfigs.py. This can be done using our "package-script.py" tool. Sample usage of that is here: https://github.com/mozilla/build-tools/blob/master/buildfarm/utils/Makefile#L4

Ted, does that seem OK to you? Could you provide an example of how to integrate something into "make check"? (I know you're not the build system own anymore, so feel free to pass this along.)

Comment 5

[(not currently active) Ted Mielczarek]

This sounds pretty sensible. I like the idea of turning things orange if people update mozconfigs but not whitelists so we catch this right away.

It's as simple as putting a check:: target in a Makefile somewhere. If you're going to do this per-product you might want to put it in the app directory for each product, like browser/Makefile.in.

Comment 6

[[e-mail] (:bhearsum)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> This sounds pretty sensible. I like the idea of turning things orange if
> people update mozconfigs but not whitelists so we catch this right away.
> 
> It's as simple as putting a check:: target in a Makefile somewhere. If
> you're going to do this per-product you might want to put it in the app
> directory for each product, like browser/Makefile.in.

Thanks for the feedback. Sounds like we should put the script that does the comparison in some central location (https://github.com/mozilla/mozilla-central/tree/master/build, I'm guessing), and then simply call out to it in those targets. Most of the code for this is already written, so this bug should mostly be reorganizing the existing code + whitelists to work in tree.

Comment 7

[Armen [:armenzg]]

Loaned bld-centos6-hp-001 for this work.

Comment 8

[Jason Yeo[:jyeo]]

Attachment: moved script and whitelist into build directory, added check target in browser/Makefile.in

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 762651 [details] [diff] [review]
moved script and whitelist into build directory, added check target in browser/Makefile.in

Review of attachment 762651 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start, a few comments:

::: browser/Makefile.in
@@ +22,5 @@
>  endif
>  endif
> +
> +check::
> +	$(PYTHON) $(topsrcdir)/build/compare-mozconfigs.py --branch releases/mozilla-aurora --revision default --hghost hg.mozilla.org --product browser --whitelist $(topsrcdir)/build/mozconfig_whitelist linux,browser/config/mozconfigs/linux32/beta,browser/config/mozconfigs/linux32/nightly

Linux shouldn't be hardcoded here. We should be comparing against whatever the target platform is. You'll need to do some if/elif comparisons on OS_ARCH + TARGET_CPU to figure that out. Here's some examples:
* Windows (32 and 64-bit): https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#22
* Mac: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#29
* Linux: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#40

::: build/compare-mozconfigs.py
@@ +1,1 @@
> +#!/usr/bin/python

This is OK, but it would be better to have Makefile in tools/scripts/release to make this easier to rebuild. Here's a similar script that we use to build standalone versions of gittool and hgtool: https://github.com/mozilla/build-tools/blob/master/buildfarm/utils/Makefile.

::: build/mozconfig_whitelist
@@ +1,1 @@
> +# 'nightly' contains things that are in nightly mozconfigs and allowed to be missing from release builds.

Because the whitelist is now in the tree, we should drop the branch list. So, the data structure would then be:
whitelist['nightly'][platform]
whitelist['release'][platform]

I would suggest including any entries that are currently in mozilla-beta or mozilla-release platforms in the "release" section.

@@ +9,5 @@
> +    'comm-beta': {},
> +    'nightly': {},
> +    }
> +
> +all_platforms = ['win32', 'linux', 'linux64', 'macosx64', 'android', 'android-armv6', 'android-x86']

It'd probably be good to rename linux and macosx64 to "linux32" and "macosx-universal", respectively, to match the directories in mozconfigs/. That should make the command line construction a bit easier.

Comment 10

[Jason Yeo[:jyeo]]

Attachment: makefile to generate compare-mozconfig-nodeps

Comment 11

[[e-mail] (:bhearsum)]

Comment on attachment 762748 [details] [diff] [review]
makefile to generate compare-mozconfig-nodeps

Review of attachment 762748 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, and worked fine for me locally.

Comment 12

[Jason Yeo[:jyeo]]

Attachment: Determine platform from makefile variables

I have moved in the nodeps version of compare-mozconfig.py. But I'm not too sure if I am doing it right for browser/Makefile.in.

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 762809 [details] [diff] [review]
Determine platform from makefile variables

Review of attachment 762809 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/Makefile.in
@@ +25,5 @@
> +ifndef MOZ_DEBUG
> +
> +# determine platform to get their mozconfigs from
> +# browser/config/mozconfigs
> +MOZCONFIG_PLATFORM := $(TARGET_OS)$(TARGET_CPU)

It looks like this gets overridden in all cases. If that's true, I think this should just be removed...

@@ +42,5 @@
> +ifeq ($(TARGET_OS),linux-gnu)
> +MOZCONFIG_PLATFORM := linux$(TARGET_CPU)
> +endif
> +check::
> +	$(PYTHON) $(topsrcdir)/build/compare-mozconfigs.py --branch releases/mozilla-aurora --revision default --hghost hg.mozilla.org --product browser --whitelist $(topsrcdir)/build/mozconfig_whitelist $(MOZCONFIG_PLATFORM),browser/config/mozconfigs/$(MOZCONFIG_PLATFORM)/beta,browser/config/mozconfigs/$(MOZCONFIG_PLATFORM)/nightly

So, I just realized that we need to modify compare-mozconfigs tool quite a bit to work...we don't want it to be downloading anything, it should be using the files that already exist on disk. 

I think the best thing to do is to move the downloading outside of verify_mozconfigs and into this script and release_sanity.py (https://github.com/mozilla/build-tools/blob/master/buildbot-helpers/release_sanity.py). Then there could be a --no-download option that we could make use of in this script. If --no-download isn't specified, --branch, --revision, etc. would be required arguments, and the script will download the specified mozconfigs prior to calling verify_mozconfigs. In both cases, it will pass paths to already existing mozconfigs to verify_mozconfigs, instead of hghost/branch/etc.

Apologies for not catching this prior to you starting this work!

::: build/mozconfig_whitelist
@@ +1,1 @@
> +# 'nightly' contains things that are in nightly mozconfigs and allowed to be missing from release builds.

One thing I missed in the initial feedback request...this actually needs to get split out into per-product files. The Firefox parts should be in browser/config/mozconfigs/whitelist and the Fennec parts in mobile/android/config/mozconfigs/whitelist. (We'll also want to put the Thunderbird parts in comm-central, but let's do that as a second pass, once Firefox/Fennec are working fine.).

This also means that mobile/android/Makefile.in will need a check target added, too.

@@ +1,4 @@
> +# 'nightly' contains things that are in nightly mozconfigs and allowed to be missing from release builds.
> +# Other keys in whitelist contain things are in that branches mozconfigs and allowed to be missing from nightly builds.
> +whitelist = {
> +    'mozilla-release': {},

Use "release" here instead of mozilla-release. This isn't actually a branch name, and we don't want to confuse it with one.

Comment 14

[Jason Yeo[:jyeo]]

Attachment: added no download option in verify mozconfig

I thought this is the easiest way to add the --no-download option without changing things too much. What do you think?

Comment 15

[[e-mail] (:bhearsum)]

Comment on attachment 763176 [details] [diff] [review]
added no download option in verify mozconfig

Review of attachment 763176 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me.

Comment 16

[Jason Yeo[:jyeo]]

Attachment: added no download option in verify mozconfig, fixed typo

I made a typo with sys.exit. Here's the fix.

Comment 17

[[e-mail] (:bhearsum)]

Comment on attachment 763590 [details] [diff] [review]
added no download option in verify mozconfig, fixed typo

Just tried this locally, and it actually fails on:
    branch_name = get_repo_name(branch)

...when in --no-download mode.

And I also realized that there's more adjustment in general regarding the whitelist format - sorry for not catching this earlier.

Now that we're putting mozconfig whitelists in the tree, we should get rid of the whitelists in this repository, and grab them from there. So, that means that:
* https://github.com/mozilla/build-tools/blob/master/buildbot-helpers/mozconfig_whitelist should be deleted
* release_sanity.py needs to have its default for whitelist removed, because it's now invalid (https://github.com/mozilla/build-tools/blob/master/buildbot-helpers/release_sanity.py#L264).
* verify_mozconfigs should be changed to look for 'release' rather than branch_name in the mozconfig whitelist. It also needs to download the whitelist when not in no_download mode.

When that's done, can you try testing in both --no-download and download mode?

Comment 18

[Jason Yeo[:jyeo]]

Attachment: Removed mozconfig_whitelist from repo, removed reading of whitelist from verify_mozconfig function

Comment 19

[Jason Yeo[:jyeo]]

Attachment: added makefile in mobile/android.

Comment 20

[[e-mail] (:bhearsum)]

Comment on attachment 763821 [details] [diff] [review]
Removed mozconfig_whitelist from repo, removed reading of whitelist from verify_mozconfig function

Review of attachment 763821 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the note below this looks OK. You mentioned on IRC that you were going to make some changes, so I'll do a few tests of my own after that. Can you also attach the latest version of your mozilla-central patch, if its changed? It'd be good to have a look at them both at the same time.

::: buildbot-helpers/release_sanity.py
@@ +266,5 @@
> +        return dict()
> +
> +    f = NamedTemporaryFile()
> +    f.write(whitelist_data)
> +    f.flush()

You can cut out all this writing/rereading by using exec() to parse whitelist_data. It works the same as execfile does here: https://github.com/mozilla/build-tools/blob/master/lib/python/release/info.py#L90.

Comment 21

[Jason Yeo[:jyeo]]

(In reply to Ben Hearsum [:bhearsum] from comment #20)
> Can you also attach the latest version of your mozilla-central patch, if its
> changed?

I have already attached it. See https://bugzilla.mozilla.org/attachment.cgi?id=763853&action=edit.

Comment 22

[Jason Yeo[:jyeo]]

Attachment: Removed mozconfig_whitelist from repo, removed reading of whitelist from verify_mozconfig function

Here's the latest version for the remove whitelist patch. I tested with download and --no-download. It should work now.

Comment 23

[[e-mail] (:bhearsum)]

Comment on attachment 763853 [details] [diff] [review]
added makefile in mobile/android.

Review of attachment 763853 [details] [diff] [review]:
-----------------------------------------------------------------

The issue noted below doesn't prevent this patch from functioning, so I pushed it to get a better test run on our machines: https://tbpl.mozilla.org/?tree=Try&rev=492c9c33b7f7

Ted, are the right person to give feedback on this patch, or should I send it over to gps or someone else?

::: browser/Makefile.in
@@ +39,5 @@
> +MOZCONFIG_PLATFORM := linux$(TARGET_CPU)
> +endif
> +
> +check::
> +	$(PYTHON) $(topsrcdir)/build/compare-mozconfigs.py --branch release --product browser --whitelist $(srcdir)/config/mozconfigs/whitelist --no-download $(MOZCONFIG_PLATFORM),$(srcdir)/config/mozconfigs/$(MOZCONFIG_PLATFORM)/beta,$(srcdir)/config/mozconfigs/$(MOZCONFIG_PLATFORM)/nightly

You shouldn't need to pass --branch or --product here. Branch is whatever repository is being built (which is why you removed the "branch_name" references in verify_mozconfigs), and product is known because of the directory this is in. Same for the mobile/android/Makefile.in.

You also need a second invocation that compares release vs. nightly.

Comment 24

[[e-mail] (:bhearsum)]

Comment on attachment 764111 [details] [diff] [review]
Removed mozconfig_whitelist from repo, removed reading of whitelist from verify_mozconfig function

Review of attachment 764111 [details] [diff] [review]:
-----------------------------------------------------------------

I just did a test run locally. release sanity and compare-mozconfigs don't have any issues themselves, but it looks like the whitelists aren't taken into account properly. Eg:
python ../build/compare-mozconfigs.py --whitelist config/mozconfigs/whitelist --no-download linux32,config/mozconfigs/linux32/nightly,config/mozconfigs/linux32/beta
INFO:release.sanity:Comparing None mozconfigs to nightly mozconfigs...
ERROR:release.sanity:found in config/mozconfigs/linux32/nightly but not in config/mozconfigs/linux32/beta: ac_add_options --enable-codesighs
ERROR:release.sanity:found in config/mozconfigs/linux32/nightly but not in config/mozconfigs/linux32/beta: ac_add_options --enable-signmar
ERROR:release.sanity:found in config/mozconfigs/linux32/nightly but not in config/mozconfigs/linux32/beta: ac_add_options --enable-profiling
ERROR:release.sanity:found in config/mozconfigs/linux32/beta but not in config/mozconfigs/linux32/nightly: ac_add_options --enable-official-branding
ERROR:release.sanity:found in config/mozconfigs/linux32/beta but not in config/mozconfigs/linux32/nightly: mk_add_options MOZ_PGO=1
ERROR:release.sanity:found in config/mozconfigs/linux32/nightly but not in config/mozconfigs/linux32/beta: ac_add_options --enable-js-diagnostics
ERROR:release.sanity:found in config/mozconfigs/linux32/nightly but not in config/mozconfigs/linux32/beta: STRIP_FLAGS="--strip-debug"
ERROR:release.sanity:found in config/mozconfigs/linux32/nightly but not in config/mozconfigs/linux32/beta: ac_add_options --with-ccache=/usr/bin/ccache

It complains about MOZ_PGO=1, yet that's in the whitelist for linux32. There will be a couple of things (eg --enable-profiling) that still have errors, but most things should already be in the whitelist.

There's also a "None" in the output because --branch isn't set. Probably best to change that log message to not include branch, because it's not a required parameter anymore.

::: buildbot-helpers/release_sanity.py
@@ +270,5 @@
> +        exec(whitelist_script, script_globals)
> +    except SyntaxError:
> +        log.error("Error reading config from %s" % url)
> +        error_tally.add('download_read_whitelist')
> +        return {}

You can move the exec() to the first try block and add a second except block, or just catch both and log a more generic message.

Comment 25

[Jason Yeo[:jyeo]]

Attachment: cleaned up verify_mozconfig

I have cleaned up the verify_mozconfig function. It is no longer reading or downloading mozconfigs. It should look cleaner now. Also, I have fixed the bug that you have mentioned. It is reading from the whitelist and comparing against it now.

Comment 26

[Jason Yeo[:jyeo]]

Attachment: checks linux cpu type, added double semicolon check target

I fixed some issues with it after looking the logs from the try repo. But there might be some issues with win64 because we do not have an entry for that in the whitelist. See https://github.com/mozilla/build-tools/blob/a1ceeb70b7605dc396757001a12bf5abf3ddc514/buildbot-helpers/mozconfig_whitelist#L14.

Comment 27

[[e-mail] (:bhearsum)]

Comment on attachment 764369 [details] [diff] [review]
cleaned up verify_mozconfig

Review of attachment 764369 [details] [diff] [review]:
-----------------------------------------------------------------

Everything here seems OK to me, just holding off on the r+ until the latest try push finishes: https://tbpl.mozilla.org/?tree=Try&rev=35cb1e9f8b1c

Comment 28

[[e-mail] (:bhearsum)]

Comment on attachment 764390 [details] [diff] [review]
checks linux cpu type, added double semicolon check target

Review of attachment 764390 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jason Yeo [:jyeo] from comment #26)
> Created attachment 764390 [details] [diff] [review]
> checks linux cpu type, added double semicolon check target
> 
> I fixed some issues with it after looking the logs from the try repo. But
> there might be some issues with win64 because we do not have an entry for
> that in the whitelist. See
> https://github.com/mozilla/build-tools/blob/
> a1ceeb70b7605dc396757001a12bf5abf3ddc514/buildbot-helpers/
> mozconfig_whitelist#L14.

Let's just not run the tests for win64 for now. Probably the best thing to do is to leave MOZCONFIG_PLATFORM unset by default, and only run compare-mozconfigs.py if it is set.

I also noticed that the tests don't appear to run at all for Mac Desktop builds. Eg https://tbpl.mozilla.org/php/getParsedLog.php?id=24308335&tree=Try&full=1:
make -C browser check
make[3]: Nothing to be done for `check'.

I'm not sure what's going on there, maybe something to do with universal builds...

The whitelists will need some updating to make the tests pass, too. Can you have a look at the results of the latest try push and update them appropriately?

Comment 29

[Jason Yeo[:jyeo]]

(In reply to Ben Hearsum [:bhearsum] from comment #28)
> I also noticed that the tests don't appear to run at all for Mac Desktop
> builds. Eg
> https://tbpl.mozilla.org/php/getParsedLog.php?id=24308335&tree=Try&full=1:
> make -C browser check
> make[3]: Nothing to be done for `check'.

It was ran, and the error was flagged. https://tbpl.mozilla.org/php/getParsedLog.php?id=24340753&tree=Ash&full=1#error0

INFO:release.sanity:Comparing browser mozconfigs to nightly mozconfigs...
ERROR:release.sanity:found in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/nightly but not in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/beta: ac_add_options --enable-codesighs
ERROR:release.sanity:found in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/nightly but not in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/beta: ac_add_options --disable-install-strip
ERROR:release.sanity:found in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/nightly but not in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/beta: ac_add_options --enable-signmar
ERROR:release.sanity:found in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/beta but not in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/nightly: ac_add_options --enable-official-branding
ERROR:release.sanity:found in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/nightly but not in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/beta: ac_add_options --enable-profiling
ERROR:release.sanity:found in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/nightly but not in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/beta: ac_add_options --enable-instruments
ERROR:release.sanity:found in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/nightly but not in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/beta: ac_add_options --enable-dtrace
ERROR:release.sanity:found in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/nightly but not in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/beta: ac_add_options --enable-js-diagnostics
ERROR:release.sanity:found in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/nightly but not in /builds/slave/try-osx64-00000000000000000000/build/browser/config/mozconfigs/macosx-universal/beta: ac_add_options --with-macbundlename-prefix=Firefox
ERROR:root:Mozconfig check failed!

Comment 30

[Jason Yeo[:jyeo]]

> It was ran, and the error was flagged.
> https://tbpl.mozilla.org/php/getParsedLog.
> php?id=24340753&tree=Ash&full=1#error0

Argh. Wrong log. Please look at this one > https://tbpl.mozilla.org/php/getParsedLog.php?id=24308335&tree=Try&full=1#error0

Comment 31

[Jason Yeo[:jyeo]]

Attachment: Compare mozconfig only if var is set

Comment 32

[[e-mail] (:bhearsum)]

> It was ran, and the error was flagged. https://tbpl.mozilla.org/php/getParsedLog.php?id=24340753&tree=Ash&full=1#error0

Huh. The output is in a strange place. In any case, that's great!

I just went over the output of the try builds. The only thing left to do is update the whitelists so the checks actually pass. All of the currently failing things are ignorable, so you should be able to just add them to the appropriate whitelist.

I also noticed that "make check" doesn't actually run for Android builds. That's not something you need to worry about, but I filed bug 885154 for it.

Comment 33

[[e-mail] (:bhearsum)]

Comment on attachment 764979 [details] [diff] [review]
Compare mozconfig only if var is set

r- only because of the whitelist updating that needs to happen. Also needs build system peer review before landing.

Comment 34

[[e-mail] (:bhearsum)]

Comment on attachment 764369 [details] [diff] [review]
cleaned up verify_mozconfig

This patch looks fine. We need to hold off on landing until mid next week though, to avoid interfering with any releases that are already in progress.

Comment 35

[Jason Yeo[:jyeo]]

Attachment: added new entries in whitelist

Comment 36

[[e-mail] (:bhearsum)]

Comment on attachment 765351 [details] [diff] [review]
added new entries in whitelist

Review of attachment 765351 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 37

[[e-mail] (:bhearsum)]

Oh, but we should remove the Android makefile changes given bug 885154. I'm morphing that bug to figure out how to run the tests on Android.

Comment 38

[Jason Yeo[:jyeo]]

Attachment: removed android makefile

I've removed the android related files.

Comment 39

[(not currently active) Ted Mielczarek]

Comment on attachment 765929 [details] [diff] [review]
removed android makefile

Review of attachment 765929 [details] [diff] [review]:
-----------------------------------------------------------------

Just some fiddly stuff.

::: browser/Makefile.in
@@ +23,5 @@
>  endif
> +
> +# determine platform to get their mozconfigs from
> +# browser/config/mozconfigs
> +ifeq ($(OS_ARCH),WINNT)

You want OS_TARGET, here and below.

@@ +33,5 @@
> +endif
> +ifeq ($(OS_ARCH),Darwin)
> +MOZCONFIG_PLATFORM := macosx-universal
> +endif
> +ifeq ($(TARGET_OS),linux-gnu)

ifeq ($(OS_TARGET),Linux)

@@ +39,5 @@
> +MOZCONFIG_PLATFORM := linux64
> +else
> +MOZCONFIG_PLATFORM := linux32
> +endif
> +endif

We could just push this into configure and AC_SUBST this value if you'd like. It's a lot easier to do things there than in Makefiles. (Plus it makes it easier for when we eventually have to rip this stuff out of a makefile.)

@@ +45,5 @@
> +check::
> +ifdef MOZCONFIG_PLATFORM
> +	$(PYTHON) $(topsrcdir)/build/compare-mozconfigs.py --whitelist $(srcdir)/config/mozconfigs/whitelist --no-download $(MOZCONFIG_PLATFORM),$(srcdir)/config/mozconfigs/$(MOZCONFIG_PLATFORM)/beta,$(srcdir)/config/mozconfigs/$(MOZCONFIG_PLATFORM)/nightly
> +	$(PYTHON) $(topsrcdir)/build/compare-mozconfigs.py --whitelist $(srcdir)/config/mozconfigs/whitelist --no-download $(MOZCONFIG_PLATFORM),$(srcdir)/config/mozconfigs/$(MOZCONFIG_PLATFORM)/release,$(srcdir)/config/mozconfigs/$(MOZCONFIG_PLATFORM)/nightly
> +endif

Not sure if make will complain in the "MOZCONFIG_PLATFORM is undefined" case. You should verify that.

::: browser/config/mozconfigs/whitelist
@@ +31,5 @@
> +    'mk_add_options PROFILE_GEN_SCRIPT=@TOPSRCDIR@/build/profile_pageloader.pl',
> +    'ac_add_options --with-ccache=/usr/bin/ccache',
> +    'export MOZILLA_OFFICIAL=1',
> +    'export MOZ_TELEMETRY_REPORTING=1',
> +    "mk_add_options PROFILE_GEN_SCRIPT='$(PYTHON) @MOZ_OBJDIR@/_profile/pgo/profileserver.py 10'",

This line disappeared from our mozconfigs, FYI. Is this just a historical list? I notice you've got the ancient profile_pageloader junk up above.

::: build/compare-mozconfigs.py
@@ +1,2 @@
> +#!/usr/bin/env python
> +### Compressed module sources ###

So uh, this is a neat trick, but it's pretty awful in this context. We have a virtualenv in the objdir, so just put your prerequisite modules there. build/ and config/ are both added to sys.path.

Also, this should probably have an MPL2 license header.

Also, if this script originated from a different location you should mention its canonical location in a comment somewhere.

Comment 40

[Jason Yeo[:jyeo]]

Do you think if it's possible to use mozinfo.json to do what I want in python instead of using configure.in? This means that the code to determine the os_target and the target_cpu will be in python instead of in configure.in or the makefile.

Comment 41

[(not currently active) Ted Mielczarek]

You can do that (although the preferred interface to use mozinfo.json is via the mozinfo module: http://mozbase.readthedocs.org/en/latest/mozinfo.html ), but you can also (since you're running in an objdir), just do

import buildconfig

if buildconfig.substs['OS_TARGET'] == ... and buildconfig.substs['TARGET_CPU'] = ...:

buildconfig.substs is the list of everything that gets AC_SUBSTed from configure, it's equivalent to the list of variables you can use in Makefiles.

Comment 42

[Jason Yeo[:jyeo]]

Attachment: Added removal of enable-profiling line

Comment 43

[Jason Yeo[:jyeo]]

I've updated a patch due to a recent addition of enable-profiling to the whitelist. (http://hg.mozilla.org/build/tools/rev/0cc9fb7c5ae5)\

Comment 44

[Jason Yeo[:jyeo]]

Attachment: Added dependencies to build dir

I did a few stuff here:
- I created a wrapper script that calls our compare_mozconfig script.
- The wrapper script doesn't work if the wrapper script is in the build directory. This is because of the precedence of imports. It tries to import the buildconfig.py that is in the build directory instead of the buildconfig.py in the virtualenv's python path. To solve this, I created a directory in the build directory to house our scripts.
- I moved the dependencies into the build directory.

Let me know if this is okay.

Comment 45

[(not currently active) Ted Mielczarek]

Comment on attachment 768516 [details] [diff] [review]
Added dependencies to build dir

Review of attachment 768516 [details] [diff] [review]:
-----------------------------------------------------------------

A few things that it would be nice to change, but nothing critical, just niceties.

::: build/compare-mozconfig/compare-mozconfigs-wrapper.py
@@ +1,1 @@
> +#!/usr/bin/python

This needs a license header.

@@ +21,5 @@
> +    based on the platform that the machine is building for"""
> +    platform = determine_platform()
> +
> +    if platform is not None:
> +        python_exe = substs['PYTHON']

You can just use sys.executable here. Alternately, if you can rename compare-mozconfigs.py to not have a dash in the filename, you could just import it (you'd have to factor out a bunch of the code into a main() method that takes the args you want to pass).

@@ +22,5 @@
> +    platform = determine_platform()
> +
> +    if platform is not None:
> +        python_exe = substs['PYTHON']
> +        topsrcdir = substs['top_srcdir']

I'd prefer if you used MozbuildObject.from_environment() and then used its topsrcdir property:
http://mxr.mozilla.org/mozilla-central/source/build/pgo/profileserver.py#27

It's slightly more code, but grovelling around in substs for this isn't great.

Comment 46

[(not currently active) Ted Mielczarek]

Also, apologies for the long delay there.

Comment 47

[Jason Yeo[:jyeo]]

Attachment: include license header

Comment 48

[[e-mail] (:bhearsum)]

Comment on attachment 775738 [details] [diff] [review]
include license header

Since this is just the license header, I think my r+ is sufficient here. I also noticed this change landed recently though: https://hg.mozilla.org/mozilla-central/rev/f138a2a6c329

So I'll add "ac_add_options --disable-elf-hack # --enable-elf-hack conflicts with --enable-profiling" to the linux nightly whitelists when I land this in a bit.

Comment 49

[[e-mail] (:bhearsum)]

Comment on attachment 775738 [details] [diff] [review]
include license header

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d8f26f00c82f

Forgot to update the author when committing this, whoops :(.

If this lands successfully on inbound/central, I'll backport it to aurora/beta, so that we'll be using it everywhere once ESR17 dies.

Comment 50

[[e-mail] (:bhearsum)]

I wrote a blog post on what developers need to do now that this is landed.

The build/tools patch here can't land until we backport to mozilla-beta.

Comment 51

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d8f26f00c82f

Comment 52

[[e-mail] (:bhearsum)]

Unfortunately, it doesn't look like this is working. I pushed a patch to try that should've failed, and the only output we got was:
make[2]: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/obj-firefox/browser/app'
/builds/slave/try-lx-00000000000000000000000/build/obj-firefox/_virtualenv/bin/python /builds/slave/try-lx-00000000000000000000000/build/build/compare-mozconfig/compare-mozconfigs-wrapper.py
make[1]: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/obj-firefox/browser'

My patch was https://hg.mozilla.org/try/rev/912b5ed7492d, which removed most of the linux whitelist entries.

Can you have a look, Jason?

Comment 53

[[e-mail] (:bhearsum)]

Jason and I just chatted in person and he realized that he hand-edited the diff to add in LICENSE headers, but forgot to update metadata. So we were missing the last 4 lines of the mozconfig compare wrapper...which includes the part that calls main() :). I've fixed that up in https://hg.mozilla.org/integration/mozilla-inbound/rev/71233da022ea, and properly attributed the patch this time.

I've also pushed a few things to try to verify the failure case + backports:
https://tbpl.mozilla.org/?tree=Try&rev=3331cae23940
https://tbpl.mozilla.org/?tree=Try&rev=bc1e64788b0d
https://tbpl.mozilla.org/?tree=Try&rev=0edb8d2ae6d4

Comment 54

[[e-mail] (:bhearsum)]

Oops, not supposed to close this until the changeset gets onto central. Sorry for the noise.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Backed out for checktest orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/552874c60c72

https://tbpl.mozilla.org/php/getParsedLog.php?id=25344278&tree=Mozilla-Inbound

Comment 56

[Jason Yeo[:jyeo]]

Attachment: Include additional modules, s/i386/i686/, be a little noisy

Comment 57

[Gregory Szorc [:gps]]

For future reference, I'd like to avoid having importable Python modules live under /build and necessitating /build be in sys.path. The reason is we can't globally include /build in sys.path from tools like mach because of naming conflicts of .py files, notably automationutils.py.

If you are creating an importable Python module, please ensure it lives in a package (preferably "moz"-prefixed) and the root directory to be added to sys.path doesn't contain any .py files so as to not create conflicts with the root namespace.

e.g. for this bug we should have put things somewhere in /python/moz* or /build/python (or similar child directory not containing .py files).

Comment 58

[Jason Yeo[:jyeo]]

Attachment: compare-mozconfig.diff

Comment 59

[Jason Yeo[:jyeo]]

My try commits worked (https://tbpl.mozilla.org/?tree=Try&rev=5e19f2a4c8bf) and the missing lines in the linux's whitelist caused a failure (https://tbpl.mozilla.org/?tree=Try&rev=734ff28e5bd2) just as expected.

Comment 60

[[e-mail] (:bhearsum)]

Comment on attachment 779858 [details] [diff] [review]
compare-mozconfig.diff

Review of attachment 779858 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think the check ran on Mac:
make -C {972ce4c6-7e08-4474-a285-3208198ce6fd} check
make[4]: Nothing to be done for `check'.
/builds/slave/try-osx64-00000000000000000000/build/obj-firefox/i386/_virtualenv/bin/python /builds/slave/try-osx64-00000000000000000000/build/build/compare-mozconfig/compare-mozconfigs-wrapper.py
make -C js/src check
make -C config check

It looks like you need i386 for Mac still, based on https://mxr.mozilla.org/mozilla-central/source/config/tests/unit-writemozinfo.py#44.

I thought you were going to throw the methods you needed into compare-mozconfigs.py too, rather than add them in separate files? Either way is fine with me, but I don't think you should include methods you're not using.

Comment 61

[Jason Yeo[:jyeo]]

Attachment: compare-mozconfig removed imported modules

Test success: https://tbpl.mozilla.org/?tree=Try&rev=a6b986fa3af2
Expected test failure: https://tbpl.mozilla.org/?tree=Try&rev=cba79c9df9af

Comment 62

[[e-mail] (:bhearsum)]

Comment on attachment 780424 [details] [diff] [review]
compare-mozconfig removed imported modules

Review of attachment 780424 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me now! And all the test output looks as I would expect.

Comment 63

[Gregory Szorc [:gps]]

Comment on attachment 780424 [details] [diff] [review]
compare-mozconfig removed imported modules

Review of attachment 780424 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: build/compare-mozconfig/compare-mozconfigs.py
@@ +73,5 @@
> +    success = True
> +
> +    diffInstance = difflib.Differ()
> +    diff_result = diffInstance.compare(mozconfig_lines, nightly_mozconfig_lines)
> +    diffList = list(diff_result)

Nit: camelCase vs under_score vs camelCase

under_score is preferred.

Comment 64

[[e-mail] (:bhearsum)]

Comment on attachment 780424 [details] [diff] [review]
compare-mozconfig removed imported modules

Relanded, after a successful try push: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7622a58c17a

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f7622a58c17a