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

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.

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.

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.)

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.

(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.

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

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 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.

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 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.

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

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.

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

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 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.

(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.

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

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 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.

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.

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 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 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?

(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!

> 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

> 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 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 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 on attachment 765351 [details] [diff] [review] added new entries in whitelist Review of attachment 765351 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

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.

I've removed the android related files.

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.

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.

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.

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

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 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.

Also, apologies for the long delay there.

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 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.

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.

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?

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

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

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

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).

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 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.

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

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 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 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