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

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Brian O'Keefe [:bokeefe]]

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.

Comment 1

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Remove useless includes of config.mk

These are the trivial cases where config.mk is immediately followed by rules.mk

Comment 2

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Remove less trivial config.mk includes

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

Comment 3

[Gregory Szorc [: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.

Comment 4

[Gregory Szorc [:gps]]

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.

Comment 5

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Remove less trivial config.mk includes, v2

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

Comment 6

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Remove less trivial config.mk includes, v3

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

Comment 7

[Gregory Szorc [: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!

Comment 8

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Remove useless includes of config.mk (for checkin, r=gps)

Comment 9

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Remove less trivial config.mk includes (for checkin, r=gps)

Comment 10

[Brian O'Keefe [:bokeefe]]

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

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/455da612ee5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf9c217ddd9

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/455da612ee5f
https://hg.mozilla.org/mozilla-central/rev/8bf9c217ddd9