Closed Bug 896177 Opened 7 years ago Closed 7 years ago

Remove config.mk includes that are in the way of the LIBRARY_NAME migration

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: bokeefe, Assigned: bokeefe)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

LIBRARY_NAME and friends need to be defined before config.mk is included, so it can set up to link the library correctly. If LIBRARY_NAME is defined in moz.build, a number of Makefile.ins that include config.mk themselves end up with LIBRARY_NAME defined after config.mk is included, and the build fails.

I have patches to delete our way to victory, so we can finish the LIBRARY_NAME conversions.
These are the trivial cases where config.mk is immediately followed by rules.mk
Attachment #778841 - Flags: review?(gps)
These are the non-trivial includes of config.mk. It turns out most of them aren't needed, but there were some more interesting cases:

content/smil/Makefile.in - Put the includes in LOCAL_INCLUDES instead of appending to INCLUDES, because INCLUDES is blown away in config.mk

dom/bindings/Makefile.in - This was just including config.mk for the side effect of including autoconf.mk, so I swapped them.

security/build/Makefile.in - This was using core_abspath from config.mk, so I copied that function to the Makefile. It's only used for converting the DIST dir into an absolute path.

This plus the previous patch were green on try: https://tbpl.mozilla.org/?tree=Try&rev=cd882502df66
Attachment #778842 - Flags: review?(gps)
Comment on attachment 778841 [details] [diff] [review]
Part 1: Remove useless includes of config.mk

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

Love it! Would review again.
Attachment #778841 - Flags: review?(gps) → review+
Comment on attachment 778842 [details] [diff] [review]
Part 2: Remove less trivial config.mk includes

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

Hmmm. Parts of this patch scare me a bit because of manipulations to config.mk-defined variables happening in Makefile.in. Although, moving variables to moz.build is essentially the same as removing config.mk from Makefile.in since backend.mk is included before config.mk. And, considering that variables in config.mk are deferred not immediate (= vs :=), this ideally shouldn't have an impact.

I didn't exhaustively audit each changed Makefile to ensure there was nothing funky like variable filtering going on between the removed config.mk and rules.mk inclusion. I'll do that before final review.

Please move core_* functions to a .mk file and resubmit for review.

::: ipc/chromium/Makefile.in
@@ -36,5 @@
>    $(srcdir)/src/base \
>    $(srcdir)/src/chrome/common \
>    $(NULL)
>  
> -include $(topsrcdir)/config/config.mk

This change initially scared me because of all the manipulations in this file. But, it looks like things should be sane.

::: security/build/Makefile.in
@@ +22,5 @@
>  
> +# This used to be defined in config.mk. Until the moz.build migration is complete,
> +# we need this here to avoid including config.mk, because doing so prevents us
> +# from moving a number of variables to moz.build
> +core_abspath = $(if $(findstring :,$(1)),$(1),$(if $(filter /%,$(1)),$(1),$(CURDIR)/$(1)))

Let's move all these core_*path* functions to a file in /config/makefiles and have both config.mk and this Makefile include it.
Attachment #778842 - Flags: review?(gps) → feedback+
(In reply to Gregory Szorc [:gps] from comment #4)
> ::: security/build/Makefile.in
> @@ +22,5 @@
> >  
> > +# This used to be defined in config.mk. Until the moz.build migration is complete,
> > +# we need this here to avoid including config.mk, because doing so prevents us
> > +# from moving a number of variables to moz.build
> > +core_abspath = $(if $(findstring :,$(1)),$(1),$(if $(filter /%,$(1)),$(1),$(CURDIR)/$(1)))
> 
> Let's move all these core_*path* functions to a file in /config/makefiles
> and have both config.mk and this Makefile include it.

I moved these all into /config/makefiles/functions.mk. If any other functions need to get moved during all the moz.build conversions, they'll have a place to live.
Attachment #778842 - Attachment is obsolete: true
Attachment #780002 - Flags: review?(gps)
Bah, botched a copy command, so check-sync-dirs doesn't like the previous version.

Try in progress at https://tbpl.mozilla.org/?tree=Try&rev=e23b824c95ea
Attachment #780002 - Attachment is obsolete: true
Attachment #780002 - Flags: review?(gps)
Attachment #780329 - Flags: review?(gps)
Comment on attachment 780329 [details] [diff] [review]
Part 2: Remove less trivial config.mk includes, v3

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

Gah, this lingered in my review queue for way too long, especially considering how trivial the interdiff is. Sorry about that!
Attachment #780329 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #7)
> Gah, this lingered in my review queue for way too long, especially
> considering how trivial the interdiff is. Sorry about that!

Not a problem; I've been completely swamped with unrelated things in any case. Thanks for reviewing!

I updated both patches to tip, courtesy of `hg pull --rebase` magic.
Keywords: checkin-needed
Attachment #780329 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/455da612ee5f
https://hg.mozilla.org/mozilla-central/rev/8bf9c217ddd9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 906310
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.