Closed Bug 1402012 Opened 7 years ago Closed 7 years ago

Don't rebuild world when configure output is unchanged

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox57 wontfix, firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: mshal, Assigned: mshal)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

This is a subset of bug 902825, which I'm splitting out since this is the more straightforward part of the patchset and gives us the benefit of not rebuilding world when the configure output is unchanged. The rest of the work to only rebuild certain subsections of the code when a configure feature flag is changed will be finished in bug 902825.
See https://bugzilla.mozilla.org/show_bug.cgi?id=902825#c104 and https://bugzilla.mozilla.org/show_bug.cgi?id=902825#c105 for some comments on the last round of reviews in that bug.
Updated to avoid mentioning mozconfig_defines in a comment since those don't exist yet.
Comment on attachment 8910814 [details]
Bug 1402012 - Create config.statusd directory;

https://reviewboard.mozilla.org/r/182290/#review189062

::: python/mozbuild/mozbuild/backend/configenvironment.py:305
(Diff revision 2)
> +        try:
> +            return self[key] is not None
> +        except KeyError:
> +            return False
> +
> +    def __iter__(self):

it's weird that this does the equivalent of dict.iteritems, not dict.iter. Better name it iteritems.

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

fwiw, I think we can kill non global defines.

::: python/mozbuild/mozbuild/backend/configenvironment.py:361
(Diff revision 2)
> +        self.substs._write_group(substs)
> +        self.defines._write_group(defines)

maybe fill_group or fill would be better than write_group.
Attachment #8910814 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8910815 [details]
Bug 1402012 - Use PartialConfigEnvironment in process_define_files.py;

https://reviewboard.mozilla.org/r/182292/#review189066
Attachment #8910815 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8910816 [details]
Bug 1402012 - Update buildconfig.py to use PartialConfigEnvironment;

https://reviewboard.mozilla.org/r/182294/#review189072
Attachment #8910816 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8910814 [details]
Bug 1402012 - Create config.statusd directory;

https://reviewboard.mozilla.org/r/182290/#review189062

> it's weird that this does the equivalent of dict.iteritems, not dict.iter. Better name it iteritems.

We discussed this in IRC, but I've renamed this to iteritems() and now call iteritems explicitly where buildconfig.defines / substs are used in dict.update() calls. Internally Python has some special-casing for how dicts are handled in these cases, and it seems we can't replicate it exactly by overriding methods, even if we subclass dict.

I also noticed the function was using the full path as the key, which magically worked because __getitem__ still finds the file (the mozpath.join just keeps the full path). I've updated this to use the basename so we don't end up with weird keys in the dict.
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1d46bec63ce
Create config.statusd directory; r=glandium
https://hg.mozilla.org/integration/autoland/rev/22ebbb5d30d4
Use PartialConfigEnvironment in process_define_files.py; r=glandium
https://hg.mozilla.org/integration/autoland/rev/cb773c661e0c
Update buildconfig.py to use PartialConfigEnvironment; r=glandium
Depends on: 1414060
Depends on: 1423438
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.