enable shared lib support in linux for lua

enable shared lib support in linux for lua

R=bungeman@google.com, reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9703

[Zach Reizner, 2013-06-12 16:10:45]

The requisite 

To try it out, make sure the lua 5.2 package is installed and run:

$ export GYP_DEFINES="skia_shared_lib=1 skia_lua=1"
$ make
$ cd out/Debug/lib.target
$ lua
> package.loadlib("./libskia.so", "luaopen_skia")()
> paint = Sk.newPaint()
> paint:setColor{a=1, r=1, g=0, b=0}
> doc = Sk.newDocumentPDF('test.pdf')
> canvas = doc:beginPage(72*8.5, 72*11)
> canvas:drawText('Hello Lua', 300, 300, paint)
> doc:close()

Then open up test.pdf and observe 'Hello Lua' in the center of the page.

[reed1, 2013-06-12 17:07:42]

can't we say 'require skia' or something like that, instead of the loadpackage
call?

[djsollen, 2013-06-12 17:35:46]

https://codereview.chromium.org/16099014/diff/1/gyp/images.gyp
File gyp/images.gyp (left):

https://codereview.chromium.org/16099014/diff/1/gyp/images.gyp#oldcode110
gyp/images.gyp:110: '../src/images/SkImageDecoder_libpng.cpp',
why did we need to remove this?

https://codereview.chromium.org/16099014/diff/1/gyp/pdf.gyp
File gyp/pdf.gyp (left):

https://codereview.chromium.org/16099014/diff/1/gyp/pdf.gyp#oldcode9
gyp/pdf.gyp:9: 'skia_lib.gyp:skia_lib',
why did we need to remove this?

[Zach Reizner, 2013-06-12 17:42:40]

I'm looping Ben in because I'm told he knows more about gyp and can comment on
if my gyp changes will cause issues.
@Ben: I'd appreciate if you'd look over the changes to the gyp files and advise
how to refactor the multiple references to SkLua.cpp for all the tools and libs
that require it.

https://codereview.chromium.org/16099014/diff/1/gyp/images.gyp
File gyp/images.gyp (left):

https://codereview.chromium.org/16099014/diff/1/gyp/images.gyp#oldcode110
gyp/images.gyp:110: '../src/images/SkImageDecoder_libpng.cpp',
On 2013/06/12 17:35:46, djsollen wrote:
> why did we need to remove this?

This file is already included above. Sourcing it twice works fine for static lib
builds, but causes multiple definition errors for the shared lib version.

https://codereview.chromium.org/16099014/diff/1/gyp/pdf.gyp
File gyp/pdf.gyp (left):

https://codereview.chromium.org/16099014/diff/1/gyp/pdf.gyp#oldcode9
gyp/pdf.gyp:9: 'skia_lib.gyp:skia_lib',
On 2013/06/12 17:35:46, djsollen wrote:
> why did we need to remove this?

I have it so pdf.gyp:pdf is included in skia_lib.gyp, which causes circular
reference errors. This change does not break the static lib build as far as I
can tell.

[bungeman-skia, 2013-06-12 19:16:55]

Most of the change seems ok, except for having targets which are "sometimes in,
sometimes out" of skia_lib. I'm not sure at the moment how to solve the issue
properly.

https://codereview.chromium.org/16099014/diff/1/gyp/images.gyp
File gyp/images.gyp (left):

https://codereview.chromium.org/16099014/diff/1/gyp/images.gyp#oldcode110
gyp/images.gyp:110: '../src/images/SkImageDecoder_libpng.cpp',
On 2013/06/12 17:42:40, Zach Reizner wrote:
> On 2013/06/12 17:35:46, djsollen wrote:
> > why did we need to remove this?
> 
> This file is already included above. Sourcing it twice works fine for static
lib
> builds, but causes multiple definition errors for the shared lib version.

This is slightly bizarre. What this (to be deleted) block is actually doing is
forcing shared objects (shared_library and executable) which depend on this
target to also compile SkImageDecoder_libpng.cpp into themselves directly. I
imagine this was done because the SkImageDecoder discovers decoders after
linking. It is possible there is no reference directly to a decoder so the
linker simply doesn't link the decoder in, as the symbol appears to be unused.
This block seems to say 'I know that, oops, just compile in the decoder.' We now
have better means of keeping the decoders around, so all blocks like this
('sources' inside 'link_settings') should be removed. If this causes something
to break, talk to Leon about fixing it.

This is causing an issue with shared_library targets since the shared_library
target has a copy of the decoder and this static_library also has a copy. The
linker doesn't know which to use. In the executable case, I believe that since
the symbol is already resolved in the executable itself, the linker never
considers the symbol in the static_library.

https://codereview.chromium.org/16099014/diff/1/gyp/pdf.gyp
File gyp/pdf.gyp (left):

https://codereview.chromium.org/16099014/diff/1/gyp/pdf.gyp#oldcode9
gyp/pdf.gyp:9: 'skia_lib.gyp:skia_lib',
On 2013/06/12 17:42:40, Zach Reizner wrote:
> On 2013/06/12 17:35:46, djsollen wrote:
> > why did we need to remove this?
> 
> I have it so pdf.gyp:pdf is included in skia_lib.gyp, which causes circular
> reference errors. This change does not break the static lib build as far as I
> can tell.

But you're only doing so if skia_lua is set? I don't like just removing this
without replacement anyway, the pdf target should state what it depends on. It
doesn't seem to break anything only because all of the current shared objects
(executables) which depend on pdf also happen to depend on most everything else.

https://codereview.chromium.org/16099014/diff/1/gyp/skia_lib.gyp
File gyp/skia_lib.gyp (right):

https://codereview.chromium.org/16099014/diff/1/gyp/skia_lib.gyp#newcode54
gyp/skia_lib.gyp:54: 'pdf.gyp:pdf',
We should at least have a comment here about how any dependencies other than
targets in lua.gyp here are because lua.gyp exposes them. This seems rather iffy
to me, as SkLua can, in theory, expose anything it wants, making skia_shared_lib
into skia_kitchen_sink_shared_lib. It has already shown that this complicates
any target in this list, as it should depend on skia_shared_lib, except when
it's in it, in which case it needs to depend on the things which are in the
non-lua skia_shared_lib.

[Zach Reizner, 2013-06-18 18:59:40]

This one works slightly differently in the way lua requires the skia bindings.

$ export GYP_DEFINES="skia_shared_lib=1 skia_lua=1"
$ make sklua
$ cd out/Debug/lib.target
$ lua
> require "libsklua"
> paint = Sk.newPaint()
> paint:setColor{a=1, r=1, g=0, b=0}
> doc = Sk.newDocumentPDF('test.pdf')
> canvas = doc:beginPage(72*8.5, 72*11)
> canvas:drawText('Hello Lua', 300, 300, paint)
> doc:close()

[reed1, 2013-06-18 19:43:43]

c parts lgtm

[bungeman-skia, 2013-06-18 20:28:06]

https://codereview.chromium.org/16099014/diff/10001/gyp/images.gyp
File gyp/images.gyp (left):

https://codereview.chromium.org/16099014/diff/10001/gyp/images.gyp#oldcode109
gyp/images.gyp:109: 'sources': [
I think this was to be done independently with
https://codereview.chromium.org/17408002/ ?

https://codereview.chromium.org/16099014/diff/10001/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.chromium.org/16099014/diff/10001/gyp/tools.gyp#newcode116
gyp/tools.gyp:116: '-Wl,-rpath', '-Wl,./',
is it possible to just use '-Wl,-rpath=./' to avoid splitting the flag? Also,
this seems ld specific, perhaps this should go in a conditional to not add this
on Windows (not sure if this is used on Windows anyway)?

[bungeman-skia, 2013-06-19 20:35:24]

The use of rpath weighs heavily upon me, but perhaps such weight may be
mitigated in some small amount until such time as a more perfect solution can be
wrought.

https://codereview.chromium.org/16099014/diff/25001/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.chromium.org/16099014/diff/25001/gyp/tools.gyp#newcode120
gyp/tools.gyp:120: '-Wl,-rpath', '-Wl,./',
rpath is rather heavy handed, (I think) we should also specify
'-–enable-new-dtags' to make ld use DT_RUNPATH in addition to DT_RPATH. Then it
is possible to use LD_LIBRARY_PATH to force the link when debugging. I'm not
sure if this is supported on Mac though...

Note that the issue here is the larger issue which Skia has studiously avoided,
library and installation paths. Autotools uses '--prefix' for this sort of
thing, I think.

[Zach Reizner, 2013-06-20 15:01:01]

New commands for running inside lua:
$ cd out/Debug
$ lua
require 'skia'
paint = Sk.newPaint()
paint:setColor{a=1, r=1, g=0, b=0}
doc = Sk.newDocumentPDF('test.pdf')
canvas = doc:beginPage(72*8.5, 72*11)
canvas:drawText('Hello Lua', 300, 300, paint)
doc:close()

[bungeman-skia, 2013-06-20 15:17:10]

lgtm at Patch Set 7.

Congratulations, you now know more about gyp and ld than most!

[commit-bot: I haz the power, 2013-06-20 15:19:07]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/zachr@google.com/16099014/31001

[commit-bot: I haz the power, 2013-06-20 15:34:38]

Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
step(s) BuildBench, BuildGm, BuildMost, BuildSkiaLib, BuildTests, BuildTools
http://108.170.217.252:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...

[Zach Reizner, 2013-06-20 17:15:13]

Committed patchset #7 manually as r9703 (presubmit successful).