Closed Bug 712587 Opened 8 years ago Closed 8 years ago

Mark all graphitle2 functions as local

Categories

(Core :: Graphics, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jacek, Assigned: jacek)

References

Details

Attachments

(1 file)

Attached patch fix v1.0Splinter Review
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.
Attachment #583434 - Flags: review?(jfkthame)
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.
(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.
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.
Attachment #583434 - Flags: review?(jfkthame)
I believe the recent Graphite update in bug 721068, which introduced the GRAPHITE2_STATIC option, resolved this issue. Marking as fixed.
Status: NEW → RESOLVED
Closed: 8 years ago
Depends on: 721068
Resolution: --- → FIXED
I can confirm that the bug is fixed. Thanks!
You need to log in before you can comment on or make changes to this bug.