Closed Bug 751521 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox Build System :: General, defect)

Other Branch
x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla15

People

(Reporter: ojab, Assigned: t.matsuu)

References

Details

Attachments

(3 files, 12 obsolete files)

924 bytes, text/plain
Details
472 bytes, patch
Details | Diff | Splinter Review
20.80 KB, patch
t.matsuu
: review+
Details | Diff | Splinter Review
Attached file 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.
Blocks: 751273
Summary: Build failure with `SHARED_LIBRARY_LIBS must contain .a files only` → --enable-system-pixman is broken after Bug 751273 fixed
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.
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.
> 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)
Attached patch combined patch (obsolete) — Splinter Review
Doh! typo
Attachment #621800 - Attachment is obsolete: true
Attached patch combined patch (obsolete) — Splinter Review
Doh! typo is in the combined patch too
Attachment #621808 - Attachment is obsolete: true
layout/media/Makefile.in and layout/media/symbols.def.in have changed so the previous patch didn't apply cleanly
Attachment #621818 - Attachment is obsolete: true
(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.
Attachment #622349 - Attachment is obsolete: true
Attachment #621586 - Attachment is obsolete: true
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.)
Attachment #622358 - Attachment is obsolete: true
Attachment #622395 - Flags: review?(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/
Update to fit to the latest m-c.
Attachment #622395 - Attachment is obsolete: true
Attachment #622395 - Flags: review?(ted.mielczarek)
Attachment #623495 - Flags: review?(ted.mielczarek)
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
Please ignore comment 12.
I posted a wrong comment for another bug. :-(
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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
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.
Another problem is when --enable-system-pixman is not set and --enable-system-cairo is set, SHARED_LIBRARY_LIBS contains "@MOZ_PIXMAN_LIBS@" string.
Attachment #623495 - Flags: review?(ted.mielczarek)
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.
Attachment #623495 - Attachment is obsolete: true
(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)
(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
Attachment #626390 - Attachment is obsolete: true
Attachment #626486 - Flags: review?(mh+mozilla)
Attachment #626486 - Flags: feedback?(stransky)
Attachment #626486 - Flags: feedback?(b3nton)
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)
Attachment #626486 - Flags: review?(mh+mozilla) → review-
Attachment #626486 - Flags: feedback?(stransky)
Attachment #626486 - Flags: feedback?(b3nton)
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.
Attachment #626486 - Attachment is obsolete: true
(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.
(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)
(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.
Attachment #626766 - Attachment is obsolete: true
Attachment #627007 - Flags: review?(mh+mozilla)
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"
Attachment #627007 - Flags: review?(mh+mozilla) → review+
Assignee: nobody → t.matsuu
Change only one message string addressed in comment #26.

Carry over r+.
Attachment #627007 - Attachment is obsolete: true
Attachment #627182 - Flags: review+
Keywords: checkin-needed
Status: NEW → ASSIGNED
Unfortunately, building with --enable-system-cairo fails with:
Exception: File not found: ../../gfx/cairo/libpixman/src/libmozlibpixman.a
when building libgkmedias.a.desc.
Keywords: checkin-needed
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
Landed a fixed up version.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a64fac52f708
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/a64fac52f708
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.