Move MOZ_BUILDID define for nsAppRunner.cpp out of Makefile.in

RESOLVED FIXED in Firefox 57

Status

Firefox Build System
General
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: Chris Manchester (mostly offline July 16-20), Unassigned)

Tracking

unspecified
mozilla57

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

It's not immediately obvious how to achieve this, but it's the only define that's out of sync with bug 1362612 so far on a desktop build. The define is added here: http://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/toolkit/xre/Makefile.in#21
So looking at this a little more closely it looks like there's no point in replicating it for a tup backend, at least. We only put MOZ_BUILDID in the Makefile so we don't actually end up re-building nsAppRunner.cpp for every toplevel build, but as I understand it the same trick wouldn't work if we replicated it in tup, because changing the command line in the Tupfile would cause the file to be re-compiled anyway.

So once we have a tup build that's working we'll want to address this issue more generally so we don't waste a lot of time re-compiling and linking in top level no-op builds due to buildid.h changing with every invocation of the build system.

Mike, can you confirm tup's behavior in this case? If so I think we should just add a special variable to add Makefile defines to our compile command lines in Make so we can move DEFINES to mozbuild and maintain the existing behavior while we transition things.
Flags: needinfo?(mshal)
(In reply to Chris Manchester (:chmanchester) from comment #1)
> So looking at this a little more closely it looks like there's no point in
> replicating it for a tup backend, at least. We only put MOZ_BUILDID in the
> Makefile so we don't actually end up re-building nsAppRunner.cpp for every
> toplevel build, but as I understand it the same trick wouldn't work if we
> replicated it in tup, because changing the command line in the Tupfile would
> cause the file to be re-compiled anyway.

Right, tup would re-compile it if the command-line changes. Though if we were generating a different command-line for every build invocation, presumably we'd have to regenerate the build backend to do so. That would be even more unnecessary work on top of re-linking XUL.

> Mike, can you confirm tup's behavior in this case? If so I think we should
> just add a special variable to add Makefile defines to our compile command
> lines in Make so we can move DEFINES to mozbuild and maintain the existing
> behavior while we transition things.

That sounds reasonable. The tup backend should "work" in that nsAppRunner.cpp will include buildid.h without this define present (this logic is in nsAppRunner.cpp itself, not the build backend), and make can continue to use the dependency-skipping hack in the Makefile.in to avoid re-linking XUL. Tup doesn't have a way to lie about dependencies though, which means it would re-link XUL on every build. This is obviously not ideal, but we can try to address that later.
Flags: needinfo?(mshal)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
Comment on attachment 8894748 [details]
Bug 1380547 - Add a variable other than DEFINES for defines that are set from Makefiles.

https://reviewboard.mozilla.org/r/165922/#review171462

::: config/config.mk:345
(Diff revision 1)
>  
>  HOST_CFLAGS += $(HOST_DEFINES) $(MOZBUILD_HOST_CFLAGS)
>  HOST_CXXFLAGS += $(HOST_DEFINES) $(MOZBUILD_HOST_CXXFLAGS)
>  
> +# Add Makefile-set defines, if any.
> +ifdef MK_COMPILE_DEFINES

Since $(FOO) would default to empty if the variable doesn't exist, you can skip the ifdef and just add $(MK_COMPILE_DEFINES) to the COMPILE_CFLAGS / COMPILE_CXXFLAGS definitions a few lines up.

::: toolkit/xre/Makefile.in:15
(Diff revision 1)
>  MOZ_BUILDID   := $(shell awk '{print $$3}' $(DEPTH)/buildid.h)
>  $(call errorIfEmpty,GRE_MILESTONE MOZ_BUILDID)
>  
> +MK_COMPILE_DEFINES = -DMOZ_BUILDID=$(MOZ_BUILDID)
> +
> +include $(topsrcdir)/config/rules.mk

I think by changing how MK_COMPILE_DEFINES is added in rules.mk, you could leave this line where it was because COMPILE_CXXFLAGS and the like would have a delayed expansion. (If I'm wrong on that, moving it here is fine.)
Attachment #8894748 - Flags: review?(mshal) → review+
(Reporter)

Comment 5

a year ago
mozreview-review-reply
Comment on attachment 8894748 [details]
Bug 1380547 - Add a variable other than DEFINES for defines that are set from Makefiles.

https://reviewboard.mozilla.org/r/165922/#review171462

> I think by changing how MK_COMPILE_DEFINES is added in rules.mk, you could leave this line where it was because COMPILE_CXXFLAGS and the like would have a delayed expansion. (If I'm wrong on that, moving it here is fine.)

Yep, that works fine.
Comment hidden (mozreview-request)

Comment 7

a year ago
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c42bcf50f52
Add a variable other than DEFINES for defines that are set from Makefiles. r=mshal

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9c42bcf50f52
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.