Improve NaCl support

Improve NaCl support

- Add nacl_make script to build Skia targets for NaCl using gyp
- Add nacl_interface for command-line apps
- Add nacl_sample as front-end for SampleApp
- Add freetype to DEPS
- Various gyp tweaks for NaCl

TODO:
- Implement GL interface
- Implement font host
- Fix plumbing so that SampleApp works properly
Committed: https://code.google.com/p/skia/source/detail?r=6245

[EricB, 2012-10-12 20:37:01]

Work in progress.

https://codereview.appspot.com/6671044/diff/1/nacl/Makefile
File nacl/Makefile (left):

https://codereview.appspot.com/6671044/diff/1/nacl/Makefile#oldcode1
nacl/Makefile:1: # Copyright (c) 2012 The Native Client Authors. All rights
reserved.
Removing this makefile; we build using our gyp files instead.

https://codereview.appspot.com/6671044/diff/1/trunk/DEPS
File trunk/DEPS (right):

https://codereview.appspot.com/6671044/diff/1/trunk/DEPS#newcode12
trunk/DEPS:12: "third_party/externals/freetype" :
"http://git.savannah.gnu.org/cgit/freetype/freetype2.git",
Will this work properly?  I imagine I can't add it to svn:externals...

https://codereview.appspot.com/6671044/diff/1/trunk/bench/benchmain.cpp
File trunk/bench/benchmain.cpp (right):

https://codereview.appspot.com/6671044/diff/1/trunk/bench/benchmain.cpp#newco...
trunk/bench/benchmain.cpp:912: #if !defined(SK_BUILD_FOR_IOS) &&
!defined(SK_BUILD_FOR_NACL)
Crashes occur when we define main().

https://codereview.appspot.com/6671044/diff/1/trunk/gyp/common.gypi
File trunk/gyp/common.gypi (right):

https://codereview.appspot.com/6671044/diff/1/trunk/gyp/common.gypi#newcode37
trunk/gyp/common.gypi:37: 'error': '<!(Skia NaCl build only currently supported
on Linux.)',
It has been suggested that the skia_nacl flag be folded into skia_os.  Doing so
would simplify some things while complicating others, although I'm not sure to
what degree.

https://codereview.appspot.com/6671044/diff/1/trunk/gyp/freetype.gyp
File trunk/gyp/freetype.gyp (right):

https://codereview.appspot.com/6671044/diff/1/trunk/gyp/freetype.gyp#newcode4
trunk/gyp/freetype.gyp:4: 'target_name': 'freetype',
Nobody was using the "skfreetype" target, so I stole it for myself.

https://codereview.appspot.com/6671044/diff/1/trunk/gyp/utils.gyp
File trunk/gyp/utils.gyp (left):

https://codereview.appspot.com/6671044/diff/1/trunk/gyp/utils.gyp#oldcode123
trunk/gyp/utils.gyp:123: 'link_settings': {
These link settings belong in gpu.gyp.

https://codereview.appspot.com/6671044/diff/1/trunk/include/views/SkOSWindow_...
File trunk/include/views/SkOSWindow_NaCl.h (right):

https://codereview.appspot.com/6671044/diff/1/trunk/include/views/SkOSWindow_...
trunk/include/views/SkOSWindow_NaCl.h:10: #ifndef SkOSWindow_NaCl_DEFINED
Copy/pasted from SkOSWindow_Android.

https://codereview.appspot.com/6671044/diff/1/trunk/src/gpu/gl/nacl/SkNativeG...
File trunk/src/gpu/gl/nacl/SkNativeGLContext_nacl.cpp (right):

https://codereview.appspot.com/6671044/diff/1/trunk/src/gpu/gl/nacl/SkNativeG...
trunk/src/gpu/gl/nacl/SkNativeGLContext_nacl.cpp:1: 
TODO

[DerekS, 2012-10-15 13:07:50]

https://codereview.appspot.com/6671044/diff/1/nacl/nacl_make
File nacl/nacl_make (right):

https://codereview.appspot.com/6671044/diff/1/nacl/nacl_make#newcode1
nacl/nacl_make:1: #!/bin/bash
can we move this file into a nacl/bin/ directory so it is clear what files are
to be used as helper scripts. Also then if I want to I can just add it to my
path to make it easier to build.

https://codereview.appspot.com/6671044/diff/1/trunk/gyp/common.gypi
File trunk/gyp/common.gypi (right):

https://codereview.appspot.com/6671044/diff/1/trunk/gyp/common.gypi#newcode37
trunk/gyp/common.gypi:37: 'error': '<!(Skia NaCl build only currently supported
on Linux.)',
On 2012/10/12 20:37:01, EricB wrote:
> It has been suggested that the skia_nacl flag be folded into skia_os.  Doing
so
> would simplify some things while complicating others, although I'm not sure to
> what degree.

The more I think about it the more I like the idea of removing that field and
using skia_os="nacl" from the start. There really shouldn't be that many places
in gyp that you will have to update and from the looks of it most of those
places you've already had to special case in some fashion for nacl anyway.

[epoger, 2012-10-29 15:42:59]

Let me know if/when you'd like me to review this (AFAIK it is still a "work in
progress"?).  Until then, I will happily ignore.

[EricB, 2012-10-30 12:14:13]

On 2012/10/29 15:42:59, epoger wrote:
> Let me know if/when you'd like me to review this (AFAIK it is still a "work in
> progress"?).  Until then, I will happily ignore.

I think it's ready for review.  It's bigger than I'd like, and there's probably
plenty to improve.

[bsalomon, 2012-10-30 12:27:07]

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/common_conditions...
File trunk/gyp/common_conditions.gypi (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/common_conditions...
trunk/gyp/common_conditions.gypi:158: }, {
maybe a comment here indicating this is skia_os != nacl

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp
File trunk/gyp/gpu.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp#newcode289
trunk/gyp/gpu.gyp:289: [ 'skia_gpu == 0', {
Do the nullify rules in target_defaults not already remove all the libraries
that would be added in this target's conditions block? I'm not sure about what
order the include/exclude rules are applied between a target and
target_defaults.

[DerekS, 2012-10-30 14:37:29]

https://codereview.appspot.com/6671044/diff/29001/trunk/DEPS
File trunk/DEPS (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/DEPS#newcode12
trunk/DEPS:12: "third_party/externals/freetype" :
"http://git.savannah.gnu.org/cgit/freetype/freetype2.git",
which version of freetype is this?  Could we suck the one from android into here
and remove the copy in the android directory?

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/SampleApp.gyp
File trunk/gyp/SampleApp.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/SampleApp.gyp#new...
trunk/gyp/SampleApp.gyp:294: '../../nacl/src/nacl_sample.cpp',
do we want to follow the android pattern of having a dependency gyp file that
contains all the nacl customizations.  I'm not sure one is better than the other
though.

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/debugger.gyp
File trunk/gyp/debugger.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/debugger.gyp#newc...
trunk/gyp/debugger.gyp:4: [ 'skia_os in ["linux", "freebsd", "openbsd",
"solaris", "nacl"]', {
do we want to allow you to build the debugger in nacl?  Seems like this is going
to be problematic

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/ports.gyp
File trunk/gyp/ports.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/ports.gyp#newcode44
trunk/gyp/ports.gyp:44: 'SK_CAN_USE_DLOPEN=1',
doesn't setting it to 1 here change the way it was previously defined in gyp?

[bungeman, 2012-10-30 15:11:38]

Drive-by reviews...

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/ports.gyp
File trunk/gyp/ports.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/ports.gyp#newcode52
trunk/gyp/ports.gyp:52: 'defines': [
Having two 'defines' like this in one dictionary is asking for trouble. I think
one of them is just ignored.

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/ports.gyp#newcode72
trunk/gyp/ports.gyp:72: 'SK_FONTHOST_FREETYPE_RUNTIME_VERSION=0x020300',\
If you're getting a newer version of FreeType, be sure to indicate that here.

[EricB, 2012-10-31 15:53:28]

Improve NaCl support

- Add nacl_make script to build Skia targets for NaCl using gyp
- Add nacl_interface for command-line apps
- Add nacl_sample as front-end for SampleApp
- Add freetype to DEPS
- Various gyp tweaks for NaCl

TODO:
- Implement GL interface
- Implement font host
- Fix plumbing so that SampleApp works properly

[EricB, 2012-10-31 15:58:22]

Uploaded patch set 10.

https://codereview.appspot.com/6671044/diff/29001/trunk/DEPS
File trunk/DEPS (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/DEPS#newcode12
trunk/DEPS:12: "third_party/externals/freetype" :
"http://git.savannah.gnu.org/cgit/freetype/freetype2.git",
On 2012/10/30 14:37:29, DerekS wrote:
> which version of freetype is this?  Could we suck the one from android into
here
> and remove the copy in the android directory?

Moved the freetype dependency from android to trunk.

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/SampleApp.gyp
File trunk/gyp/SampleApp.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/SampleApp.gyp#new...
trunk/gyp/SampleApp.gyp:294: '../../nacl/src/nacl_sample.cpp',
On 2012/10/30 14:37:29, DerekS wrote:
> do we want to follow the android pattern of having a dependency gyp file that
> contains all the nacl customizations.  I'm not sure one is better than the
other
> though. 

I sort of follow that pattern with nacl.gyp, which provides the ppapi wrapper. 
Since this is just one file and it's only used by SampleApp, I just inserted it
into the sources list.  I could create another target in nacl.gyp called
'nacl_sample' and instead add that as a dependency here...

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/common_conditions...
File trunk/gyp/common_conditions.gypi (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/common_conditions...
trunk/gyp/common_conditions.gypi:158: }, {
On 2012/10/30 12:27:07, bsalomon wrote:
> maybe a comment here indicating this is skia_os != nacl

Done.

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/debugger.gyp
File trunk/gyp/debugger.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/debugger.gyp#newc...
trunk/gyp/debugger.gyp:4: [ 'skia_os in ["linux", "freebsd", "openbsd",
"solaris", "nacl"]', {
On 2012/10/30 14:37:29, DerekS wrote:
> do we want to allow you to build the debugger in nacl?  Seems like this is
going
> to be problematic

We don't.  Gyp was complaining that qt_sdk wasn't defined, so I added this. 
Instead, I edited everything.gyp to skip this dependency altogether on nacl.

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp
File trunk/gyp/gpu.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp#newcode289
trunk/gyp/gpu.gyp:289: [ 'skia_gpu == 0', {
On 2012/10/30 12:27:07, bsalomon wrote:
> Do the nullify rules in target_defaults not already remove all the libraries
> that would be added in this target's conditions block? I'm not sure about what
> order the include/exclude rules are applied between a target and
> target_defaults.

So they do.  I'm not sure why I thought this was needed.  I do think that
inserting the above conditional is necessary.

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/ports.gyp
File trunk/gyp/ports.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/ports.gyp#newcode44
trunk/gyp/ports.gyp:44: 'SK_CAN_USE_DLOPEN=1',
On 2012/10/30 14:37:29, DerekS wrote:
> doesn't setting it to 1 here change the way it was previously defined in gyp?

See https://code.google.com/p/skia/source/detail?spec=svn6210&r=5398

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/ports.gyp#newcode52
trunk/gyp/ports.gyp:52: 'defines': [
On 2012/10/30 15:11:38, bungeman wrote:
> Having two 'defines' like this in one dictionary is asking for trouble. I
think
> one of them is just ignored.

Done.

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/ports.gyp#newcode72
trunk/gyp/ports.gyp:72: 'SK_FONTHOST_FREETYPE_RUNTIME_VERSION=0x020300',\
On 2012/10/30 15:11:38, bungeman wrote:
> If you're getting a newer version of FreeType, be sure to indicate that here.

Done.

[bsalomon, 2012-10-31 16:11:03]

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp
File trunk/gyp/gpu.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp#newcode205
trunk/gyp/gpu.gyp:205: [ 'skia_gpu', {
is this one needed?

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp#newcode289
trunk/gyp/gpu.gyp:289: [ 'skia_gpu == 0', {
On 2012/10/31 15:58:23, EricB wrote:
> On 2012/10/30 12:27:07, bsalomon wrote:
> > Do the nullify rules in target_defaults not already remove all the libraries
> > that would be added in this target's conditions block? I'm not sure about
what
> > order the include/exclude rules are applied between a target and
> > target_defaults.
> 
> So they do.  I'm not sure why I thought this was needed.  I do think that
> inserting the above conditional is necessary.

I'm missing somethinge... I don't see where X11 is added. And the nullify
doesn't remove GL?

[EricB, 2012-10-31 17:51:08]

Uploaded patch set 11.

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp
File trunk/gyp/gpu.gyp (right):

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp#newcode205
trunk/gyp/gpu.gyp:205: [ 'skia_gpu', {
On 2012/10/31 16:11:04, bsalomon wrote:
> is this one needed?

Removed.

https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp#newcode289
trunk/gyp/gpu.gyp:289: [ 'skia_gpu == 0', {
On 2012/10/31 16:11:04, bsalomon wrote:
> On 2012/10/31 15:58:23, EricB wrote:
> > On 2012/10/30 12:27:07, bsalomon wrote:
> > > Do the nullify rules in target_defaults not already remove all the
libraries
> > > that would be added in this target's conditions block? I'm not sure about
> what
> > > order the include/exclude rules are applied between a target and
> > > target_defaults.
> > 
> > So they do.  I'm not sure why I thought this was needed.  I do think that
> > inserting the above conditional is necessary.
> 
> I'm missing somethinge... I don't see where X11 is added. And the nullify
> doesn't remove GL?

This block was left over from when I was using the skia_nacl flag instead of
skia_os=nacl.  So these were added in the skia_os=="linux" block and needed to
be removed for nacl.  I've removed this conditional.

[bsalomon, 2012-10-31 17:54:37]

On 2012/10/31 17:51:08, EricB wrote:
> Uploaded patch set 11.
> 
> https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp
> File trunk/gyp/gpu.gyp (right):
> 
> https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp#newcode205
> trunk/gyp/gpu.gyp:205: [ 'skia_gpu', {
> On 2012/10/31 16:11:04, bsalomon wrote:
> > is this one needed?
> 
> Removed.
> 
> https://codereview.appspot.com/6671044/diff/29001/trunk/gyp/gpu.gyp#newcode289
> trunk/gyp/gpu.gyp:289: [ 'skia_gpu == 0', {
> On 2012/10/31 16:11:04, bsalomon wrote:
> > On 2012/10/31 15:58:23, EricB wrote:
> > > On 2012/10/30 12:27:07, bsalomon wrote:
> > > > Do the nullify rules in target_defaults not already remove all the
> libraries
> > > > that would be added in this target's conditions block? I'm not sure
about
> > what
> > > > order the include/exclude rules are applied between a target and
> > > > target_defaults.
> > > 
> > > So they do.  I'm not sure why I thought this was needed.  I do think that
> > > inserting the above conditional is necessary.
> > 
> > I'm missing somethinge... I don't see where X11 is added. And the nullify
> > doesn't remove GL?
> 
> This block was left over from when I was using the skia_nacl flag instead of
> skia_os=nacl.  So these were added in the skia_os=="linux" block and needed to
> be removed for nacl.  I've removed this conditional.

LGTM

[epoger, 2012-11-01 14:43:08]

LGTM (just skimmed it, really, after Brian's more thorough inspection)

https://codereview.appspot.com/6671044/diff/33004/trunk/gyp/ports.gyp
File trunk/gyp/ports.gyp (right):

https://codereview.appspot.com/6671044/diff/33004/trunk/gyp/ports.gyp#newcode69
trunk/gyp/ports.gyp:69: #We use Android's repo, which provides at least FreeType
2.4.0
Please indicate why we handle FreeType differently for nacl.  I guess we need to
compile FreeType into our own binary for nacl, while in the other cases we use a
dynamic library at runtime?  Why don't we just ALWAYS compile it into our own
binary?

(I assume there are good reasons for the setup you're using here, but please
either document them here or point at some other place where the reasoning is
documented.)

[EricB, 2012-11-01 14:51:20]

Uploaded patch set 12 which adds a comment.

https://codereview.appspot.com/6671044/diff/33004/trunk/gyp/ports.gyp
File trunk/gyp/ports.gyp (right):

https://codereview.appspot.com/6671044/diff/33004/trunk/gyp/ports.gyp#newcode69
trunk/gyp/ports.gyp:69: #We use Android's repo, which provides at least FreeType
2.4.0
On 2012/11/01 14:43:08, epoger wrote:
> Please indicate why we handle FreeType differently for nacl.  I guess we need
to
> compile FreeType into our own binary for nacl, while in the other cases we use
a
> dynamic library at runtime?  Why don't we just ALWAYS compile it into our own
> binary?
> 
> (I assume there are good reasons for the setup you're using here, but please
> either document them here or point at some other place where the reasoning is
> documented.)

We need our own version of freetype for nacl since the naclports version is
ancient.  On other OS we can use a dynamic library.  I don't know why, other
than code size, we don't use our own version all the time.  I added a comment.

[epoger, 2012-11-01 15:40:29]

https://codereview.appspot.com/6671044/diff/32009/trunk/gyp/ports.gyp
File trunk/gyp/ports.gyp (right):

https://codereview.appspot.com/6671044/diff/32009/trunk/gyp/ports.gyp#newcode64
trunk/gyp/ports.gyp:64: # we have to include our own version.
... "because the naclports version is too old (<0x020300) to provide
functionality we need"?

[EricB, 2012-11-01 16:43:43]

https://codereview.appspot.com/6671044/diff/32009/trunk/gyp/ports.gyp
File trunk/gyp/ports.gyp (right):

https://codereview.appspot.com/6671044/diff/32009/trunk/gyp/ports.gyp#newcode64
trunk/gyp/ports.gyp:64: # we have to include our own version.
On 2012/11/01 15:40:29, epoger wrote:
> ... "because the naclports version is too old (<0x020300) to provide
> functionality we need"?

Done.

[epoger, 2012-11-01 16:47:36]

LGTM

[DerekS, 2012-11-01 17:03:18]

https://codereview.appspot.com/6671044/diff/32046/trunk/DEPS
File trunk/DEPS (right):

https://codereview.appspot.com/6671044/diff/32046/trunk/DEPS#newcode12
trunk/DEPS:12: "third_party/externals/freetype" :
"https://android.googlesource.com/platform/external/freetype.git",
it hasn't been an issue yet, but do we want to try to pin it to a specific
revision?

https://codereview.appspot.com/6671044/diff/32046/trunk/gyp/freetype.gyp
File trunk/gyp/freetype.gyp (right):

https://codereview.appspot.com/6671044/diff/32046/trunk/gyp/freetype.gyp#newc...
trunk/gyp/freetype.gyp:63: ],
add the cflags from the android deps file to a dependency here.  You can also
remove the freetype stuff from the android file.

[EricB, 2012-11-01 17:13:22]

Uploaded patch set 14.

https://codereview.appspot.com/6671044/diff/32046/trunk/DEPS
File trunk/DEPS (right):

https://codereview.appspot.com/6671044/diff/32046/trunk/DEPS#newcode12
trunk/DEPS:12: "third_party/externals/freetype" :
"https://android.googlesource.com/platform/external/freetype.git",
On 2012/11/01 17:03:18, DerekS wrote:
> it hasn't been an issue yet, but do we want to try to pin it to a specific
> revision?

Let's leave it as-is for this CL but possibly do so in the future.

https://codereview.appspot.com/6671044/diff/32046/trunk/gyp/freetype.gyp
File trunk/gyp/freetype.gyp (right):

https://codereview.appspot.com/6671044/diff/32046/trunk/gyp/freetype.gyp#newc...
trunk/gyp/freetype.gyp:63: ],
On 2012/11/01 17:03:18, DerekS wrote:
> add the cflags from the android deps file to a dependency here.  You can also
> remove the freetype stuff from the android file.

Done.

[DerekS, 2012-11-01 17:18:31]

lgtm