351561 - Fix thebes/cairo linking issues

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Thebes has generic functionality that's not really tied in to the rest of the platform, and this can be useful for binary components that want to deal with any image data.  This forces thebes to always be a shared library even in static builds.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: force thebes to build shared

Force thebes to be shared even in static builds, to make the thebes API available for extension use.

Comment 2

[Benjamin Smedberg]

My concerns as listed in the newsgroup thread are still valid; most of all, the fact that using the thebes API requires importing cairo symbols, which vary by system/nonsystem cairo.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm going to morph this bug for a bit, because we need to figure out what we're doing with thebes.  Right now, we have the following in gfxTypes.h:

#if defined(MOZ_ENABLE_LIBXUL)
#define THEBES_API
#elif defined(IMPL_THEBES)
#define THEBES_API NS_EXPORT
#else
#define THEBES_API NS_IMPORT
#endif

IMPL_THEBES is defined when building thebes only.  But when we're doing a static build, we end up building the thebes symbols with NS_EXPORT, but when the thebes includes are #included from from gfx/src/thebes/*, they end up being imported with NS_IMPORT.  This is what's causing the crud mentioned in bug 350380, the "warning: locally defined symbol imported" warnings.

cairo and pixman are built with:

FORCE_STATIC_LIB = 1
FORCE_USE_PIC = 1

and produce mozcairo.lib and mozlibpixman.lib.  configure.in sets MOZ_CAIRO_LIBS to be mozcairo mozlibpixman (.lib or -l as appropriate), or sets it to be the system cairo stuff.  layout/build/Makefile.in has:

ifdef MOZ_ENABLE_CAIRO_GFX
EXTRA_DSO_LIBS	+= thebes
endif
...
ifdef MOZ_ENABLE_CAIRO_GFX
ifndef MOZ_TREE_CAIRO
# In an in-tree cairo build, cairo is part of libthebes
# otherwise, we need to link it separately.
EXTRA_DSO_LDOPTS += $(MOZ_CAIRO_LIBS)
endif
endif

content/canvas/src, gfx/src/thebes, gfx/thebes/src, layout/svg/renderer/src/cairo all have "libpixman cairo" as part of REQUIRES.  These also all have CXXFLAGS += $(MOZ_CAIRO_CFLAGS).

I would like to have thebes be the entry point into cairo; thebes would be exporting all the (renamed) cairo symbols, and anyone that needs to link with cairo would either a) link with system cairo or b) link with thebes.

Note that is is irrelevant to whether we make thebes dynamic or static; I'm trying to make things work statically first, and then look into the dynamic issues.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: thebes linkage, attempt #1

I hate our build system.

This patch doesn't fully work, but I'm lost.

This works for static builds; but because thebes has no export/import information with this, it fails for dynamic builds (because inlined functions end up exporting a symbol).  What I think I need is:

- For static builds, do this as-is.  THEBES_API and cairo_public should both be nothing.

- For dynamic builds, I need THEBES_API and cairo_public to be dllimport by default, but be dllexport when building thebes.  This is what the older code did, which caused problems in the static build case.

ideally, there would be a global define that I can use to decide whether to do dllexport/dllimport or nothing, based on whether --enable-static was specified, but I can't find that.  Does such a thing exist?  Or am I totally fixing the wrong thing here?

Comment 5

[Benjamin Smedberg]

Attachment: thebes linkage in static builds, rev. 2

I haven't tried it, but I think this is what you want.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: thebes linking patch

Yep, I came up with much the same solution.  Here's the full patch that does the right thing with dllimport/dllexport for both cairo and thebes.  I've tested this with a clean static build and a clean shared build on windows, and I'm testing on linux right now.

Comment 7

[Benjamin Smedberg]

Comment on attachment 242226 [details] [diff] [review]
thebes linking patch

Why did you remove the -DIMPL_THEBES from gfx/thebes/src/Makefile.in? it seems that would still be necessary for dynamic builds, unless I'm missing something.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #7)
> (From update of attachment 242226 [details] [diff] [review] [edit])
> Why did you remove the -DIMPL_THEBES from gfx/thebes/src/Makefile.in? it seems
> that would still be necessary for dynamic builds, unless I'm missing something.

It's actually still in there; it was added being added twice to DEFINES.  I'm just blowing away one of them.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in.