Closed Bug 1042878 Opened 5 years ago Closed 5 years ago

move MOZ_CAIRO_CFLAGS et al additions to C*FLAGS into moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

919 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
87.29 KB, patch
glandium
: review+
Details | Diff | Splinter Review
13.67 KB, patch
glandium
: review+
Details | Diff | Splinter Review
No description provided.
This patch also takes care of some Makefile.ins whose sole purpose was to add
appropriate flags.

I've no idea if the .split() idiom is kosher.  I suppose the Right Thing would
be to make the emitter for config.status churn out richer Python values than
strings, but perhaps that fix can be saved for later.

Builds on x86-64 Linux, need to run this through Try.
Attachment #8461050 - Flags: review?(mh+mozilla)
Attachment #8461050 - Flags: review?(gps)
Comment on attachment 8461050 [details] [diff] [review]
move MOZ_CAIRO_CFLAGS et al additions to C*FLAGS into moz.build

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

Note I'd rather change those variables to use AC_SUBST_LIST first. Instead of having to remove all those splits afterwards.

::: gfx/2d/Makefile.in
@@ -6,5 @@
> -ifeq ($(MOZ_WIDGET_TOOLKIT),$(findstring $(MOZ_WIDGET_TOOLKIT),android gtk2 gtk3 gonk qt))
> -OS_CXXFLAGS += $(CAIRO_FT_CFLAGS)
> -endif
> -
> -include $(topsrcdir)/config/rules.mk

That include needs to be kept because of the DEFINES hack below.

::: gfx/skia/generate_mozbuild.py
@@ +133,5 @@
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'qt':
> +    CXXFLAGS += [
> +        CONFIG['MOZ_CAIRO_CFLAGS'],
> +        CONFIG['MOZ_PANGO_CFLAGS'],
> +        CONFIG['CAIRO_FT_CFLAGS'],

What happened to split?
Attachment #8461050 - Flags: review?(mh+mozilla)
Attachment #8461050 - Flags: review?(gps)
Attachment #8461050 - Flags: review+
(In reply to Mike Hommey [:glandium] from comment #2)
> Note I'd rather change those variables to use AC_SUBST_LIST first. Instead
> of having to remove all those splits afterwards.

Do you want to see that patch, or rs+?

> ::: gfx/2d/Makefile.in
> @@ -6,5 @@
> > -ifeq ($(MOZ_WIDGET_TOOLKIT),$(findstring $(MOZ_WIDGET_TOOLKIT),android gtk2 gtk3 gonk qt))
> > -OS_CXXFLAGS += $(CAIRO_FT_CFLAGS)
> > -endif
> > -
> > -include $(topsrcdir)/config/rules.mk
> 
> That include needs to be kept because of the DEFINES hack below.

I was hoping to get rid of those DEFINES hacks in a followup patch.  Is that easily doable in moz.build, or does the -DUNICODE define get added at the wrong time?

> ::: gfx/skia/generate_mozbuild.py
> @@ +133,5 @@
> > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'qt':
> > +    CXXFLAGS += [
> > +        CONFIG['MOZ_CAIRO_CFLAGS'],
> > +        CONFIG['MOZ_PANGO_CFLAGS'],
> > +        CONFIG['CAIRO_FT_CFLAGS'],
> 
> What happened to split?

Probably was doing these before I realized split() was needed and I didn't go back and fix them up.
Flags: needinfo?(mh+mozilla)
(In reply to Nathan Froyd (:froydnj) from comment #3)
> (In reply to Mike Hommey [:glandium] from comment #2)
> > Note I'd rather change those variables to use AC_SUBST_LIST first. Instead
> > of having to remove all those splits afterwards.
> 
> Do you want to see that patch, or rs+?

The former.

> > ::: gfx/2d/Makefile.in
> > @@ -6,5 @@
> > > -ifeq ($(MOZ_WIDGET_TOOLKIT),$(findstring $(MOZ_WIDGET_TOOLKIT),android gtk2 gtk3 gonk qt))
> > > -OS_CXXFLAGS += $(CAIRO_FT_CFLAGS)
> > > -endif
> > > -
> > > -include $(topsrcdir)/config/rules.mk
> > 
> > That include needs to be kept because of the DEFINES hack below.
> 
> I was hoping to get rid of those DEFINES hacks in a followup patch.  Is that
> easily doable in moz.build, or does the -DUNICODE define get added at the
> wrong time?

It's both not easily doable in moz.build (we have nothing (yet) to exclude predefined stuff) and added at the wrong time (config.mk).
Flags: needinfo?(mh+mozilla)
This could be a separate bug, but it's easiest to fix it here, since
removing this assignment enables removing the Makefile.in later.

Untested, but if everything else builds without a custom SHELL
assignment, this module should too.
Attachment #8461587 - Flags: review?(mh+mozilla)
I kept finding other things to make into AC_SUBST_LIST (the next patch) and
thought that the more extensive changes required re-review.
Attachment #8461050 - Attachment is obsolete: true
Attachment #8461588 - Flags: review?(mh+mozilla)
This change tidies up moz.build files by not requiring split()
everywhere.  The main tool was:

find . -name moz.build |xargs perl -p -i -e "s/CFLAGS'\].split\(\)/CFLAGS']/"

with small fixups for included moz.build files that aren't named as such.

The pkg.m4 changes are necessary so that AC_SUBST_LISTs in the main
configure.in agree with whatever PKG_CHECK_MODULES uses.
Attachment #8461589 - Flags: review?(mh+mozilla)
Swapping parts around as requested.
Attachment #8461589 - Attachment is obsolete: true
Attachment #8461589 - Flags: review?(mh+mozilla)
Attachment #8461632 - Flags: review?(mh+mozilla)
Swapping parts around as requested.
Attachment #8461588 - Attachment is obsolete: true
Attachment #8461588 - Flags: review?(mh+mozilla)
Attachment #8461633 - Flags: review?(mh+mozilla)
And a few more for good measure; there's a use of TK_CFLAGS in
ipc/chromium/Makefile.in, but I think that's about it.
Attachment #8461633 - Attachment is obsolete: true
Attachment #8461633 - Flags: review?(mh+mozilla)
Attachment #8461715 - Flags: review?(mh+mozilla)
Comment on attachment 8461587 [details] [diff] [review]
part 0 - remove 'SHELL := ksh' assignment in layout/build/Makefile.in

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

::: layout/build/Makefile.in
@@ -5,5 @@
>  
> -# Solaris sh blows
> -ifeq ($(OS_ARCH),SunOS)
> -SHELL := ksh
> -endif

I guess it mattered when the file looked like this 10 years ago:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/build/Makefile.in&rev=1.109
Attachment #8461587 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8461632 [details] [diff] [review]
part 1 - make most *CFLAGS variables from configure lists in moz.build

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

python/mozbuild/mozbuild/backend/visualstudio.py does split() a bunch of those in consume_finished(), so you need to change that file too.

::: configure.in
@@ +3526,5 @@
>  LDFLAGS=$_SAVE_LDFLAGS
>  LIBS=$_SAVE_LIBS
>  
>  AC_SUBST(MOZ_NATIVE_LIBEVENT)
> +AC_SUBST_LIST(MOZ_LIBEVENT_CFLAGS)

You can remove this entirely.

@@ +4365,5 @@
>  
>      TK_LIBS="$TK_LIBS $MOZ_STARTUP_NOTIFICATION_LIBS"
>  fi
>  AC_SUBST(MOZ_ENABLE_STARTUP_NOTIFICATION)
> +AC_SUBST_LIST(MOZ_STARTUP_NOTIFICATION_CFLAGS)

And this.

@@ +4554,5 @@
>  AC_SUBST(MOZ_ENABLE_QTNETWORK)
>  AC_SUBST(MOZ_ENABLE_QMSYSTEM2)
>  AC_SUBST(MOZ_ENABLE_QTMOBILITY)
>  AC_SUBST(MOZ_ENABLE_XREMOTE)
> +AC_SUBST_LIST(MOZ_GTK2_CFLAGS)

And that.

@@ +4559,2 @@
>  AC_SUBST(MOZ_GTK2_LIBS)
> +AC_SUBST_LIST(MOZ_GTK3_CFLAGS)

And that.

@@ +4647,5 @@
>  then
>      PKG_CHECK_MODULES(_PANGOCHK, pango >= $PANGO_VERSION)
>  
>      PKG_CHECK_MODULES(MOZ_PANGO, pango >= $PANGO_VERSION pangoft2 >= $PANGO_VERSION pangocairo >= $PANGO_VERSION)
> +    AC_SUBST_LIST(MOZ_PANGO_CFLAGS)

And that.

@@ +4689,5 @@
>          ])
>      fi
>  
>      AC_SUBST(MOZ_ENABLE_GNOMEVFS)
> +    AC_SUBST_LIST(MOZ_GNOMEVFS_CFLAGS)

And that.

@@ +4721,5 @@
>          ])
>      fi
>  
>      AC_SUBST(MOZ_ENABLE_GIO)
> +    AC_SUBST_LIST(MOZ_GIO_CFLAGS)

And that.

@@ +4751,5 @@
>          AC_DEFINE(MOZ_ENABLE_GCONF)
>      fi
>  
>      AC_SUBST(MOZ_ENABLE_GCONF)
> +    AC_SUBST_LIST(MOZ_GCONF_CFLAGS)

And that.

@@ +4775,5 @@
>          AC_DEFINE(MOZ_ENABLE_LIBPROXY)
>      fi
>  fi
>  AC_SUBST(MOZ_ENABLE_LIBPROXY)
> +AC_SUBST_LIST(MOZ_LIBPROXY_CFLAGS)

And that.

@@ +4819,5 @@
>      fi
>  fi
>  
>  AC_SUBST(MOZ_ENABLE_GNOMEUI)
> +AC_SUBST_LIST(MOZ_GNOMEUI_CFLAGS)

And that.

@@ +4842,5 @@
>          AC_DEFINE(MOZ_ENABLE_DBUS)
>      fi
>  fi
>  AC_SUBST(MOZ_ENABLE_DBUS)
> +AC_SUBST_LIST(MOZ_DBUS_CFLAGS)

And that.

@@ +4847,2 @@
>  AC_SUBST(MOZ_DBUS_LIBS)
> +AC_SUBST_LIST(MOZ_DBUS_GLIB_CFLAGS)

And that.

@@ +5320,5 @@
>      LIBS=$_SAVE_LIBS
>  fi
>  
>  AC_SUBST(MOZ_NATIVE_LIBVPX)
> +AC_SUBST_LIST(MOZ_LIBVPX_CFLAGS)

And that.

@@ +5476,5 @@
>            AC_MSG_ERROR([Need alsa for Ogg, Wave or WebM decoding on Linux.  Disable with --disable-ogg --disable-wave --disable-webm.  (On Ubuntu, you might try installing the package libasound2-dev.)])])
>  fi
>  
>  AC_SUBST(MOZ_ALSA)
> +AC_SUBST_LIST(MOZ_ALSA_CFLAGS)

And that.

@@ +5510,5 @@
>      fi
>  fi
>  
>  AC_SUBST(MOZ_PULSEAUDIO)
> +AC_SUBST_LIST(MOZ_PULSEAUDIO_CFLAGS)

And that.

@@ +5565,5 @@
>          AC_MSG_ERROR([gstreamer-plugins-base found, but no libgstvideo. Something has gone terribly wrong. Try reinstalling gstreamer-plugins-base; failing that, disable the gstreamer backend with --disable-gstreamer.])
>      fi
>      LDFLAGS=$_SAVE_LDFLAGS
>  
> +    AC_SUBST_LIST(GSTREAMER_CFLAGS)

And that.

@@ +5973,5 @@
>  
>    if test "$OS_TARGET" = "Linux" -o "$OS_ARCH" = "SunOS" && \
>      test -z "$SKIP_LIBRARY_CHECKS"; then
>      PKG_CHECK_MODULES(MOZ_GTHREAD, gthread-2.0)
> +    AC_SUBST_LIST(MOZ_GTHREAD_CFLAGS)

And that.

@@ +7886,5 @@
>  ; then
>      GLIB_GMODULE_LIBS=`$GLIB_CONFIG gmodule --libs`
>  fi
>  
> +AC_SUBST_LIST(GLIB_CFLAGS)

And that.

@@ +8100,5 @@
>      AC_SUBST(MOZ_ENABLE_DWRITE_FONT)
>      AC_SUBST(MOZ_ENABLE_D2D_SURFACE)
>      AC_SUBST(MOZ_ENABLE_D3D9_LAYER)
>      AC_SUBST(MOZ_ENABLE_D3D10_LAYER)
> +    AC_SUBST_LIST(CAIRO_FT_CFLAGS)

And that.

You can remove all I pointed those because only one AC_SUBST/AC_SUBST_LIST/AC_SUBST_SET is enough in configure.in. And the one in PKG_CHECK_MODULES counts.
Attachment #8461632 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8461715 [details] [diff] [review]
part 2 - move MOZ_CAIRO_CFLAGS et al additions to C*FLAGS into moz.build

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

::: browser/components/shell/src/Makefile.in
@@ +6,2 @@
>  clobber::
>  	rm -f $(DIST)/lib/$(LIBRARY_NAME).lib

You can remove this rule. First, because no-one really expects make -C objdir clobber to work (we should actually remove that rule, and make it do the same as mach clobber). Second, because there's no LIBRARY_NAME in this directory.
Thirdly, you've been bitrotted by bug 1038458.

::: gfx/2d/Makefile.in
@@ -2,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -ifeq ($(MOZ_WIDGET_TOOLKIT),$(findstring $(MOZ_WIDGET_TOOLKIT),android gtk2 gtk3 gonk qt))

I love some of the convoluted tests we have.

::: gfx/skia/generate_mozbuild.py
@@ +128,5 @@
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'qt':
> +    CXXFLAGS += CONFIG['MOZ_CAIRO_CFLAGS']
> +    CXXFLAGS += CONFIG['MOZ_PANGO_CFLAGS']
> +    CXXFLAGS += CONFIG['CAIRO_FT_CFLAGS']

You could also do:

if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('android', 'gonk', 'gtk2', 'gtk3', 'qt'):
    CXXFLAGS += CONFIG['MOZ_CAIRO_CFLAGS']
    CXXFLAGS += CONFIG['CAIRO_FT_CFLAGS']

if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('gtk2', 'gtk3', 'qt'):
    CXXFLAGS += CONFIG['MOZ_PANGO_CFLAGS']

::: gfx/thebes/moz.build
@@ +285,5 @@
>      # top of the android java runtime.
>      DEFINES['MOZ_USING_ANDROID_JAVA_WIDGETS'] = True
> +    CXXFLAGS += CONFIG['CAIRO_FT_CFLAGS']
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':

if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('android', 'gonk', 'qt'):

@@ +288,5 @@
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> +    CXXFLAGS += CONFIG['CAIRO_FT_CFLAGS']
> +
> +if CONFIG['MOZ_WIDGET_GTK']:

if CONFIG[MOZ_WIDGET_TOOLKIT'] in ('gtk2', 'gtk3', 'qt'):
Attachment #8461715 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #12)
> python/mozbuild/mozbuild/backend/visualstudio.py does split() a bunch of
> those in consume_finished(), so you need to change that file too.

Ah, thanks for pointing that out, I'll fix that up.

> You can remove all I pointed those because only one
> AC_SUBST/AC_SUBST_LIST/AC_SUBST_SET is enough in configure.in. And the one
> in PKG_CHECK_MODULES counts.

Does the AC_SUBST_LIST from PKG_CHECK_MODULES count even if we never execute the PKG_CHECK_MODULES?  That seems counterintuitive.

If the answer is yes, then I think all of the ones you pointed out are valid to remove.  If the answer is no, then we'd need to be very very careful which ones get removed (CAIRO_FT_CFLAGS and MOZ_PULSEAUDIO_CFLAGS are the ones I noticed, perhaps there are others).
(In reply to Nathan Froyd (:froydnj) from comment #14)
> (In reply to Mike Hommey [:glandium] from comment #12)
> > python/mozbuild/mozbuild/backend/visualstudio.py does split() a bunch of
> > those in consume_finished(), so you need to change that file too.
> 
> Ah, thanks for pointing that out, I'll fix that up.

None of the variables that visualstudio.py uses got the AC_SUBST_LIST treatment, so none of them need to be fixed.  I can try to fix those up too if you like.
(In reply to Nathan Froyd (:froydnj) from comment #14)
> (In reply to Mike Hommey [:glandium] from comment #12)
> > python/mozbuild/mozbuild/backend/visualstudio.py does split() a bunch of
> > those in consume_finished(), so you need to change that file too.
> 
> Ah, thanks for pointing that out, I'll fix that up.
> 
> > You can remove all I pointed those because only one
> > AC_SUBST/AC_SUBST_LIST/AC_SUBST_SET is enough in configure.in. And the one
> > in PKG_CHECK_MODULES counts.
> 
> Does the AC_SUBST_LIST from PKG_CHECK_MODULES count even if we never execute
> the PKG_CHECK_MODULES?  That seems counterintuitive.

Yes. AC_SUBST* are m4 macros and don't expand inline (contrary to AC_DEFINE)

(In reply to Nathan Froyd (:froydnj) from comment #15)
> (In reply to Nathan Froyd (:froydnj) from comment #14)
> > (In reply to Mike Hommey [:glandium] from comment #12)
> > > python/mozbuild/mozbuild/backend/visualstudio.py does split() a bunch of
> > > those in consume_finished(), so you need to change that file too.
> > 
> > Ah, thanks for pointing that out, I'll fix that up.
> 
> None of the variables that visualstudio.py uses got the AC_SUBST_LIST
> treatment, so none of them need to be fixed.  I can try to fix those up too
> if you like.

Ah, it so happens that I had a match because my grep was not limited enough. It matched on PIXMAN_CFLAGS (which is affected) because it contains MOZ_PIXMAN_CFLAGS.
Updated with review comments.  Builds do appear to work, so I learned something
about autoconf...which I'll promptly forget until the next time I learn it.
Attachment #8461632 - Attachment is obsolete: true
Attachment #8464973 - Flags: review?(mh+mozilla)
Comment on attachment 8464973 [details] [diff] [review]
part 1 - make most *CFLAGS variables from configure lists in moz.build

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

::: configure.in
@@ -7847,1 @@
>  AC_SUBST(GLIB_LIBS)

As a followup, you can remove the corresponding *_LIBS AC_SUBSTs for the same reason.
Attachment #8464973 - Flags: review?(mh+mozilla) → review+
Assignee: nobody → nfroyd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1052943
Depends on: 1054154
QA Whiteboard: [qa-]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.