751521 - --enable-system-pixman is broken after Bug 751273 fixed

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[ojab]

Attachment: my mozconfig

mozilla-inbound is broken for me since 
changeset:   92892:025285f57616
parent:      92888:5900fe7cd355
parent:      92891:d4e35005f5a9
user:        Ehsan Akhgari <[e-mail]>
date:        Wed May 02 21:52:33 2012 -0400
summary:     Merge mozilla-central into mozilla-inbound on a CLOSED TREE

which fixes Bug 750661 and dependencies. Also reproducible with hg tip (-r92982:d7271f499b8b) The actual error is:

make[7]: Leaving directory `/home/ojab/opt2/xulrunner/layout/build'
make -C media export
make[7]: Entering directory `/home/ojab/opt2/xulrunner/layout/media'
/sources/mozilla-inbound/config/rules.mk:745: *** SHARED_LIBRARY_LIBS must contain .a files only.  Stop.
make[7]: Leaving directory `/home/ojab/opt2/xulrunner/layout/media'
make[6]: *** [export] Error 2
make[6]: Leaving directory `/home/ojab/opt2/xulrunner/layout'
make[5]: *** [export_tier_platform] Error 2
make[5]: Leaving directory `/home/ojab/opt2/xulrunner'
make[4]: *** [tier_platform] Error 2

And SHARED_LIBRARY_LIBS expands to:
/sources/mozilla-inbound/config/rules.mk:745: *** .../../gfx/ots/src/libmozots.a ../../gfx/qcms/libmozqcms.a ../../gfx/cairo/cairo/src/libmozcairo.a  -lpixman-1   ../../gfx/graphite2/src/libmozgraphite2.a ../../media/libvorbis/lib/libvorbis.a ../../media/libogg/src/libogg.a  ../../media/libtheora/lib/libtheora.a  ../../media/libopus/libopus.a  ../../media/libnestegg/src/libnestegg.a  ../../media/libsydneyaudio/src/libsydneyaudio.a  ../../media/libcubeb/src/libcubeb.a  ../../gfx/angle/libangle.a ../../parser/expat/lib/libmozexpat_s.a  must contain .a files only.  Stop.

Comment 1

[Takanori MATSUURA]

MOZ_CAIRO_LIBS generated by configure script contains both cairo and pixman libraries:
https://hg.mozilla.org/mozilla-central/annotate/e6228decd18e/configure.in#l8146

This may include "-lpixman-1" in the MOZ_CAIRO_LIBS. And it causes error.

Comment 2

[Takanori MATSUURA]

Attachment: Separate pixman detection from cairo (doesn't work)

This works with --enable-system-pixman but not without it.

We cannot test the combination of --enable-system-cairo because of the bug 722975.

Any advice or updated patch is welcome.

Comment 3

[Andrew Benton]

Attachment: add pixman and xrender to the MOZ_GTK2 flags

> Created attachment 621586 [details] [diff] [review]
> Separate pixman detection from cairo (doesn't work)
> 
> This works with --enable-system-pixman but not without it.
> 
> We cannot test the combination of --enable-system-cairo because of the bug
> 722975.
> 
> Any advice or updated patch is welcome.

You missed a bit. With this patch and the above patch from Takanori MATSUURA (many thanks for that!) I was able to build Firefox with --enable-system-cairo. (I have patched my system cairo with the expose-snapshot patch)

Comment 4

[Andrew Benton]

Attachment: combined patch

Comment 5

[Andrew Benton]

Attachment: add pixman and xrender to the MOZ_GTK2 flags

Doh! typo

Comment 6

[Andrew Benton]

Attachment: combined patch

Doh! typo is in the combined patch too

Comment 7

[Andrew Benton]

Attachment: Rediffed the patch against the current trunk

layout/media/Makefile.in and layout/media/symbols.def.in have changed so the previous patch didn't apply cleanly

Comment 8

[Takanori MATSUURA]

Attachment: Separate pixman detection from cairo v1

(In reply to Andrew Benton from comment #7)
> Created attachment 622349 [details] [diff] [review]
> Rediffed the patch against the current trunk

Thanks, Andrew.
I found that _moz_pixman* is defined only when both MOZ_TREE_CAIRO and MOZ_TREE_PIXMAN are defined. So I've updated a patch with a bit modified.

Comment 9

[Takanori MATSUURA]

Attachment: Separate pixman detection from cairo v2

This patch now works both with and without --enable-system-pixman.
I suppose attachment 621815 [details] [diff] [review] is not required now.

Before checking this in, we may need try build.
(However I don't have an account.)

Comment 10

[(not currently active) Ted Mielczarek]

If you'd like to get access to the try server, file a bug and CC me and I will vouch for you:
http://www.mozilla.org/hacking/commit-access-policy/

Comment 11

[Takanori MATSUURA]

Attachment: Separate pixman detection from cairo v3

Update to fit to the latest m-c.

Comment 12

[Takanori MATSUURA]

There are two possible solutions:

1. Use os.mkdirs instead of os.mkdir to generate cachedir
xpcom/idl-parser/header.py:
     if options.cachedir is not None:
         if not os.path.isdir(options.cachedir):
             os.mkdir(options.cachedir)
         sys.path.append(options.cachedir)

2. "make export" at xpcom/idl-parser before parsing idl files
xpcom/idl-parser/Makefile.in:
# Generate the PLY lexer and parser.
export:: $(PARSER_SRCS) $(PLY_PROGS)
	$(PYTHON_PATH) \
	  $(PLY_INCLUDE) \
	  -I$(topsrcdir)/xpcom/idl-parser \
	  $(topsrcdir)/xpcom/idl-parser/header.py --cachedir=$(DEPTH)/xpcom/idl-parser/cache --regen

Comment 13

[Takanori MATSUURA]

Please ignore comment 12.
I posted a wrong comment for another bug. :-(

Comment 14

[Martin Stránský [:stransky]]

Comment on attachment 623495 [details] [diff] [review]
Separate pixman detection from cairo v3

The patch does not build with latest trunk & --enabled-system-cairo & --enabled-system-pixman. libxul.so link fails because of missing libXrender.so library:

../../widget/gtk2/nsWindow.o: In function `nsWindow::GetSurfaceForGdkDrawable(_GdkDrawable*, nsIntSize const&)':
/home/komat/tmp646-trunk/src/widget/gtk2/nsWindow.cpp:6000: undefined reference to `XRenderFindStandardFormat'
/home/komat/tmp646-trunk/src/widget/gtk2/nsWindow.cpp:6003: undefined reference to `XRenderFindStandardFormat'
../../dom/plugins/ipc/PluginInstanceChild.o: In function `mozilla::plugins::PluginInstanceChild::CreateOptSurface()':
/home/komat/tmp646-trunk/src/dom/plugins/ipc/PluginInstanceChild.cpp:2708: undefined reference to `XRenderFindStandardFormat'
../../gfx/thebes/gfxXlibSurface.o: In function `gfxXlibSurface::CreateSimilarSurface(gfxASurface::gfxContentType, nsIntSize const&)':
/home/komat/tmp646-trunk/src/gfx/thebes/gfxXlibSurface.cpp:229: undefined reference to `XRenderFindStandardFormat'
../../gfx/thebes/gfxXlibSurface.o: In function `DisplayTable::GetColormapAndVisual(Screen*, XRenderPictFormat*, Visual*, unsigned long*, Visual**)':
/home/komat/tmp646-trunk/src/gfx/thebes/gfxXlibSurface.cpp:345: undefined reference to `XRenderFindVisualFormat'
../../gfx/thebes/gfxXlibSurface.o: In function `gfxXlibSurface::FindRenderFormat(_XDisplay*, gfxASurface::gfxImageFormat)':
/home/komat/tmp646-trunk/src/gfx/thebes/gfxXlibSurface.cpp:507: undefined reference to `XRenderFindStandardFormat'
/home/komat/tmp646-trunk/src/gfx/thebes/gfxXlibSurface.cpp:509: undefined reference to `XRenderFindStandardFormat'
/home/komat/tmp646-trunk/src/gfx/thebes/gfxXlibSurface.cpp:517: undefined reference to `XRenderFindVisualFormat'
/home/komat/tmp646-trunk/src/gfx/thebes/gfxXlibSurface.cpp:520: undefined reference to `XRenderFindStandardFormat'
/home/komat/tmp646-trunk/src/gfx/thebes/gfxXlibSurface.cpp:522: undefined reference to `XRenderFindStandardFormat'
../../gfx/thebes/gfxXlibNativeRenderer.o: In function `gfxXlibNativeRenderer::DrawDirect(gfxContext*, nsIntSize, unsigned int, Screen*, Visual*)':
/home/komat/tmp646-trunk/src/gfx/thebes/gfxXlibNativeRenderer.cpp:258: undefined reference to `XRenderFindVisualFormat'
../../gfx/thebes/gfxXlibNativeRenderer.o: In function `FormatConversionIsExact':
/home/komat/tmp646-trunk/src/gfx/thebes/gfxXlibNativeRenderer.cpp:301: undefined reference to `XRenderFindVisualFormat'
../../gfx/thebes/gfxXlibNativeRenderer.o: In function `CreateTempXlibSurface':
/home/komat/tmp646-trunk/src/gfx/thebes/gfxXlibNativeRenderer.cpp:370: undefined reference to `XRenderFindVisualFormat'
../../gfx/layers/ShadowLayerUtilsX11.o: In function `mozilla::layers::GetXRenderPictFormatFromId(_XDisplay*, unsigned long)':
/home/komat/tmp646-trunk/src/gfx/layers/ipc/ShadowLayerUtilsX11.cpp:69: undefined reference to `XRenderFindFormat'
collect2: ld returned 1 exit status

Comment 15

[Andrew Benton]

(In reply to Martin Stránský from comment #14)
> Comment on attachment 623495 [details] [diff] [review]
> Separate pixman detection from cairo v3
> 
> The patch does not build with latest trunk & --enabled-system-cairo &
> --enabled-system-pixman. libxul.so link fails because of missing
> libXrender.so library:

Try the patch I posted in comment #5

Comment 16

[Martin Stránský [:stransky]]

I believe the proper way is to fix the MOZ_CAIRO_OSLIBS which is empty now but it's still used for libxul link. It may be something like:

diff --git a/toolkit/library/Makefile.in b/toolkit/library/Makefile.in
--- a/toolkit/library/Makefile.in
+++ b/toolkit/library/Makefile.in
@@ -339,5 +339,5 @@ EXTRA_DSO_LDOPTS += \
   $(MOZ_JS_LIBS) \
   $(NSS_LIBS) \
-  $(MOZ_CAIRO_OSLIBS) \
+  $(MOZ_CAIRO_LIBS) \
   $(MOZ_APP_EXTRA_LIBS) \
   $(SQLITE_LIBS) \

or so.

Comment 17

[Martin Stránský [:stransky]]

Another problem is when --enable-system-pixman is not set and --enable-system-cairo is set, SHARED_LIBRARY_LIBS contains "@MOZ_PIXMAN_LIBS@" string.

Comment 18

[Takanori MATSUURA]

Attachment: Separate pixman detection from cairo v4

Tested on Fedora 16:
* set --enable-system-pixman and unset --enable-system-cairo
* unset --enable-system-pixman and unset --enable-system-cairo

Not tested:
* set --enable-system-pixman and set --enable-system-cairo

Expected to fail:
* unset --enable-system-pixman and set --enable-system-cairo
This will be failed with the message of "--enable-system-cairo should be set with --enable-system-pixman." because I believe system cairo is built with system libpixman.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Takanori MATSUURA from comment #18)
> Created attachment 626390 [details] [diff] [review]
> Separate pixman detection from cairo v4
> 
> Tested on Fedora 16:
> * set --enable-system-pixman and unset --enable-system-cairo
> * unset --enable-system-pixman and unset --enable-system-cairo
> 
> Not tested:
> * set --enable-system-pixman and set --enable-system-cairo
> 
> Expected to fail:
> * unset --enable-system-pixman and set --enable-system-cairo
> This will be failed with the message of "--enable-system-cairo should be set
> with --enable-system-pixman." because I believe system cairo is built with
> system libpixman.

AFAIK, we don't use pixman directly, only cairo does. So --enable-system-cairo should just imply --enable-system-pixman (which it somehow does, currently)

Comment 20

[Takanori MATSUURA]

Attachment: Separate pixman detection from cairo v5

(In reply to Mike Hommey [:glandium] from comment #19)
> AFAIK, we don't use pixman directly, only cairo does. So
> --enable-system-cairo should just imply --enable-system-pixman (which it
> somehow does, currently)

Thanks.

Updated patch now implies --enable-system-pixman when --enable-system-cairo is set.

TBPL for this patch:
https://tbpl.mozilla.org/?tree=Try&rev=34699c1d6c21

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 626486 [details] [diff] [review]
Separate pixman detection from cairo v5

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

::: configure.in
@@ +8037,5 @@
>  MOZ_TREE_PIXMAN=1 )
>  
> +# System cairo depends on system libpixman
> +if test -z "$MOZ_TREE_CAIRO"; then
> +    MOZ_TREE_PIXMAN=

Please throw an error if --enable-system-cairo is given with --disable-system-pixman

@@ +8148,2 @@
>      if test "$MOZ_X11"; then
> +        MOZ_CAIRO_OSLIBS="$MOZ_CAIRO_OSLIBS $XLDFLAGS $XRENDER_LIBS $FT2_LIBS $FONTCONFIG_LIBS"

This is very likely to break building with in-tree freetype (which android builds do)

Comment 22

[Takanori MATSUURA]

Attachment: Separate pixman detection from cairo v6

Try: https://tbpl.mozilla.org/?tree=Try&rev=a7a0e98914f3

(In reply to Mike Hommey [:glandium] from comment #21)
> ::: configure.in
> @@ +8037,5 @@
> >  MOZ_TREE_PIXMAN=1 )
> >  
> > +# System cairo depends on system libpixman
> > +if test -z "$MOZ_TREE_CAIRO"; then
> > +    MOZ_TREE_PIXMAN=
> 
> Please throw an error if --enable-system-cairo is given with
> --disable-system-pixman

Fixed. I'd like someone to test this because I don't have system cairo ready for building Firefox from m-c.


> @@ +8148,2 @@
> >      if test "$MOZ_X11"; then
> > +        MOZ_CAIRO_OSLIBS="$MOZ_CAIRO_OSLIBS $XLDFLAGS $XRENDER_LIBS $FT2_LIBS $FONTCONFIG_LIBS"
> 
> This is very likely to break building with in-tree freetype (which android
> builds do)

I suppose MOZ_X11 is not set on Android. I don't get the build error in this area on Android with the previous patch.

Comment 23

[Mike Hommey [:glandium]]

(In reply to Takanori MATSUURA from comment #22)
> > This is very likely to break building with in-tree freetype (which android
> > builds do)
> 
> I suppose MOZ_X11 is not set on Android. I don't get the build error in this
> area on Android with the previous patch.

The option still exists to create such builds for X.

Comment 24

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #23)
> (In reply to Takanori MATSUURA from comment #22)
> > > This is very likely to break building with in-tree freetype (which android
> > > builds do)
> > 
> > I suppose MOZ_X11 is not set on Android. I don't get the build error in this
> > area on Android with the previous patch.
> 
> The option still exists to create such builds for X.

(Note that I don't know if such builds work already. If they don't, then it's fine to error out, then. If they do, it would be better to preserve the functionality)

Comment 25

[Takanori MATSUURA]

Attachment: Separate pixman detection from cairo v7

(In reply to Mike Hommey [:glandium] from comment #24)
> (In reply to Mike Hommey [:glandium] from comment #23)
> > The option still exists to create such builds for X.
> 
> (Note that I don't know if such builds work already. If they don't, then
> it's fine to error out, then. If they do, it would be better to preserve the
> functionality)

I remove this part from the patch because it is not critical to fix this bug.

Try: https://tbpl.mozilla.org/?tree=Try&rev=22dec7021309


I'm not sure why libraries are hard-coded here. But the issue should be discussed as another bug.

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 627007 [details] [diff] [review]
Separate pixman detection from cairo v7

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

::: configure.in
@@ +8029,5 @@
>  
> +# System cairo depends on system pixman
> +if test "${MOZ_TREE_PIXMAN}" = "force"; then
> +    if test -z "${MOZ_TREE_CAIRO}"; then
> +        AC_MSG_ERROR([System cairo should be used with system pixman.])

Nit: This is kind of a confusing message. It sounds as if you require --enable-system-pixman when you use --enable-system-cairo, when it's actually about the opposite. Reverting is not going to make things much clearer. I'd go for "--disable-system-pixman is incompatible with --enable-system-cairo"

Comment 27

[Takanori MATSUURA]

Attachment: Separate pixman detection from cairo v7.1

Change only one message string addressed in comment #26.

Carry over r+.

Comment 28

[Mike Hommey [:glandium]]

Unfortunately, building with --enable-system-cairo fails with:
Exception: File not found: ../../gfx/cairo/libpixman/src/libmozlibpixman.a
when building libgkmedias.a.desc.

Comment 29

[Mike Hommey [:glandium]]

This hunk from v5 is needed before "if test "$MOZ_TREE_PIXMAN"; then":

# System cairo depends on system libpixman
if test -z "$MOZ_TREE_CAIRO"; then
    MOZ_TREE_PIXMAN=
fi

Comment 30

[Mike Hommey [:glandium]]

Landed a fixed up version.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a64fac52f708

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a64fac52f708