Last Comment Bug 751521 - --enable-system-pixman is broken after Bug 751273 fixed
: --enable-system-pixman is broken after Bug 751273 fixed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Other Branch
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Takanori MATSUURA
:
Mentors:
Depends on:
Blocks: 751273
  Show dependency treegraph
 
Reported: 2012-05-03 03:59 PDT by ojab
Modified: 2012-05-26 05:35 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
my mozconfig (924 bytes, text/plain)
2012-05-03 03:59 PDT, ojab
no flags Details
Separate pixman detection from cairo (doesn't work) (19.76 KB, patch)
2012-05-07 07:15 PDT, Takanori MATSUURA
no flags Details | Diff | Splinter Review
add pixman and xrender to the MOZ_GTK2 flags (472 bytes, patch)
2012-05-07 17:27 PDT, Andrew Benton
no flags Details | Diff | Splinter Review
combined patch (15.37 KB, patch)
2012-05-07 17:43 PDT, Andrew Benton
no flags Details | Diff | Splinter Review
add pixman and xrender to the MOZ_GTK2 flags (472 bytes, patch)
2012-05-07 17:51 PDT, Andrew Benton
no flags Details | Diff | Splinter Review
combined patch (15.43 KB, patch)
2012-05-07 17:52 PDT, Andrew Benton
no flags Details | Diff | Splinter Review
Rediffed the patch against the current trunk (14.09 KB, patch)
2012-05-09 05:27 PDT, Andrew Benton
no flags Details | Diff | Splinter Review
Separate pixman detection from cairo v1 (19.70 KB, patch)
2012-05-09 06:12 PDT, Takanori MATSUURA
no flags Details | Diff | Splinter Review
Separate pixman detection from cairo v2 (20.50 KB, patch)
2012-05-09 09:30 PDT, Takanori MATSUURA
no flags Details | Diff | Splinter Review
Separate pixman detection from cairo v3 (20.46 KB, patch)
2012-05-13 05:55 PDT, Takanori MATSUURA
no flags Details | Diff | Splinter Review
Separate pixman detection from cairo v4 (22.08 KB, patch)
2012-05-23 04:27 PDT, Takanori MATSUURA
no flags Details | Diff | Splinter Review
Separate pixman detection from cairo v5 (21.86 KB, patch)
2012-05-23 10:00 PDT, Takanori MATSUURA
mh+mozilla: review-
Details | Diff | Splinter Review
Separate pixman detection from cairo v6 (22.00 KB, patch)
2012-05-24 05:49 PDT, Takanori MATSUURA
no flags Details | Diff | Splinter Review
Separate pixman detection from cairo v7 (20.78 KB, patch)
2012-05-24 16:21 PDT, Takanori MATSUURA
mh+mozilla: review+
Details | Diff | Splinter Review
Separate pixman detection from cairo v7.1 (20.80 KB, patch)
2012-05-25 04:47 PDT, Takanori MATSUURA
t.matsuu: review+
Details | Diff | Splinter Review

Description ojab 2012-05-03 03:59:20 PDT
Created attachment 620643 [details]
my mozconfig

mozilla-inbound is broken for me since 
changeset:   92892:025285f57616
parent:      92888:5900fe7cd355
parent:      92891:d4e35005f5a9
user:        Ehsan Akhgari <ehsan@mozilla.com>
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 2012-05-06 08:48:02 PDT
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 2012-05-07 07:15:27 PDT
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.
Comment 3 Andrew Benton 2012-05-07 17:27:29 PDT
Created attachment 621800 [details] [diff] [review]
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 2012-05-07 17:43:51 PDT
Created attachment 621808 [details] [diff] [review]
combined patch
Comment 5 Andrew Benton 2012-05-07 17:51:03 PDT
Created attachment 621815 [details] [diff] [review]
add pixman and xrender to the MOZ_GTK2 flags

Doh! typo
Comment 6 Andrew Benton 2012-05-07 17:52:04 PDT
Created attachment 621818 [details] [diff] [review]
combined patch

Doh! typo is in the combined patch too
Comment 7 Andrew Benton 2012-05-09 05:27:06 PDT
Created attachment 622349 [details] [diff] [review]
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 2012-05-09 06:12:34 PDT
Created attachment 622358 [details] [diff] [review]
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 2012-05-09 09:30:52 PDT
Created attachment 622395 [details] [diff] [review]
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 Ted Mielczarek [:ted.mielczarek] 2012-05-09 09:35:31 PDT
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 2012-05-13 05:55:31 PDT
Created attachment 623495 [details] [diff] [review]
Separate pixman detection from cairo v3

Update to fit to the latest m-c.
Comment 12 Takanori MATSUURA 2012-05-13 16:24:50 PDT
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 2012-05-13 16:27:39 PDT
Please ignore comment 12.
I posted a wrong comment for another bug. :-(
Comment 14 Martin Stránský 2012-05-17 03:10:53 PDT
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 2012-05-17 05:05:03 PDT
(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ý 2012-05-17 05:13:36 PDT
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ý 2012-05-17 05:38:37 PDT
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 2012-05-23 04:27:45 PDT
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.
Comment 19 Mike Hommey [:glandium] 2012-05-23 04:44:11 PDT
(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 2012-05-23 10:00:16 PDT
Created attachment 626486 [details] [diff] [review]
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] 2012-05-24 02:54:16 PDT
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 2012-05-24 05:49:18 PDT
Created attachment 626766 [details] [diff] [review]
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] 2012-05-24 06:07:02 PDT
(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] 2012-05-24 06:08:00 PDT
(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 2012-05-24 16:21:57 PDT
Created attachment 627007 [details] [diff] [review]
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] 2012-05-24 23:03:37 PDT
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 2012-05-25 04:47:49 PDT
Created attachment 627182 [details] [diff] [review]
Separate pixman detection from cairo v7.1

Change only one message string addressed in comment #26.

Carry over r+.
Comment 28 Mike Hommey [:glandium] 2012-05-25 13:23:59 PDT
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] 2012-05-25 13:32:24 PDT
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] 2012-05-25 23:46:06 PDT
Landed a fixed up version.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a64fac52f708
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-05-26 05:35:20 PDT
https://hg.mozilla.org/mozilla-central/rev/a64fac52f708

Note You need to log in before you can comment on or make changes to this bug.