Last Comment Bug 712587 - Mark all graphitle2 functions as local
: Mark all graphitle2 functions as local
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: Jacek Caban
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 721068
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-21 02:26 PST by Jacek Caban
Modified: 2012-02-01 04:36 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (1.10 KB, patch)
2011-12-21 02:26 PST, Jacek Caban
no flags Details | Diff | Splinter Review

Description Jacek Caban 2011-12-21 02:26:53 PST
Created attachment 583434 [details] [diff] [review]
fix v1.0

In our case, both graphite2 and the code using it ends up linked in the same library, but its functions are marked as exported/imported. It's not well handled by mingw. Marking them all as GR2_LOCAL has other nice effect on platforms supporting visibility("hidden"). I've made the change in a way that touches upstream code as minimally as possible.
Comment 1 Jonathan Kew (:jfkthame) 2011-12-21 02:38:30 PST
AFAICT, this ought to work, but I'd prefer to get upstream to accept a patch that allows us to control this from the makefile rather than applying a local code-level patch.

I just pushed https://hg.mozilla.org/try/rev/67bbb9e7eaa9 to tryserver to see what effect this has (ignore the gfxMacFont debugging chunk accidentally included there!), and will negotiate with upstream about the change provided this works as intended.
Comment 2 Jacek Caban 2011-12-21 02:43:28 PST
(In reply to Jonathan Kew (:jfkthame) from comment #1)
> AFAICT, this ought to work, but I'd prefer to get upstream to accept a patch
> that allows us to control this from the makefile rather than applying a
> local code-level patch.

If they would accept it, that's indeed better.

> I just pushed https://hg.mozilla.org/try/rev/67bbb9e7eaa9 to tryserver to
> see what effect this has (ignore the gfxMacFont debugging chunk accidentally
> included there!),

It's also a part of my try commit:

https://tbpl.mozilla.org/?tree=Try&rev=905f704e269f

Sorry I didn't mention it earlier.

> and will negotiate with upstream about the change provided this works as intended.

Thanks.
Comment 3 Jonathan Kew (:jfkthame) 2011-12-21 14:17:04 PST
Oops, my tryserver job was pointless as I pushed from an old tree where Graphite was disabled in configure.in. :(

New push at https://hg.mozilla.org/try/rev/bb69fb296160 confirms that this does resolve the dll-visibility warnings. I'll discuss with upstream.
Comment 4 Jonathan Kew (:jfkthame) 2012-01-30 16:03:37 PST
I believe the recent Graphite update in bug 721068, which introduced the GRAPHITE2_STATIC option, resolved this issue. Marking as fixed.
Comment 5 Jacek Caban 2012-02-01 04:36:14 PST
I can confirm that the bug is fixed. Thanks!

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