281950 - Landing of cairo/libpixman

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[tor]

Both SVG and <canvas> need cairo, which we need to ship as it's not yet
a standard part of linux distributions and will not be on platforms.  This
patch defaults to the in-tree cairo, allowing a system cairo to be used
with --enable-system-cairo.

The patch uses unmodified libpixman and cairo source trees located in the
directories gfx/cairo/{libpixman,cairo}.  It works, but I could use input
from autoconf and mozilla build system experts about the appropriate ways
of doing the things I'm attempting.

Comment 1

[tor]

Attachment: cairo landing patch

Comment 2

[Benjamin Smedberg]

Comment on attachment 174053 [details] [diff] [review]
cairo landing patch

Chris, bounce this back to me if you don't have time.

Comment 3

[Benjamin Smedberg]

Comment on attachment 174053 [details] [diff] [review]
cairo landing patch

>Index: configure.in

>+MOZ_CAIRO_LIBS=-"lcairo -lpixman"

Ick, try "-lcairo -lpixman" ;)

>+   [ --enable-system-cairo Use system cairo (located with pkgconfig)],

Would it make sense to support an explicit path in addition to pkgconfig
(--with-system-cairo=/usr/lib/cairo) or do we need pkgconfig to pick up the
correct CFLAGS?

>Index: Makefile.in

> clean clobber realclean clobber_all distclean::
[snip]
>+ifndef MOZ_SYSTEM_CAIRO
>+	$(MAKE) -C gfx/cairo/libpixman $@
>+	$(MAKE) -C gfx/cairo/cairo $@
>+endif

Do you care that the realclean and clobber_all target probably aren't
supported? I think that tinderboxen may still use them.

>+cairo:
>+ifndef MOZ_SYSTEM_CAIRO
>+	$(MAKE) -C gfx/cairo/libpixman install
>+	$(MAKE) -C gfx/cairo/cairo install

I see the "make install" but I don't ever see the "make"... marking r- for
this, re-request r? if there is something I'm missing.

>+	mkdir -p $(DIST)/bin

This shouldn't be needed, but if it is, don't use "-p".

>+ifneq ($(OS_ARCH),Darwin)
>+	-cp $(DIST)/lib/libpixman.so.1 $(DIST)/bin
>+	-cp $(DIST)/lib/libcairo.so.1 $(DIST)/bin
>+else
>+	-cp $(DIST)/lib/libpixman.1.dylib $(DIST)/bin
>+	-cp $(DIST)/lib/libcairo.1.dylib $(DIST)/bin
>+endif
>+endif

Why "cp" instead of $(INSTALL) ?... we've already built nsinstall at this
point, and it will save a lot of dependency overhead later in depend builds.

Comment 4

[tor]

(In reply to comment #3)
> >+MOZ_CAIRO_LIBS=-"lcairo -lpixman"
> 
> Ick, try "-lcairo -lpixman" ;)

Heh, a typo that actually worked.

> >+   [ --enable-system-cairo Use system cairo (located with pkgconfig)],
> 
> Would it make sense to support an explicit path in addition to pkgconfig
> (--with-system-cairo=/usr/lib/cairo) or do we need pkgconfig to pick up the
> correct CFLAGS?

The cairo header files don't seem to carry a version number, so the only way
to check if the version is new enough is what pkgconfig tells us.

> > clean clobber realclean clobber_all distclean::
> [snip]
> >+ifndef MOZ_SYSTEM_CAIRO
> >+	$(MAKE) -C gfx/cairo/libpixman $@
> >+	$(MAKE) -C gfx/cairo/cairo $@
> >+endif
> 
> Do you care that the realclean and clobber_all target probably aren't
> supported? I think that tinderboxen may still use them.

I split the targets that can be passed through and those that need to
be mapped - not sure that's the most elegant way of doing things.

> >+cairo:
> >+ifndef MOZ_SYSTEM_CAIRO
> >+	$(MAKE) -C gfx/cairo/libpixman install
> >+	$(MAKE) -C gfx/cairo/cairo install
> 
> I see the "make install" but I don't ever see the "make"... marking r- for
> this, re-request r? if there is something I'm missing.

The rules for the install target of libpixman and cairo cause them to
build the targets - calling make twice on each directory seemed redundant.

> >+	mkdir -p $(DIST)/bin
> 
> This shouldn't be needed, but if it is, don't use "-p".

Leftover from development - removed.

> >+ifneq ($(OS_ARCH),Darwin)
> >+	-cp $(DIST)/lib/libpixman.so.1 $(DIST)/bin
> >+	-cp $(DIST)/lib/libcairo.so.1 $(DIST)/bin
> >+else
> >+	-cp $(DIST)/lib/libpixman.1.dylib $(DIST)/bin
> >+	-cp $(DIST)/lib/libcairo.1.dylib $(DIST)/bin
> >+endif
> >+endif
> 
> Why "cp" instead of $(INSTALL) ?... we've already built nsinstall at this
> point, and it will save a lot of dependency overhead later in depend builds.

No reason - changed.

Comment 5

[tor]

Attachment: update, add packaging

Comment 6

[tor]

Landed.