When building for Android, package executables in APKs (trunk)

When building for Android, package executables in APKs (trunk)
Committed: https://code.google.com/p/skia/source/detail?r=4368

[epoger, 2012-06-18 14:39:12]

Please add reviewers from the "Publish+Mail Comments" page, not the "Edit Issue"
page.  (Only the "Publish+Mail" page causes emails to be sent to the new
reviewers.)

[epoger, 2012-06-18 14:55:59]

https://codereview.appspot.com/6307101/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6307101/diff/1/Makefile#newcode32
Makefile:32: ALL_TARGETS := core SampleApp bench gm tests tools SkiaAndroidApp
I'm worried about adding this to ALL_TARGETS.  If somebody is building for Mac
and they run "make all", they would be surprised to see something called
"SkiaAndroidApp" building.  (And would it cause a failure if they didn't have
the android directory?)

https://codereview.appspot.com/6307101/diff/1/Makefile#newcode55
Makefile:55: rm -rf out xcodebuild ../android/app/bin ../android/app/gen
../android/app/libs
Is there some way we could make the android targets leave their droppings in
out/ just like the other platforms do?  Or, second best, ../android/out ?

https://codereview.appspot.com/6307101/diff/1/gyp/all.gyp
File gyp/all.gyp (right):

https://codereview.appspot.com/6307101/diff/1/gyp/all.gyp#newcode35
gyp/all.gyp:35: 'conditions': [
Looking at the comment in line 14, I don't think this file is used at all. 
Please try reverting this change and see if it makes a difference...

https://codereview.appspot.com/6307101/diff/1/gyp/bench.gyp
File gyp/bench.gyp (right):

https://codereview.appspot.com/6307101/diff/1/gyp/bench.gyp#newcode30
gyp/bench.gyp:30: 'android_system.gyp:Android_EntryPoint',
So, every target should depend on Android_EntryPoint when building on Android,
right?  Maybe try putting this dependency in
http://code.google.com/p/skia/source/browse/trunk/gyp/common_conditions.gypi
instead of bench.gyp, gm.gyp, and tests.gyp?

[EricB, 2012-06-18 15:03:16]

https://codereview.appspot.com/6307101/diff/1/gyp/bench.gyp
File gyp/bench.gyp (right):

https://codereview.appspot.com/6307101/diff/1/gyp/bench.gyp#newcode30
gyp/bench.gyp:30: 'android_system.gyp:Android_EntryPoint',
We want bench, gm, and tests to depend on Android_EntryPoint, but SampleApp,
once it's made to work properly, won't be able to use the same Android app,
since it will use UI elements.

On 2012/06/18 14:55:59, epoger wrote:
> So, every target should depend on Android_EntryPoint when building on Android,
> right?  Maybe try putting this dependency in
> http://code.google.com/p/skia/source/browse/trunk/gyp/common_conditions.gypi
> instead of bench.gyp, gm.gyp, and tests.gyp?

[epoger, 2012-06-18 15:15:24]

https://codereview.appspot.com/6307101/diff/1/gyp/bench.gyp
File gyp/bench.gyp (right):

https://codereview.appspot.com/6307101/diff/1/gyp/bench.gyp#newcode30
gyp/bench.gyp:30: 'android_system.gyp:Android_EntryPoint',
On 2012/06/18 15:03:16, EricB wrote:
> We want bench, gm, and tests to depend on Android_EntryPoint, but SampleApp,
> once it's made to work properly, won't be able to use the same Android app,
> since it will use UI elements.
> 
> On 2012/06/18 14:55:59, epoger wrote:
> > So, every target should depend on Android_EntryPoint when building on
Android,
> > right?  Maybe try putting this dependency in
> > http://code.google.com/p/skia/source/browse/trunk/gyp/common_conditions.gypi
> > instead of bench.gyp, gm.gyp, and tests.gyp?
> 

Ah.  In that case, maybe put the conditional in
http://code.google.com/p/skia/source/browse/trunk/gyp/apptype_console.gypi ?

$ grep apptype_console gyp/* 
gyp/bench.gyp:    'apptype_console.gypi',
gyp/gm.gyp:    'apptype_console.gypi',
gyp/shapeops_edge.gyp:    'apptype_console.gypi',
gyp/tests.gyp:    'apptype_console.gypi',
gyp/tools.gyp:    'apptype_console.gypi',

[EricB, 2012-06-18 18:32:54]

https://codereview.appspot.com/6307101/diff/7001/Makefile
File Makefile (right):

https://codereview.appspot.com/6307101/diff/7001/Makefile#newcode32
Makefile:32: ALL_TARGETS := core SampleApp bench gm tests tools SkiaAndroidApp
This is still bad.  I haven't figured out a workaround, since adding the target
conditionally to skia.gyp doesn't seem to do anything.

https://codereview.appspot.com/6307101/diff/7001/Makefile#newcode55
Makefile:55: rm -rf out xcodebuild
android targets are now built in out/ so we don't need to remove them here.

[EricB, 2012-06-18 18:36:33]

https://codereview.appspot.com/6307101/diff/9002/Makefile
File Makefile (right):

https://codereview.appspot.com/6307101/diff/9002/Makefile#newcode32
Makefile:32: ALL_TARGETS := core SampleApp bench gm tests tools SkiaAndroidApp
Still need a workaround.

https://codereview.appspot.com/6307101/diff/9002/gyp/apptype_console.gypi
File gyp/apptype_console.gypi (right):

https://codereview.appspot.com/6307101/diff/9002/gyp/apptype_console.gypi#new...
gyp/apptype_console.gypi:20: ],
Note that this causes "tools" to be build as a single shared library rather than
multiple binaries.

[epoger, 2012-06-18 18:48:30]

LGTM except for one thing.

https://codereview.appspot.com/6307101/diff/9002/Makefile
File Makefile (right):

https://codereview.appspot.com/6307101/diff/9002/Makefile#newcode32
Makefile:32: ALL_TARGETS := core SampleApp bench gm tests tools SkiaAndroidApp
On 2012/06/18 18:36:33, EricB wrote:
> Still need a workaround.

For now, please leave SkiaAndroidApp off this list, and require Android
developers to manually "make SkiaAndroidApp all" or something like that.

(You could add an 'androidall' target if you prefer that.)

I think it's fine to require Android developers to specify a slightly different
target than other platforms, as long as you update
https://sites.google.com/site/skiadocs/user-documentation/quick-start-guides/...
to include the info.