358934 - additional cairo headers need to be in config/system-headers

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

When building the 1.8 branch on FC6 x86_64, I hit linker errors due to cairo headers not being in config/system-headers (which is used for wrapping system headers to push/pop visibility).  In particular, I think my errors where in nsCanvasRenderingContext2D.cpp and nsSVGCairoCanvas.cpp, and I needed to add (at most) cairo-xlib.h and cairo-xlib-xrender.h .

However, I think we may want to add more than those.  Based on LXR, I see the following includes from outside of cairo into cairo:

cairo-atsui.h
cairo-beos.h
cairo-ft.h
cairo-glitz.h
cairo-nquartz.h
cairo-os2.h
cairo-pdf.h
cairo-ps.h
cairo-quartz.h
cairo-win32.h
cairo-xlib.h
cairo-xlib-xrender.h

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

Comment 2

[Benjamin Smedberg]

Is this --with-system-cairo or something? The cairo headers are internal to our tree by default.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Even though they're internal to our tree, they're not annotated with __attribute__((visibility("default"))) for public APIs.  (Same reason NSS and NSPR headers are in config/system-headers.)

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(And we already build cairo with VISIBILITY_FLAGS= because of that.)

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

ifndef MOZ_ENABLE_LIBXUL, though.  So perhaps this change, plus the existing listing of cairo.h, should be #if !defined(MOZ_ENABLE_LIBXUL) || defined(system cairo))

Comment 6

[Benjamin Smedberg]

Yes, I think so... not sure how to do that easily :-( perhaps piping it through the Xul preprocessor would work.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Well, it's something we need to figure out for bug 359000 and bug 359001 as well...

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(That said, are we sure that the cairo API shouldn't be exported from libxul?)

Comment 9

[Benjamin Smedberg]

vlad and I have been arguing about that, but it's a separate issue and has its own bugs already I think. He wants to (in addition to exporting thebes, which makes me even more uncomfortable), but I really want to avoid more ELF exposure than necessary and Darin convinced me a long time ago that it would be very valuable to avoid exposing any nonfrozen symbols from our shared libs.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Checked in to trunk.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 244509 [details] [diff] [review]
patch

This is a low risk patch that will, along with the other dependencies of bug 359275:

* allow Linux distros to take advantage of improved load and runtime performance with WRAP_SYSTEM_HEADERS, other performance optimizations
(prelinking), and security improvements (SELinux)

* make it easier for developers to build on new distributions like Fedora Core
6

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 244509 [details] [diff] [review]
patch

approved for 1.8/1.8.0 branches, a=dveditz

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.

Comment 15

[Jay Patel [:jay]]

v.fixed on both branches by code and cvs log inspection, and from the lack of any comments regarding new issues or regressions.

If dbaron or anyone else can confirm, that will be great!