Open Bug 902825 Opened 11 years ago Updated 2 years ago

Don't rebuild the world after configure / mozilla-config.h dependencies

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect

Tracking

(Not tracked)

People

(Reporter: gps, Unassigned)

References

Details

Attachments

(11 files, 5 obsolete files)

confdefs.txt
30.58 KB, text/plain
Details
AC_DEFINE usage by file
291.19 KB, text/plain
Details
Bug 902825 - Rename non_global_defines to non_global_config;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 902825 - Rename ALLDEFINES to GLOBAL_DEFINES;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 902825 - Allow generating dist/include/mozconfig/*.h;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 902825 - Add non_global() to set non-global variables in moz.configure;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 902825 - Remove MOZ_CALLGRIND from mozilla-config.h;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 902825 - Remove explicit NECKO_WIFI define;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 902825 - Change ALLSUBSTS to GLOBAL_SUBSTS;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 902825 - Mark MOZ_CONFIGURE_OPTIONS as non-global;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 902825 - Output a set of all defines from moz.configure;
59 bytes, text/x-review-board-request
glandium
: review+
Details 
We all know that it sucks when mozilla-config.h changes and the world is rebuilt because everything depends on mozilla-config.h. But I don't believe we have a bug on making it suck less... until now.

I believe a few people have ideas on how to make this much better. I know one of them is mshal, so needinfo on him.

Please chime in with your ideas. Hopefully we can make this suck much less.
Flags: needinfo?(mshal)
Using less AC_DEFINEs is about the only thing we can do imho.
Here's a typical example of what shouldn't be AC_DEFINEd:
https://mxr.mozilla.org/mozilla-central/search?string=BUILD_CTYPES
I bet jcranmer - master of static analysis and DXR - could tell us which variables have AC_DEFINE but aren't actually used.
Flags: needinfo?(Pidgeot18)
Do we write down mozilla-config.h every time we run configure, even if nothing has changed?
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> Do we write down mozilla-config.h every time we run configure, even if
> nothing has changed?

No. All files going through substitution use Python to buffer the output and compare against the existing file to avoid the write/touch if nothing has changed.
Here is a random idea I had: instead of dumping all the AC_DEFINE's into a globally-included header, dump only the critical ones (like XP_UNIX) in mozilla-config.h, and move the ones which say "enable this component" (e.g., MOZ_ENABLE_DBUS) into a separate mozilla/Configuration.h file. That way, when someone adds a new configure option, only those files which are built off of ifdefs change.
(In reply to comment #6)
> Here is a random idea I had: instead of dumping all the AC_DEFINE's into a
> globally-included header, dump only the critical ones (like XP_UNIX) in
> mozilla-config.h, and move the ones which say "enable this component" (e.g.,
> MOZ_ENABLE_DBUS) into a separate mozilla/Configuration.h file. That way, when
> someone adds a new configure option, only those files which are built off of
> ifdefs change.

This is a fantastic idea!
(In reply to Joshua Cranmer [:jcranmer] from comment #6)
> Here is a random idea I had: instead of dumping all the AC_DEFINE's into a
> globally-included header, dump only the critical ones (like XP_UNIX) in
> mozilla-config.h, and move the ones which say "enable this component" (e.g.,
> MOZ_ENABLE_DBUS) into a separate mozilla/Configuration.h file. That way,
> when someone adds a new configure option, only those files which are built
> off of ifdefs change.

You'd have to include mozilla/Configuration.h manually. And it's likely to end up being (transitively) included everywhere anyways.
(In reply to Mike Hommey [:glandium] from comment #8)
> (In reply to Joshua Cranmer [:jcranmer] from comment #6)
> > Here is a random idea I had: instead of dumping all the AC_DEFINE's into a
> > globally-included header, dump only the critical ones (like XP_UNIX) in
> > mozilla-config.h, and move the ones which say "enable this component" (e.g.,
> > MOZ_ENABLE_DBUS) into a separate mozilla/Configuration.h file. That way,
> > when someone adds a new configure option, only those files which are built
> > off of ifdefs change.
> 
> You'd have to include mozilla/Configuration.h manually. And it's likely to
> end up being (transitively) included everywhere anyways.

Many of the configuration options shouldn't need to be used except locally to identify if something needs to be stubbed out or included in some module loader somewhere. I would have to look at all the AC_DEFINE's in more detail to figure out if this would be effective or not. The boundary line should just usably be everything that comes from an actual "configury check" (like HAVE_MEMMOVE) versus just a reported thing from some --enable-obscure-option where the main purpose of the AC_DEFINE is to avoid adding a ifdef OBSCURE_OPTION/DEFINES += foo/endif block.
We're not adding "configury checks" very often. In fact, looking at the recent changes to AC_DEFINEs, the last "configury checks" to have been touches were removed, not added.
Not only that, but we're usually not using their value anyways. So maybe we should just stop including mozilla-config.h everywhere and include it where it's actually necessary.
Ah, things like HAVE_VISIBILITY_HIDDEN_ATTRIBUTE, used in nscore.h, make my point moot :(
How often do we change non-configury checks that are AC_DEFINE'ed?
According to "hg grep --all AC_DEFINE configure.in build/autoconf | awk -F : '{print $2}' | uniq", 38 times in the last 10000 changesets before a11cd50edb0d. But that doesn't discriminate which is which.
(In reply to comment #13)
> According to "hg grep --all AC_DEFINE configure.in build/autoconf | awk -F :
> '{print $2}' | uniq", 38 times in the last 10000 changesets before
> a11cd50edb0d. But that doesn't discriminate which is which.

So, it's about one every 263 changesets, and given the fact that we have a few hundred changesets going in every day, if you pull about once a day, you're screwed :(
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> (In reply to comment #13)
> > According to "hg grep --all AC_DEFINE configure.in build/autoconf | awk -F :
> > '{print $2}' | uniq", 38 times in the last 10000 changesets before
> > a11cd50edb0d. But that doesn't discriminate which is which.
> 
> So, it's about one every 263 changesets, and given the fact that we have a
> few hundred changesets going in every day, if you pull about once a day,
> you're screwed :(

Those 10000 changesets cover 3 months.
Attached file confdefs.txt — 
Attached is a list of all of the confdefs provided by configure. The way I made this was that I first saved the results of searching for HAVE_*:
wget 'http://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%2F%5CbHAVE_%5BA-Z_%5D%2B%5Cb%2F&format=json&limit=100000' -O /tmp/HAVE.json [that's /\bHAVE_[A-Z_]+\b/ for those who can't speak URL-encoded ASCII]

Then I downloaded everybody mentioned in the configure script:
for f in $(grep -A1 'confdefs.pytmp' configure | grep "'''" | sed -e "s/^[^']*''' \([^']*\) '''.*$/\1/" | sed -e '/^[a-z_]*$/d' -e 's/\$.*$//' -e '/^HAVE_/d' | sort -u); do wget "http://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%2F%5cb$f%5Cb%2F&format=json&limit=10000" -O /tmp/$f.json; done

[This is missing a few things like GL_PROVIDER_GLX, since that's defined via a configure variable]. The python script to build the output is here:
masterDict = dict()
for jf in os.listdir('/tmp'):
    if not jf.endswith(".json"):
        continue
    with open(jf, 'r') as jfd:
        dxrres = simplejson.load(jfd)['results']

    for result in dxrres:
        fname = result['path']
        for line in result['lines']:
            text = line['line']
            varname = text[text.find('<b>') + 3:text.find('</b>', text.find('<b>'))]
            dname = masterDict.setdefault(varname, dict())
            dname[fname] = dname.get(fname, 0) + 1

print 'Total Headers C++ Other Name'
for var in masterDict:
    doth, dotc, other = 0, 0, 0
    for fname in masterDict[var]:
        if fname.count('.h') or fname.count('.H'):
            doth += 1
        elif fname.count('.c') or fname.count('.C') or fname.count('.m'):
            dotc += 1
        else:
            other += 1
    print '  %3d     %3d %3d   %3d %s' % (len(masterDict[var]), doth, dotc, other, var)
Flags: needinfo?(Pidgeot18)
This lists each file and the AC_DEFINEs used by that file. About 214 headers include a MOZ_* definition in them. Exclude MOZ_WIDGET, MOZ_XUL, MOZ_X11, and MOZ_ASAN, and you'll get down to about 150. (Note there's about 717 that include something from configure).
Comment on attachment 788798 [details]
confdefs.txt

Note that a lot of the HAVE_* ones come from autoconf itself.
The linux kernel has a post-processing script (linux/scripts/basic/fixdep.c) that removes the dependency on autoconf.h (their mega-#define header), and adds a bunch of fake dependencies on stub files that correspond to the actual defines used. There isn't really a good way to get the defines used from the preprocessor, so instead what it does is get the list of dependencies from the dep file, and essentially grep through them all for CONFIG_* strings. This way it can pick up CONFIG_* macros used in header files and such too.

For example, if a .c file has a line like:

#ifdef CONFIG_EXT3_FS_POSIX_ACL

And the dep file produced by gcc looks like:

...
include/generated/autoconf.h \
include/linux/limits.h \
include/linux/ioctl.h \
...

Then the script will adjust the dep file to look like:

...
$(wildcard include/config/ext3/fs/security.h) \
include/linux/limits.h \
include/linux/ioctl.h \
...

I believe the kconfig process then just touches the include/config/ext3/fs/security.h stub file if that option changes, so only those files that use the option are rebuilt.

I don't know if it's worth pursuing that for m-c or not - one potential difficulty in comparison is that linux has all of its symbols named CONFIG_*, which makes grepping easy. Some of ours start with MOZ_*, but not all of them do. Just having a list of symbols to grep for might not be sufficient, since we could compile a file with an #ifdef on a new symbol before adding it to configure, for example.
(In reply to Joshua Cranmer [:jcranmer] from comment #6)
> Here is a random idea I had: instead of dumping all the AC_DEFINE's into a
> globally-included header, dump only the critical ones (like XP_UNIX) in
> mozilla-config.h, and move the ones which say "enable this component" (e.g.,
> MOZ_ENABLE_DBUS) into a separate mozilla/Configuration.h file. That way,
> when someone adds a new configure option, only those files which are built
> off of ifdefs change.

I like this idea too - though could we put each option into its own file? eg: mozilla/config/dbus.h, or mozilla/config/valgrind.h, etc. Then if I flip back & forth between --enable-dbus, only mozilla/config/dbus.h gets a new timestamp.

We'd have to go through and #include "mozilla/config/dbus.h" in places that use MOZ_ENABLE_DBUS, but I don't think that's too onerous (and we can probably use a static checker to verify those are correct).
Flags: needinfo?(mshal)
Here's a WIP patch (it doesn't actually compile) for the approach in #c20. Do you all think it is worth pursuing? Basically what it does is:

 - Move the non-@ALLDEFINES@ bits from mozilla-config.h to mozilla-global.h
 - Only force-include mozilla-global.h
 - config/Makefile.in adds a step to split out mozilla-config.h into dist/include/mozconfig/*.h

Still to be done:
 1) Use an install manifest for dist/include/mozconfig to remove old headers
 2) Generate mozconfig/*.h for things that aren't defined (maybe it's better to use config.status as the input rather than mozilla-config.h?)
 3) Actually go through sources and add the correct #include "mozconfig/foo.h"

For example, for sources that use #ifdef NS_PRINTING, we would need to add #include "mozconfig/ns_printing.h" in the source file. Then if we change the value of NS_PRINTING in configure, we get a new mozilla-config.h, an updated mozconfig/ns_printing.h, and then only those files that actually use NS_PRINTING get recompiled.

I think 1) and 2) above should be fairly easy to do, but I have no idea how onerous 3) will be to initially port and then maintain afterward. Any thoughts there? Anything I'm missing?
Comment on attachment 823716 [details] [diff] [review]
0001-WIP-Generate-dist-include-mozconfig-.h.patch

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

The "moz config" file generation should be handled in config.status.

I worry about us catching #ifdefs that aren't triggered due to a missing #include. Before, you could use any symbol anywhere and it would practically guaranteed to work.

Perhaps we can enhance the Clang static analysis plugin to require any file containing |#ifdef MOZ_<FOO>| to also |#include "mozconfig/foo.h"|?
There was some discussion of this yesterday on irc... glandium suggested adding ifs and DEFINES in individual moz.build files to cut down on AC_DEFINES.

This may share a bit of the pitfall mentioned in comment 22, because people are used to having symbols available everywhere, but we might be able to find a way to flag if something is tested in a C++ file that looks like it should be mentioned in the build environment but isn't.
(In reply to :Ehsan Akhgari from comment #4)
> Do we write down mozilla-config.h every time we run configure, even if
> nothing has changed?

We do nowadays, since mozilla-config.h goes through the GENERATED_FILES machinery, and we changed GENERATED_FILES to always update mtime in bug 1218999.  So anytime config.status changes, for whatever reason (though most of the reasons are C++-related), mozilla-config.h changes (mtime-wise, if not contents-wise), and that rebuilds the world.  C++ files also depend on autoconf.mk, too, which is in a roughly similar boat.
Now that we have moz.configure, it should become much easier for us to do more advanced things with defines. I reckon mshal's experimentation could be done a lot less hackier now.
I've been playing around with this again - moz.configure definitely does make this much more straightforward. Of course we still have a fair bit of things in old-configure.in to handle, but I think it is manageable. I should have some patches up soon, unless I hit a major snag.
Assignee: nobody → mshal
Well, I hit a snag, and then got distracted by other work. So this almost works - I can label a set_define() in moz.configure with mozconfig_header=True, which will cause the define to be in MOZ_FOO.h, rather than in mozilla-config.h. Then every *.cpp / *.h that wants to use MOZ_FOO can #include "mozconfig/MOZ_FOO.h" and continue to use the ifdefs as normal, and if the value of MOZ_FOO changes, only MOZ_FOO.h is updated and mozilla-config.h remains unchanged.

The first snag I hit was that everything in the RecursiveMake backend also depends on autoconf.mk, which has ALLSUBSTS (and therefore also all defines). So autoconf.mk would change whenever MOZ_FOO changes, and rebuild everything. I was able to work around this by removing all mozconfig_headers from ALLSUBSTS, along with MOZ_CONFIGURE_OPTIONS. (The latter changes every time you add or remove a configure option, so for example adding --disable-foo will change MOZ_CONFIGURE_OPTIONS, which changes the contents of autoconf.mk, which rebuilds the world).

The problem I haven't been able to work around yet is that config.status always changes when a configure runs, which causes the file_generate actions to trigger. Even if we special-case out mozilla-config.h to avoid the timestamp update from bug 1218999, we still have lots of other generated headers, some of which are used extensively (eg: xpcom-config.h, js-config.h, etc). So we need to figure out a solution for config.status and its use as a dependency in the build in order for this to be beneficial. I have a rough idea that involves turning config.status's lists & dicts into a directory of files rather than a single file with all the data, so that for example processing xpcom-config.h would result in reading only the files for the 3 variables it actually references. That way xpcom-config.h would only be regenerated when xpcom-config.h.in or any of those 3 config.status.d/[varname] files change, rather than every time the main config.status script changes. However, I don't have time to explore this now, and maybe somebody else has a simpler solution.

I'll post the patches I have so far, in case someone else wants to pick this up.
This part is just some hacks to avoid the autoconf.mk dependency and ignoring bug 1218999 for mozilla-config.h, but unfortunately is not enough to actually make it work.
What converting a variable might look like.
The final hack that makes it "work" by disabling bug 1218999 for *.h / *.inc. So now those files are always rebuilt and discarded when configure changes, but that means we don't retrigger building *.cpp. Obviously we need a solution here that accommodates both, but with this patch you can see that toggling --enable-callgrind on and off in a .mozconfig results in a ~1m build instead of a full recompile. This is just for demonstration purposes, not a path towards a fix.
I'm not working on this at the moment, but may pick it up again sometime in the future. Feel free to ping me if you want to work on this and need some help.
Assignee: mshal → nobody
I'm going to take another look at this for Q2.
Assignee: nobody → mshal
It looks like I can work around the fact that lots of things are re-generated when config.status changes (#c27) if I do the following:

1) Replace uses of buildconfig.topsrcdir / .topobjdir with a separate file that contains only those two variables
2) Revert bug 1231315

The first part helps because many of the generated files, including most of those that use file_generate, only import buildconfig (and therefore config.status) because they use buildconfig.topsrcdir and/or .topobjdir. Instead of importing the whole of buildconfig, a separate module that contains just those two variables will change much less frequently.

However, there are still a few files that rely on the rest of the stuff in configure, and these are generally the CONFIGURE_DEFINE_FILES (mozilla-config.h, js-config.h, xpcom-config.h, etc). Originally these were generated as a part of configure, but they were moved to separate steps of the build in bug 1231315. Unfortunately that means they are re-generated everytime configure runs, and due to the mtime stamping from bug 1218999, the world gets rebuilt. By going back to generating them during configure, we can use the normal FileAvoidWrite semantics and only write out new versions of mozilla-config.h and such when the contents actually change. This seems to have the intended behavior - I can then flip --enable-callgrind on and off and rebuild only ~1m worth of stuff instead of the whole tree.

glandium, do you have any thoughts on how to proceed here? Is backing out bug 1231315 going to be viable at all if it means we can skip rebuilding world here? Or should I try to pursue splitting up config.status as I mentioned in #c27? Or maybe there's an easier solution that I'm not seeing?
Flags: needinfo?(mh+mozilla)
Attachment #8845483 - Attachment is obsolete: true
Attachment #8845484 - Attachment is obsolete: true
Attachment #8845486 - Attachment is obsolete: true
Attachment #8845489 - Attachment is obsolete: true
Here's another attempt at getting this to work. With the current patch series, I'm able to get a "no-op" incremental build after a configure down to rebuilding ~60 files instead of the ~3750 that we do today. The last patch also shows what marking a mozconfig_header in moz.configure will do -- when adding "ac_add_options --enable-callgrind" to the mozconfig, we currently rebuild the world since mozilla-config.h materially changes, but by moving that #define out of mozilla-config.h, we only rebuild ~60 files (only 2 files actually depend on MOZ_CALLGRIND). I picked that flag in particular since it was used by so few files, and can show the benefits. Not all feature flags will be useful to move out, since they may be needed by widely used headers. This is just supposed to provide the way forward so that we can move out particular flags that people might like to turn on/off in their mozconfig and are only used by a few files.

The main problem I had is in working around the interaction of bug 1231315 and bug 1218999 as it relates to changing the timestamps of CONFIGURE_DEFINE_FILES when config.status changes. I think the simplest way forward as shown by this patch series is to revert bug 1231315, which means CONFIGURE_DEFINE_FILES are generated during moz.build processing rather than as separate GENERATED_FILES build steps. I'm not sure if this breaks other things, though. :gps, do you recall why this was done? It seems more correct to do it as separate build steps, but unfortunately that conflicts with how make decides to rebuild things.

In any case, I'm going to wait for glandium to get back since I think he would be best to review this. But in the meantime, I would definitely appreciate any feedback on the following:

1) Will reverting bug 1231315 break something? (It's hard to tell from the bug and the bug it blocks)

2) Does this seem like a reasonable approach for moving things out of mozilla-config.h? We won't kill the header entirely, but I hope to get it down to things that "everything" needs and changes infrequently, so that after a re-configure we're building as little as possible.
Flags: needinfo?(ted)
Flags: needinfo?(gps)
Flags: needinfo?(cmanchester)
I'm not sure what problem bug 1231315 was fixing. I think you'll have to wait for glandium to get back from holiday and ask him.

The approach in this series seems reasonable to me. But I think you should run it by the C++ modules members - some subset of {ehsan, botond, froydnj, glandium, Waldo} - because they will be the ones that can answer whether this is a good idea. My biggest concern is for code that assumes mozilla-config.h is included and therefore doesn't worry about including individual .h files. In the new world, if you forget to include a .h file, #ifdef will always fail. This feels like the kind of thing that we can detect via static analysis though: we know what symbols require an explicit #include, so it should be possible to grep source for those symbols and error if the #include is missing.
Flags: needinfo?(gps)
Ok, let's start with froydnj. Can you apply the patches in this bug and see whether it works as desired when changing your mozconfig? If there's a particular define that you think would be useful to be able to turn on/off in configure and only update the subset of files that depend on it, it should be pretty easy to add similar to the MOZ_CALLGRIND patch (assuming the define of interest is in moz.configure)
Flags: needinfo?(ted)
Flags: needinfo?(nfroyd)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(cmanchester)
Comment on attachment 8862092 [details]
Bug 902825 - Rename non_global_defines to non_global_config;

https://reviewboard.mozilla.org/r/134074/#review145342

I'd rather avoid moving this back to configure if we can afford it, and I think we can, by moving this in install manifests (which can handle preprocessed files, with local defines, and should be using FileAvoidWrite).
Attachment #8862092 - Flags: review?(mh+mozilla)
Comment on attachment 8862093 [details]
Bug 902825 - Rename ALLDEFINES to GLOBAL_DEFINES;

https://reviewboard.mozilla.org/r/134076/#review145348

::: config/builddirs.py.in:5
(Diff revision 2)
> +topsrcdir = '@top_srcdir@'
> +topobjdir = '@topobjdir@'

I'd rather reuse MozbuildObject.from_environment() here, like buildconfig does. It shouldn't load config.status unless .defines, .non_global_defines and .substs are used. And to avoid the duplicated work, have buildconfig use the resulting config object from the builddirs module.

Alternatively, having buildconfig.{defines,non_global_defines,substs} be "lazy proxies" would work too, but that's likely more work than warranted.
Attachment #8862093 - Flags: review?(mh+mozilla)
Comment on attachment 8862094 [details]
Bug 902825 - Allow generating dist/include/mozconfig/*.h;

https://reviewboard.mozilla.org/r/134078/#review145350

::: commit-message-709c9:12
(Diff revision 2)
> +way to declare a mozconfig_defines in set_define(). mozconfig_defines
> +are essentially analogous to non_global_defines, except the latter are
> +specific to old-configure.in, and mozconfig_defines are specific to
> +moz.configure.

why does this need to have a different name?

::: commit-message-709c9:19
(Diff revision 2)
> +specific to old-configure.in, and mozconfig_defines are specific to
> +moz.configure.
> +
> +Ideally these would go into dist/include/mozconfig/ instead of
> +dist/mozconfig/mozconfig/, but the way the RecursiveMake backend uses install
> +manifests means they would be removed after we create them.

Just add them to the install manifest for dist/include.
Attachment #8862094 - Flags: review?(mh+mozilla)
Comment on attachment 8862095 [details]
Bug 902825 - Add non_global() to set non-global variables in moz.configure;

https://reviewboard.mozilla.org/r/134080/#review145352

::: commit-message-28095:3
(Diff revision 2)
> +Bug 902825 - Add some hacks to avoid rebuilding world; r?glandium
> +
> +Since MOZ_CONFIGURE_OPTIONS changes every time you enable or disable a

I wonder, at this point, if we shouldn't remove ALLSUBSTS and fill autoconf*.mk with the variables we do need. Or only fill ALLSUBSTS with the few things we do need (and rename it because ALL would be a lie). It feels like there shouldn't be a lot left used by Makefiles directly.

::: python/mozbuild/mozbuild/backend/configenvironment.py:169
(Diff revision 2)
> +        # that naturally always changes when configure options change. Things
> +        # that use MOZ_CONFIGURE_OPTIONS don't get it from ALLSUBSTS, so it
> +        # isn't actually needed here.
>          self.substs['ALLSUBSTS'] = '\n'.join(sorted(['%s = %s' % (name,
> -            serialize(self.substs[name])) for name in self.substs if self.substs[name]]))
> +            serialize(self.substs[name])) for name in self.substs if self.substs[name] and
> +                                                     name not in self.mozconfig_defines and

That doesn't feel right. ALLSUBSTS contains substs, not defines.
Attachment #8862095 - Flags: review?(mh+mozilla)
Comment on attachment 8862096 [details]
Bug 902825 - Remove MOZ_CALLGRIND from mozilla-config.h;

https://reviewboard.mozilla.org/r/134082/#review145354

::: js/moz.configure:128
(Diff revision 2)
>  @depends('--enable-callgrind')
>  def callgrind(value):
>      if value:
>          return True
>  
> -set_define('MOZ_CALLGRIND', callgrind)
> +set_define('MOZ_CALLGRIND', callgrind, mozconfig_header=True)

I'm still unsure what the distinction for "mozconfig_header"s buys you.
Attachment #8862096 - Flags: review?(mh+mozilla)
Clearing ni? for now, since glandium wants these reworked.  I promise to find more time to actually try these out after they are reworked. :)
Flags: needinfo?(nfroyd)
(In reply to Mike Hommey [:glandium] from comment #48)
> I'd rather avoid moving this back to configure if we can afford it, and I
> think we can, by moving this in install manifests (which can handle
> preprocessed files, with local defines, and should be using FileAvoidWrite).

How should this handle the special logic in process_define_files.py? The CONFIGURE_DEFINE_FILES aren't just preprocessed with the preprocessor, but have the logic for #undef and such: https://dxr.mozilla.org/mozilla-central/rev/5322c03f4c8587fe526172d3f87160031faa6d75/python/mozbuild/mozbuild/action/process_define_files.py#63

Should this be a new type of entry in the manifest? Eg: a ConfigureDefineFile to go alongside PreprocessedFile that would use process_define_files.py during the manifest processing.
Flags: needinfo?(mh+mozilla)
Separately, it seems to me that if we move the CONFIGURE_DEFINE_FILES into install manifests somehow (whether by using process_define_files.py, or some other mechanism), they will still depend on config.status. Which means if config.status changes, we will re-process them on every build since we'd be using FileAvoidWrite. This puts us essentially in the same place that we were in before bug 1218999, which is the other half of the two conflicting bugs that makes this not work. Should we just back out bug 1218999 instead of bug 1231315? This would put us back to potentially processing some files unnecessarily during a build, but I believe we'll have that anyway with the install manifest approach.
I think I worked out an alternative that allows actions like process_define_files() to have finer-grained dependencies on the config values they actually use, so we don't need to back out either of the bugs or try to make install manifests work. I still need to clean up the patches, so I'll post them in a bit...
Flags: needinfo?(mh+mozilla)
Comment on attachment 8862093 [details]
Bug 902825 - Rename ALLDEFINES to GLOBAL_DEFINES;

https://reviewboard.mozilla.org/r/134076/#review145348

> I'd rather reuse MozbuildObject.from_environment() here, like buildconfig does. It shouldn't load config.status unless .defines, .non_global_defines and .substs are used. And to avoid the duplicated work, have buildconfig use the resulting config object from the builddirs module.
> 
> Alternatively, having buildconfig.{defines,non_global_defines,substs} be "lazy proxies" would work too, but that's likely more work than warranted.

The latest patchset doesn't use a separate builddirs module - buildconfig still uses MozbuildObject.from_environment(), but allows access to defines & substs individually.
Comment on attachment 8862094 [details]
Bug 902825 - Allow generating dist/include/mozconfig/*.h;

https://reviewboard.mozilla.org/r/134078/#review145350

> Just add them to the install manifest for dist/include.

Now that all recursive make install manifests use --track, these just go in dist/include/mozconfig and we don't need to add a separate -I flag for them.
Comment on attachment 8862094 [details]
Bug 902825 - Allow generating dist/include/mozconfig/*.h;

https://reviewboard.mozilla.org/r/134078/#review145350

> why does this need to have a different name?

For a mozconfig_define to work properly (meaning we can change its value and only rebuild things that depend on it), we have to remove it from ALLDEFINES (mozilla-config.h) and ALLSUBSTS (autoconf.mk). Some non_global_defines are still used in Makefile.ins, so they need to be in ALLSUBSTS for now.
Comment on attachment 8862095 [details]
Bug 902825 - Add non_global() to set non-global variables in moz.configure;

https://reviewboard.mozilla.org/r/134080/#review145352

> I wonder, at this point, if we shouldn't remove ALLSUBSTS and fill autoconf*.mk with the variables we do need. Or only fill ALLSUBSTS with the few things we do need (and rename it because ALL would be a lie). It feels like there shouldn't be a lot left used by Makefiles directly.

I definitely like the idea, and think we should work towards that. I don't think we're quite at a point where we can explicitly list them out, though. There are still many variables used by config.mk/rules.mk, and the Android Makefiles have a bunch, too. IMO it would be a bit of a pain to enumerate them and keep it up-to-date, at least until we get more of the toolchain invocations into mozbuild. At that point I agree we should kill ALLSUBSTS.
Comment on attachment 8862095 [details]
Bug 902825 - Add non_global() to set non-global variables in moz.configure;

https://reviewboard.mozilla.org/r/134080/#review145352

> That doesn't feel right. ALLSUBSTS contains substs, not defines.

Good point - I'm happy to use a better name if you have one. The reason we do need to remove them here is that some defines are also substs (eg: MOZ_PERMISSIONS uses both set_config() and set_define(), which was one of my first experiments). And since all substs go into autoconf.mk and everything has autoconf.mk as a dependency, if we change the value of that configure option, then everything gets rebuilt even if mozilla-config.h is unchanged.
Comment on attachment 8862096 [details]
Bug 902825 - Remove MOZ_CALLGRIND from mozilla-config.h;

https://reviewboard.mozilla.org/r/134082/#review145354

> I'm still unsure what the distinction for "mozconfig_header"s buys you.

Setting this flag tells the build system that this define won't be present in ALLDEFINES (mozilla-config.h), so any C++ code that uses the define needs to explicitly include the mozconfig/DEFINE.h header to use it. Additionally, if it is a subst it is removed from ALLSUBSTS, and should be accessed by buildconfig.get_subst() so that Python dependencies can be tracked accurately. If the C++ changes aren't made, then we can't remove the define from mozilla-config.h since the C++ code will have nowhere else to find the define.
These patches still need some good tests, so I'm just looking for a pre-review here on whether or not this approach will work. The first 3 patches fix things so that we don't rebuild just because configure ran and config.status was touched - these will probably be useful regardless since it makes configure runs less expensive. But with only these three, toggling a define (eg: turning --enable-callgrind on and off) will update mozilla-config.h, and so rebuild the world.

The last 3 are to implement the mozconfig_header mechanism and show an example of how it can be used with MOZ_CALLGRIND. In this case, toggling --enable-callgrind on and off will only rebuild the few files that use it, since mozilla-config.h is unchanged.

Also flagging froydnj for a needinfo since he promised to look and see if it actually helps the workflow :)
Flags: needinfo?(nfroyd)
I added some tests. I think it's ready for a full review.
I rebased to the latest m-c, which picked up some conflicts in the buildconfig.py commit. Both mozversioncontrol/__init__.py and mozjar.py required minor updates.
Comment on attachment 8862094 [details]
Bug 902825 - Allow generating dist/include/mozconfig/*.h;

https://reviewboard.mozilla.org/r/134078/#review182208

::: python/mozbuild/mozbuild/configure/__init__.py:853
(Diff revision 5)
>          when = self._normalize_when(when, 'set_config')
>  
>          self._execution_queue.append((
>              self._resolve_and_set, (self._config, name, value, when)))
>  
> -    def set_define_impl(self, name, value, when=None):
> +    def set_define_impl(self, name, value, when=None, mozconfig_header=False):

I don't see a reason why this should be optional.
Attachment #8862094 - Flags: review?(mh+mozilla)
Comment on attachment 8862095 [details]
Bug 902825 - Add non_global() to set non-global variables in moz.configure;

https://reviewboard.mozilla.org/r/134080/#review182210

::: python/mozbuild/mozbuild/backend/configenvironment.py:173
(Diff revision 5)
> +        # dependency on autoconf.mk in the RecursiveMake backend. For the same
> +        # reasons, we don't include MOZ_CONFIGURE_OPTIONS in ALLSUBSTS, since
> +        # that naturally always changes when configure options change. Things
> +        # that use MOZ_CONFIGURE_OPTIONS don't get it from ALLSUBSTS, so it
> +        # isn't actually needed here.
>          self.substs['ALLSUBSTS'] = '\n'.join(sorted(['%s = %s' % (name,

> IMO it would be a bit of a pain to enumerate them and keep it up-to-date, at least until we get more of the toolchain invocations into mozbuild.

How about this: we enumerate all the variables we have *now* and any new one is not available from makefiles unless you touch autoconf.mk.
Attachment #8862095 - Flags: review?(mh+mozilla)
Comment on attachment 8862092 [details]
Bug 902825 - Rename non_global_defines to non_global_config;

https://reviewboard.mozilla.org/r/134074/#review182204

::: python/mozbuild/mozbuild/backend/configenvironment.py:249
(Diff revision 5)
> +        self._defines = {}
> +        self.topobjdir = topobjdir
> +        self._files = set()
> +        self._config_track = mozpath.join(topobjdir, 'config.statusd', 'config.track')
> +
> +    def _get_data(self, typ, var, default_value=None):

You could have a single separate pseudo-dict class, an instance of which would be self.substs and another, self.defines. Then the other changes would be much smaller, without a risk to miss some parts.

::: python/mozbuild/mozbuild/backend/configenvironment.py:255
(Diff revision 5)
> +        data = default_value
> +        try:
> +            filename = mozpath.join(self.topobjdir, 'config.statusd', typ, var)
> +            self._files.add(filename)
> +            with open(filename) as f:
> +                data = eval(f.read())

eval makes me cringe a little. json.loads would seem more appropriate (and works for simple types, e.g. json.loads('1') return 1)

::: python/mozbuild/mozbuild/backend/configenvironment.py:267
(Diff revision 5)
> +            self._substs[var] = self._get_data('substs', var, default_value)
> +            if (self._substs[var] is not None) and (var not in ('CPP', 'CXXCPP', 'SHELL')) and (var in os.environ):
> +                self._substs[var] = os.environ[var]
> +        return self._substs[var]
> +
> +    def set_subst(self, var, value):

why do you need a set_subst and not a set_define? I guess the answer is in the other patches...
Attachment #8862092 - Flags: review?(mh+mozilla)
Comment on attachment 8862092 [details]
Bug 902825 - Rename non_global_defines to non_global_config;

https://reviewboard.mozilla.org/r/134074/#review182204

> You could have a single separate pseudo-dict class, an instance of which would be self.substs and another, self.defines. Then the other changes would be much smaller, without a risk to miss some parts.

Good idea - this does simplify things. I considered this originally, but went with .get_subst() so that it was a little more obvious that it wasn't a plain dict. However, I agree it made it too much of a mess.

> eval makes me cringe a little. json.loads would seem more appropriate (and works for simple types, e.g. json.loads('1') return 1)

Yes, that works much better. The only odd thing I noticed is now buildconfig.substs['PERL'] is unicode instead of a string, which makes generate_certdata.py fail on Windows unless you convert it to a string. Otherwise, it seems to work just fine.

> why do you need a set_subst and not a set_define? I guess the answer is in the other patches...

set_subst() is gone now in favor of the __setitem__ method, so defines should have work for this as well. I don't believe we set defines anywhere, but we manually override substs in l10n-repack.py and unpack.py.
Comment on attachment 8862094 [details]
Bug 902825 - Allow generating dist/include/mozconfig/*.h;

https://reviewboard.mozilla.org/r/134078/#review182208

> I don't see a reason why this should be optional.

I gave it a default so we could leave all the set_define('FOO', foo) unchanged for the time being. Would you prefer having no default and changing all the existing set_defines to set_define('FOO', foo, false) or something?
(In reply to Mike Hommey [:glandium] from comment #85)
> > IMO it would be a bit of a pain to enumerate them and keep it up-to-date, at least until we get more of the toolchain invocations into mozbuild.
> 
> How about this: we enumerate all the variables we have *now* and any new one
> is not available from makefiles unless you touch autoconf.mk.

Mind if I file a followup bug for this? I do agree it should be done, but changing ALLSUBSTS or autoconf.mk further doesn't fix things for this bug.
(In reply to Michael Shal [:mshal] (out - 9/18) from comment #89)
> (In reply to Mike Hommey [:glandium] from comment #85)
> > > IMO it would be a bit of a pain to enumerate them and keep it up-to-date, at least until we get more of the toolchain invocations into mozbuild.
> > 
> > How about this: we enumerate all the variables we have *now* and any new one
> > is not available from makefiles unless you touch autoconf.mk.
> 
> Mind if I file a followup bug for this? I do agree it should be done, but
> changing ALLSUBSTS or autoconf.mk further doesn't fix things for this bug.

It avoids the hacks you put in place in this bug.
Comment on attachment 8862092 [details]
Bug 902825 - Rename non_global_defines to non_global_config;

https://reviewboard.mozilla.org/r/134074/#review183556

::: python/mozbuild/mozbuild/backend/configenvironment.py:242
(Diff revision 6)
> +            if self._environ_override:
> +                if (self._dict[key] is not None) and (key not in ('CPP', 'CXXCPP', 'SHELL')) and (key in os.environ):
> +                    self._dict[key] = os.environ[key]

Why not entirely skip reading the file if we get the value from the environment?

::: python/mozbuild/mozbuild/backend/configenvironment.py:257
(Diff revision 6)
> +        data = None
> +        try:
> +            data = self[key]
> +        except KeyError:
> +            pass
> +        return data is not None

try:
    return self[key] is not None
except KeyError:
    return False

::: python/mozbuild/mozbuild/backend/configenvironment.py:330
(Diff revision 6)
> +        global_defines = [name for name in config['defines']
> +            if not name in config['non_global_defines']
> +            ]

That's weird indentation.

I'd go for
... = [
   ... for ... in ...
   if ...
]

::: python/mozbuild/mozbuild/backend/configenvironment.py:352
(Diff revision 6)
> +            # we just overwrite the file with a value of None, which is equivalent
> +            # to a non-existing file.
> +            with FileAvoidWrite(filename) as fh:
> +                json.dump(None, fh)
> +
> +        with open(self._config_track, 'w') as fh:

FileAvoidWrite?

::: python/mozbuild/mozbuild/backend/configenvironment.py:357
(Diff revision 6)
> +        with open(self._config_track, 'w') as fh:
> +            for f in sorted(new_files):
> +                fh.write('%s\n' % f)
> +
> +    def get_dependencies(self):
> +        files = set()

you're not using this variable.
Attachment #8862092 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8862093 [details]
Bug 902825 - Rename ALLDEFINES to GLOBAL_DEFINES;

https://reviewboard.mozilla.org/r/134076/#review183562
Attachment #8862093 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8902405 [details]
Bug 902825 - Remove explicit NECKO_WIFI define;

https://reviewboard.mozilla.org/r/173986/#review183564

::: mobile/android/base/generate_build_config.py:36
(Diff revision 4)
> -    CONFIG.update(buildconfig.substs)
> -    DEFINES = dict(buildconfig.defines)
> +    CONFIG.update(buildconfig.config.substs)
> +    DEFINES = dict(buildconfig.config.defines)

huh?

::: toolkit/crashreporter/tools/unit-symbolstore.py:157
(Diff revision 4)
> -    @patch.dict('buildconfig.substs', {'MAKECAB': 'makecab'})
>      def test_copy_debug_copies_binaries(self):
>          """
>          Test that CopyDebug copies binaries as well on Windows.
>          """
> +        buildconfig.substs['MAKECAB'] = 'makecab'

mmmm you're altering substs for all tests... not that it actually should matter much...

why does patch.dict not work?

::: toolkit/mozapps/installer/find-dupes.py:105
(Diff revision 4)
>      args = parser.parse_args()
>  
>      allowed_dupes = []
>      for filename in args.dupes_files:
>          pp = Preprocessor()
> -        pp.context.update(buildconfig.defines)
> +        pp.context.update(buildconfig.config.defines)

defines['ALL_DEFINES'] ?

::: toolkit/mozapps/installer/packager.py:229
(Diff revision 4)
>      parser.add_argument('--non-resource', nargs='+', metavar='PATTERN',
>                          default=[],
>                          help='Extra files not to be considered as resources')
>      args = parser.parse_args()
>  
> -    defines = dict(buildconfig.defines)
> +    defines = dict(buildconfig.config.defines)

same here
Attachment #8902405 - Flags: review?(mh+mozilla)
Comment on attachment 8862094 [details]
Bug 902825 - Allow generating dist/include/mozconfig/*.h;

https://reviewboard.mozilla.org/r/134078/#review183566

::: commit-message-709c9:19
(Diff revision 6)
> +
> +1) They are used in C/C++ code by doing #include "mozconfig/DEFINE_NAME.h"
> +
> +And if the define is also a subst, then:
> +
> +2) They are used in Python code with buildconfig.get_subst()

This needs an update, there's no get_subst anymore.

::: python/mozbuild/mozbuild/backend/configenvironment.py:153
(Diff revision 6)
>          global_defines = [name for name in self.defines
> -            if not name in self.non_global_defines]
> +            if not name in self.non_global_defines and
> +            name not in self.mozconfig_defines
> +            ]

weird indentation

::: python/mozbuild/mozbuild/configure/__init__.py:853
(Diff revision 6)
>          when = self._normalize_when(when, 'set_config')
>  
>          self._execution_queue.append((
>              self._resolve_and_set, (self._config, name, value, when)))
>  
> -    def set_define_impl(self, name, value, when=None):
> +    def set_define_impl(self, name, value, when=None, mozconfig_header=False):

> I gave it a default so we could leave all the set_define('FOO', foo) unchanged for the time being. Would you prefer having no default and changing all the existing set_defines to set_define('FOO', foo, false) or something?

Just create files for every single one of them. I don't see why we should allow to opt-in or opt-out.
Attachment #8862094 - Flags: review?(mh+mozilla)
Comment on attachment 8862095 [details]
Bug 902825 - Add non_global() to set non-global variables in moz.configure;

https://reviewboard.mozilla.org/r/134080/#review183572

::: python/mozbuild/mozbuild/backend/configenvironment.py:175
(Diff revision 6)
> +        # that naturally always changes when configure options change. Things
> +        # that use MOZ_CONFIGURE_OPTIONS don't get it from ALLSUBSTS, so it
> +        # isn't actually needed here.
>          self.substs['ALLSUBSTS'] = '\n'.join(sorted(['%s = %s' % (name,
> -            serialize(name, self.substs[name])) for name in self.substs if self.substs[name]]))
> +            serialize(name, self.substs[name])) for name in self.substs if self.substs[name] and
> +                                                     name not in self.mozconfig_defines and

This actually doesn't make sense. ALLSUBSTS contains things from set_config, not set_define. Why should mozconfig_defines (which tracks set_defines) have an influence on things set with set_config?
Attachment #8862095 - Flags: review?(mh+mozilla)
Comment on attachment 8862096 [details]
Bug 902825 - Remove MOZ_CALLGRIND from mozilla-config.h;

https://reviewboard.mozilla.org/r/134082/#review183574
Attachment #8862096 - Flags: review?(mh+mozilla) → review+
BTW, if you want to avoid rebuilding the world when mozilla-config.h changes, you can change its content to #include the mozconfig/ headers instead of keeping its current #defines.
Comment on attachment 8862092 [details]
Bug 902825 - Rename non_global_defines to non_global_config;

https://reviewboard.mozilla.org/r/134074/#review186688

::: python/mozbuild/mozbuild/backend/configenvironment.py:330
(Diff revision 6)
> +        global_defines = [name for name in config['defines']
> +            if not name in config['non_global_defines']
> +            ]

Also switched to using 'if name not in' instead of 'if not name in'.
Comment on attachment 8902405 [details]
Bug 902825 - Remove explicit NECKO_WIFI define;

https://reviewboard.mozilla.org/r/173986/#review183564

> huh?

I added an __iter__ method to PartialConfigDict(), which means we can now call update() on it. To do so I had to move the .track file logic from PartialConfigEnvironment to PartialConfigDict so that it knows the full list of variables. Unfortunately this will make the interdiff of the config.statusd commit a little busy, but it seems to be simpler overall.

> mmmm you're altering substs for all tests... not that it actually should matter much...
> 
> why does patch.dict not work?

Internally it looks like mock.py tries to call .copy() and .clear() methods on the (assumed) dict, and has a backup way of using __iter__ / __delitem__ if those aren't enabled. Unfortunately, it expects __iter__ to return just the keys like iterating through a dict would, but calling update on the dict (needed for CONFIG.update(buildconfig.substs) in the other issue) expects __iter__ to return (key, value) tuples. I'm not sure how to support both with that, and writing supporting copy() and clear() just for this test case seems a bit overkill.

I changed the test to patch buildconfig.substs._dict directly instead.
Depends on: 1402012
I've spent a decent amount of time trying to remove ALLSUBSTS by using an explicit list, but I don't think it's a good approach at this time. There are still hundreds of actively used substs, so it's hard to narrow down and verify a specific list. Listing them out also makes it very easy to miss a subst, and a build won't necessarily fail if the subst is missing, but just build incorrectly.

I revisited an idea from https://bugzilla.mozilla.org/show_bug.cgi?id=902825#c50, which is to unify the mozconfig_header list with the non_global_defines. However, since part of the cause of rebuilding world is the substs in ALLSUBSTS, I've tried the following approach:

1) Rename non_global_defines to non_global_config, so that we can use this list to prune variables from both ALLDEFINES (mozilla-config.h) and ALLSUBSTS (autoconf.mk)
2) Remove the sole non-global subst in use (NECKO_WIFI), so we can safely remove all existing non-global config variables from autoconf.mk
3) Add a way of setting non-global config items from moz.configure, so we can ignore MOZ_CONFIGURE_OPTIONS without resorting to hacks.

Since ALLSUBSTS/ALLDEFINES don't actually contain "all" of those values, I've renamed them to GLOBAL_SUBSTS and GLOBAL_DEFINES, which makes it more obvious that they exclude the non-global configure variables.

Probably the only odd part now is that since we are no longer specifying which defines are non-global in set_define(), we need a way to keep track of all possible defines so that a header can be generated. We have to generate the header (eg: MOZ_CALLGRIND.h) whether or not the define exists so C++ compilation will work, but we only want to generate headers for the non-global config items that are possible defines (so we don't want a MOZ_CONFIGURE_OPTIONS.h, since that is only a subst). I've done this now by adding all defines to config['DEFINES'], and then later pruning out those with a value of None. Another option would be to have a separate list of just the variable names, though that is essentially redundant with defines.keys().
Comment on attachment 8862092 [details]
Bug 902825 - Rename non_global_defines to non_global_config;

https://reviewboard.mozilla.org/r/134074/#review193032
Comment on attachment 8862093 [details]
Bug 902825 - Rename ALLDEFINES to GLOBAL_DEFINES;

https://reviewboard.mozilla.org/r/134076/#review193034
Comment on attachment 8902405 [details]
Bug 902825 - Remove explicit NECKO_WIFI define;

https://reviewboard.mozilla.org/r/173986/#review193036
Attachment #8902405 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8862095 [details]
Bug 902825 - Add non_global() to set non-global variables in moz.configure;

https://reviewboard.mozilla.org/r/134080/#review193046
Attachment #8862095 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915754 [details]
Bug 902825 - Change ALLSUBSTS to GLOBAL_SUBSTS;

https://reviewboard.mozilla.org/r/186962/#review193048
Attachment #8915754 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915755 [details]
Bug 902825 - Mark MOZ_CONFIGURE_OPTIONS as non-global;

https://reviewboard.mozilla.org/r/186964/#review193050
Attachment #8915755 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8915756 [details]
Bug 902825 - Output a set of all defines from moz.configure;

https://reviewboard.mozilla.org/r/186966/#review193040

::: python/mozbuild/mozbuild/configure/__init__.py:841
(Diff revision 1)
> -        if name in data:
> +        if name in data and data[name] is not None:
>              raise ConfigureError(
>                  "Cannot add '%s' to configuration: Key already "
>                  "exists" % name)
>          value = self._resolve(value, need_help_dependency=False)
>          if value is not None:

seems like you could just go with if value is not None or always_set here.

::: python/mozbuild/mozbuild/configure/__init__.py:871
(Diff revision 1)
>          when = self._normalize_when(when, 'set_define')
>  
>          defines = self._config.setdefault('DEFINES', {})
> +
>          self._execution_queue.append((
> -            self._resolve_and_set, (defines, name, value, when)))
> +            self._resolve_and_set, (defines, name, value, when, True)))

always_set=True
Attachment #8915756 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8862094 [details]
Bug 902825 - Allow generating dist/include/mozconfig/*.h;

https://reviewboard.mozilla.org/r/134078/#review193052
Attachment #8862094 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8862096 [details]
Bug 902825 - Remove MOZ_CALLGRIND from mozilla-config.h;

https://reviewboard.mozilla.org/r/134082/#review193044

::: js/src/builtin/Profilers.cpp:15
(Diff revision 7)
>  
>  #include "mozilla/Sprintf.h"
>  
>  #include <stdarg.h>
>  
> +#include "mozconfig/MOZ_CALLGRIND.h"

is there a bug on file to make it an static analysis error to forget to include the mozconfig header when using the macro?
Product: Core → Firefox Build System
I *finally* tested this a while back and the feature works great!  <3
Flags: needinfo?(nfroyd)

IIRC the only thing that is really blocking this from landing is the static analysis check. If anyone has interest in writing that, this bug would still be very useful in avoiding full rebuilds on mozilla-config.h changes.

Assignee: mshal → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: