Updates to Android.mk generation.

Updates to Android.mk generation.

Generate SkUserConfig.
Include arm64 as another build flavor.
Add tests.

gyp/common_conditions.gypi:
Add conditions for Android framework. These will get written into the generated SkUserConfig.

include/core/SkUserConfig.h:
Generated version that will ultimately be checked into Android (but not here).

platform_tools/android/bin/gyp_to_android.py:
Generate SkUserConfig.
Add arm64 (note that arm64 is not currently respected by our gyp files, so it results in use _none.cpp for the various opts).
Reset the common defines, which are now passed to the generated SkUserConfig.

platform_tools/android/gyp_gen/generate_user_config.py:
New script to generate SkUserConfig.h.

platform_tools/android/gyp_gen/gypd_parser.py:
Fix a lint error (unused import).

platform_tools/android/gyp_gen/makefile_writer.py:
Append any remaining DEFINES to LOCAL_CFLAGS (previously this was done during parsing).
Add a warning for arm64 (corresponds to downstream Android.mk).

platform_tools/android/gyp_gen/vars_dict_lib.py:
Add OrderedSet.reset().
Add DEFINES to VarsDict.

platform_tools/android/tests/expectations/:
Add and update expectations files.

platform_tools/android/tests/generate_user_config_tests.py:
New test for generate_user_config.py

platform_tools/android/tests/inputs/SkUserConfig.h:
Input to the new test, so we don't have to update the expectations each time the real SkUserConfig.h changes.

platform_tools/android/tests/makefile_writer_tests.py:
Add a way to rebaseline test_write_local_vars, which has changed.
Refactor EXPECTATIONS_DIR and compare_files into a separate file for sharing with generate_user_config_tests.py.

platform_tools/android/tests/utils.py:
Common code for tests.

platform_tools/android/tests/var_dict_tests.py:
Use a for loop to test the new key (DEFINES) and future proof this test to test any new keys in the future.

BUG=skia:1975
Committed: http://code.google.com/p/skia/source/detail?r=13975

[scroggo, 2014-03-12 19:53:53]

Derek,

This CL generates SkUserConfig and public_headers.gypi

I still need to look back over the python files (run pylint, add tests, etc),
but I wanted you to see the changes to the makefile and SkUserConfig.

[scroggo, 2014-03-25 22:03:00]

https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi
File gyp/public_headers.gypi (right):

https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi#...
gyp/public_headers.gypi:4: #  THIS FILE IS AUTOGENERATED BY GYP_TO_ANDROID.PY.
DO NOT EDIT.
I've only included this file to show what the generated version will look like.
In truth, I think it should not reside in our repository (since we don't need
it, and it easy for it to get out of date). This means forking, but that might
be better than the alternative.

https://codereview.chromium.org/198063002/diff/20001/platform_tools/android/t...
File platform_tools/android/tests/generate_user_config_tests.py (right):

https://codereview.chromium.org/198063002/diff/20001/platform_tools/android/t...
platform_tools/android/tests/generate_user_config_tests.py:25: def
test_missing_sk_user_config(self):
I plan to add a test similar to the comparison tests in
https://codereview.chromium.org/205383006/

https://codereview.chromium.org/198063002/diff/20001/platform_tools/android/t...
File platform_tools/android/tests/var_dict_tests.py (right):

https://codereview.chromium.org/198063002/diff/20001/platform_tools/android/t...
platform_tools/android/tests/var_dict_tests.py:63:
v_dict.LOCAL_CFLAGS.add('cflag')
Could simplify this by looping over v_dict.keys()

[djsollen, 2014-03-26 13:07:53]

https://codereview.chromium.org/198063002/diff/20001/Android.mk
File Android.mk (right):

https://codereview.chromium.org/198063002/diff/20001/Android.mk#newcode6
Android.mk:6:
###############################################################################
Is this file just for reference? Or do you plan to check it in?

https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi
File gyp/public_headers.gypi (right):

https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi#...
gyp/public_headers.gypi:4: #  THIS FILE IS AUTOGENERATED BY GYP_TO_ANDROID.PY.
DO NOT EDIT.
On 2014/03/25 22:03:00, scroggo wrote:
> I've only included this file to show what the generated version will look
like.
> In truth, I think it should not reside in our repository (since we don't need
> it, and it easy for it to get out of date). This means forking, but that might
> be better than the alternative.

I think the Chromium bots that run WebView tests may use this file. We will need
to confirm with them before we remove it from this repo.

https://codereview.chromium.org/198063002/diff/20001/include/core/SkUserConfig.h
File include/core/SkUserConfig.h (left):

https://codereview.chromium.org/198063002/diff/20001/include/core/SkUserConfi...
include/core/SkUserConfig.h:3: * Copyright 2006 The Android Open Source Project
What branch is this file in? I'm a little lost as to what versions of this file
the code review tool is comparing.

[scroggo, 2014-03-26 15:58:38]

https://codereview.chromium.org/198063002/diff/20001/Android.mk
File Android.mk (right):

https://codereview.chromium.org/198063002/diff/20001/Android.mk#newcode6
Android.mk:6:
###############################################################################
On 2014/03/26 13:07:53, djsollen wrote:
> Is this file just for reference? Or do you plan to check it in?

Just for reference. I realized that reitveld shows the diff between your branch
and the branch it's based on, so I checked out a branch, generated the makefile
using gyp_to_android, and committed it. Then I checked out a new branch based
off that one, made my changes, and generated the makefile again. This diff shows
that difference. If I try to commit as is, it will fail, so I'll need to rebase
off master and remove the changes to this file (which doesn't exist), but it's
an easy way for you to see what difference this change will make.

https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi
File gyp/public_headers.gypi (right):

https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi#...
gyp/public_headers.gypi:4: #  THIS FILE IS AUTOGENERATED BY GYP_TO_ANDROID.PY.
DO NOT EDIT.
On 2014/03/26 13:07:53, djsollen wrote:
> On 2014/03/25 22:03:00, scroggo wrote:
> > I've only included this file to show what the generated version will look
> like.
> > In truth, I think it should not reside in our repository (since we don't
need
> > it, and it easy for it to get out of date). This means forking, but that
might
> > be better than the alternative.
> 
> I think the Chromium bots that run WebView tests may use this file. We will
need
> to confirm with them before we remove it from this repo.

You think they use the copy that's checked into Skia? That's good to know. That
means they've been using an out of date version since M32. If they need this
file too, I think it would really make sense to autogenerate it with each check
in (at least every check in that adds/removes a header file, but it's probably
no more work to just generate it each time). Otherwise, it will almost always be
out of date.

It appears they only reference public_headers here:
https://code.google.com/p/chromium/codesearch#chromium/src/skia/skia_system.g...,
which according to Torne's comment here:
https://googleplex-android-review.git.corp.google.com/#/c/431165 is only used by
WebView (at least currently).

Looking back at that CL, I realize that he modified the version inside the
chromium tree (on Android). In that case, I think this file should be checked
into chromium itself (is there any reason it didn't just go into
chromium/src/skia alongside skia_system.gypi to begin with?). It could be
updated along with the DEPS roll.

https://codereview.chromium.org/198063002/diff/20001/include/core/SkUserConfig.h
File include/core/SkUserConfig.h (left):

https://codereview.chromium.org/198063002/diff/20001/include/core/SkUserConfi...
include/core/SkUserConfig.h:3: * Copyright 2006 The Android Open Source Project
On 2014/03/26 13:07:53, djsollen wrote:
> What branch is this file in? I'm a little lost as to what versions of this
file
> the code review tool is comparing.

I'm using the same trickery that I used for Android.mk. I committed a straight
copy of the original into include/core (locally). Then I ran my generation code
and committed to a new branch. So this compares the generated version to the
version that is checked into include/config.

https://codereview.chromium.org/198063002/diff/20001/include/core/SkUserConfi...
include/core/SkUserConfig.h:41: // ANDROID Specific changes - NO NOT CHECK BACK
INTO code.google.com/p/skia
Note that the Android specific changes have been moved to the bottom of the file
(though feel free to double check to make sure I haven't missed anything).
They've now been generated from common_conditions or from the generator code
itself (for the more complicated ones e.g. ENDIAN-ness).

[djsollen, 2014-03-26 20:17:03]

modulo my comments I'm fine with the non-python portion of the code.

https://codereview.chromium.org/198063002/diff/20001/gyp/common.gypi
File gyp/common.gypi (right):

https://codereview.chromium.org/198063002/diff/20001/gyp/common.gypi#newcode93
gyp/common.gypi:93: 'SK_SCALAR_TO_FLOAT_EXCLUDED',
why do we need to call this out here?  Won't we need this for now if the WebView
wanted to use the system copy of Skia?

https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi
File gyp/public_headers.gypi (right):

https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi#...
gyp/public_headers.gypi:4: #  THIS FILE IS AUTOGENERATED BY GYP_TO_ANDROID.PY.
DO NOT EDIT.
On 2014/03/26 15:58:39, scroggo wrote:
> On 2014/03/26 13:07:53, djsollen wrote:
> > On 2014/03/25 22:03:00, scroggo wrote:
> > > I've only included this file to show what the generated version will look
> > like.
> > > In truth, I think it should not reside in our repository (since we don't
> need
> > > it, and it easy for it to get out of date). This means forking, but that
> might
> > > be better than the alternative.
> > 
> > I think the Chromium bots that run WebView tests may use this file. We will
> need
> > to confirm with them before we remove it from this repo.
> 
> You think they use the copy that's checked into Skia? That's good to know.
That
> means they've been using an out of date version since M32. If they need this
> file too, I think it would really make sense to autogenerate it with each
check
> in (at least every check in that adds/removes a header file, but it's probably
> no more work to just generate it each time). Otherwise, it will almost always
be
> out of date.
> 
> It appears they only reference public_headers here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/skia/skia_system.g...,
> which according to Torne's comment here:
> https://googleplex-android-review.git.corp.google.com/#/c/431165 is only used
by
> WebView (at least currently).
> 
> Looking back at that CL, I realize that he modified the version inside the
> chromium tree (on Android). In that case, I think this file should be checked
> into chromium itself (is there any reason it didn't just go into
> chromium/src/skia alongside skia_system.gypi to begin with?). It could be
> updated along with the DEPS roll.

I agree that it should be auto-generated as part of our DEPS roll into chromium
and that it is best suited to live in their repo.

[hal.canary, 2014-03-27 12:30:15]

https://codereview.chromium.org/198063002/diff/110001/platform_tools/android/...
File platform_tools/android/gyp_gen/gen_public_headers.py (right):

https://codereview.chromium.org/198063002/diff/110001/platform_tools/android/...
platform_tools/android/gyp_gen/gen_public_headers.py:39: headers = []
More Pythonic:

  return sorted(
    os.path.join(dirpath, filename)  
      for dirpath, dirnames, filenames in os.walk(top_dir)
	for filename in filenames
	    if filename.endswith('.h'))

[scroggo, 2014-03-27 19:33:00]

Derek, I believe I've addressed your comments. Hal, can you review the python
code?

https://codereview.chromium.org/198063002/diff/20001/gyp/common.gypi
File gyp/common.gypi (right):

https://codereview.chromium.org/198063002/diff/20001/gyp/common.gypi#newcode93
gyp/common.gypi:93: 'SK_SCALAR_TO_FLOAT_EXCLUDED',
On 2014/03/26 20:17:03, djsollen wrote:
> why do we need to call this out here?  Won't we need this for now if the
WebView
> wanted to use the system copy of Skia?

Removed.

https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi
File gyp/public_headers.gypi (right):

https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi#...
gyp/public_headers.gypi:4: #  THIS FILE IS AUTOGENERATED BY GYP_TO_ANDROID.PY.
DO NOT EDIT.
On 2014/03/26 20:17:03, djsollen wrote:
> On 2014/03/26 15:58:39, scroggo wrote:
> > On 2014/03/26 13:07:53, djsollen wrote:
> > > On 2014/03/25 22:03:00, scroggo wrote:
> > > > I've only included this file to show what the generated version will
look
> > > like.
> > > > In truth, I think it should not reside in our repository (since we don't
> > need
> > > > it, and it easy for it to get out of date). This means forking, but that
> > might
> > > > be better than the alternative.
> > > 
> > > I think the Chromium bots that run WebView tests may use this file. We
will
> > need
> > > to confirm with them before we remove it from this repo.
> > 
> > You think they use the copy that's checked into Skia? That's good to know.
> That
> > means they've been using an out of date version since M32. If they need this
> > file too, I think it would really make sense to autogenerate it with each
> check
> > in (at least every check in that adds/removes a header file, but it's
probably
> > no more work to just generate it each time). Otherwise, it will almost
always
> be
> > out of date.
> > 
> > It appears they only reference public_headers here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/skia/skia_system.g...,
> > which according to Torne's comment here:
> > https://googleplex-android-review.git.corp.google.com/#/c/431165 is only
used
> by
> > WebView (at least currently).
> > 
> > Looking back at that CL, I realize that he modified the version inside the
> > chromium tree (on Android). In that case, I think this file should be
checked
> > into chromium itself (is there any reason it didn't just go into
> > chromium/src/skia alongside skia_system.gypi to begin with?). It could be
> > updated along with the DEPS roll.
> 
> I agree that it should be auto-generated as part of our DEPS roll into
chromium
> and that it is best suited to live in their repo.

Glad to hear it. I've filed https://code.google.com/p/skia/issues/detail?id=2350
to track moving it to Chromium/auto-generating it. I'll remove the code that
auto generates here.

https://codereview.chromium.org/198063002/diff/110001/platform_tools/android/...
File platform_tools/android/gyp_gen/gen_public_headers.py (right):

https://codereview.chromium.org/198063002/diff/110001/platform_tools/android/...
platform_tools/android/gyp_gen/gen_public_headers.py:39: headers = []
On 2014/03/27 12:30:15, Hal Canary wrote:
> More Pythonic:
> 
>   return sorted(
>     os.path.join(dirpath, filename)  
>       for dirpath, dirnames, filenames in os.walk(top_dir)
> 	for filename in filenames
> 	    if filename.endswith('.h'))

Thanks for the feedback! I'm actually removing this, since I think the correct
place for public_headers to belong is in chromium, and therefore the generation
should belong to chromium.

[scroggo, 2014-03-28 14:05:11]

On 2014/03/27 19:33:00, scroggo wrote:
> Derek, I believe I've addressed your comments. Hal, can you review the python
> code?
> 
> https://codereview.chromium.org/198063002/diff/20001/gyp/common.gypi
> File gyp/common.gypi (right):
> 
> https://codereview.chromium.org/198063002/diff/20001/gyp/common.gypi#newcode93
> gyp/common.gypi:93: 'SK_SCALAR_TO_FLOAT_EXCLUDED',
> On 2014/03/26 20:17:03, djsollen wrote:
> > why do we need to call this out here?  Won't we need this for now if the
> WebView
> > wanted to use the system copy of Skia?
> 
> Removed.
> 
> https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi
> File gyp/public_headers.gypi (right):
> 
>
https://codereview.chromium.org/198063002/diff/20001/gyp/public_headers.gypi#...
> gyp/public_headers.gypi:4: #  THIS FILE IS AUTOGENERATED BY GYP_TO_ANDROID.PY.
> DO NOT EDIT.
> On 2014/03/26 20:17:03, djsollen wrote:
> > On 2014/03/26 15:58:39, scroggo wrote:
> > > On 2014/03/26 13:07:53, djsollen wrote:
> > > > On 2014/03/25 22:03:00, scroggo wrote:
> > > > > I've only included this file to show what the generated version will
> look
> > > > like.
> > > > > In truth, I think it should not reside in our repository (since we
don't
> > > need
> > > > > it, and it easy for it to get out of date). This means forking, but
that
> > > might
> > > > > be better than the alternative.
> > > > 
> > > > I think the Chromium bots that run WebView tests may use this file. We
> will
> > > need
> > > > to confirm with them before we remove it from this repo.
> > > 
> > > You think they use the copy that's checked into Skia? That's good to know.
> > That
> > > means they've been using an out of date version since M32. If they need
this
> > > file too, I think it would really make sense to autogenerate it with each
> > check
> > > in (at least every check in that adds/removes a header file, but it's
> probably
> > > no more work to just generate it each time). Otherwise, it will almost
> always
> > be
> > > out of date.
> > > 
> > > It appears they only reference public_headers here:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/skia/skia_system.g...,
> > > which according to Torne's comment here:
> > > https://googleplex-android-review.git.corp.google.com/#/c/431165 is only
> used
> > by
> > > WebView (at least currently).
> > > 
> > > Looking back at that CL, I realize that he modified the version inside the
> > > chromium tree (on Android). In that case, I think this file should be
> checked
> > > into chromium itself (is there any reason it didn't just go into
> > > chromium/src/skia alongside skia_system.gypi to begin with?). It could be
> > > updated along with the DEPS roll.
> > 
> > I agree that it should be auto-generated as part of our DEPS roll into
> chromium
> > and that it is best suited to live in their repo.
> 
> Glad to hear it. I've filed
https://code.google.com/p/skia/issues/detail?id=2350
> to track moving it to Chromium/auto-generating it. I'll remove the code that
> auto generates here.
> 
>
https://codereview.chromium.org/198063002/diff/110001/platform_tools/android/...
> File platform_tools/android/gyp_gen/gen_public_headers.py (right):
> 
>
https://codereview.chromium.org/198063002/diff/110001/platform_tools/android/...
> platform_tools/android/gyp_gen/gen_public_headers.py:39: headers = []
> On 2014/03/27 12:30:15, Hal Canary wrote:
> > More Pythonic:
> > 
> >   return sorted(
> >     os.path.join(dirpath, filename)  
> >       for dirpath, dirnames, filenames in os.walk(top_dir)
> > 	for filename in filenames
> > 	    if filename.endswith('.h'))
> 
> Thanks for the feedback! I'm actually removing this, since I think the correct
> place for public_headers to belong is in chromium, and therefore the
generation
> should belong to chromium.

Hal's given me a verbal LGTM; can I get an lgtm from one of you?

[hal.canary, 2014-03-28 14:09:39]

On 2014/03/28 14:05:11, scroggo wrote:
> Hal's given me a verbal LGTM; can I get an lgtm from one of you?


LGTM.  As mentioned, you should replace the OrderedSet class with Python's
builtins: sorted(set())

[djsollen, 2014-03-28 14:34:58]

lgtm

[scroggo, 2014-03-28 14:46:44]

On 2014/03/28 14:09:39, Hal Canary wrote:
> On 2014/03/28 14:05:11, scroggo wrote:
> > Hal's given me a verbal LGTM; can I get an lgtm from one of you?
> 
> 
> LGTM.  As mentioned, you should replace the OrderedSet class with Python's
> builtins: sorted(set())

I just remembered - I don't want them to be sorted alphabetically - I want them
in the same order they were inserted in. This is important because the build
order determines the order in which our image decoders are tested on streams, so
we need to compile them in an order such that the most common ones get tested
first.

[scroggo, 2014-03-28 14:48:09]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-03-28 14:48:16]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/scroggo@google.com/198063002/160001

[commit-bot: I haz the power, 2014-03-28 15:59:12]

Change committed as 13975

[scroggo, 2014-03-28 17:39:27]

https://codereview.chromium.org/198063002/diff/20001/gyp/common.gypi
File gyp/common.gypi (right):

https://codereview.chromium.org/198063002/diff/20001/gyp/common.gypi#newcode93
gyp/common.gypi:93: 'SK_SCALAR_TO_FLOAT_EXCLUDED',
On 2014/03/27 19:33:00, scroggo wrote:
> On 2014/03/26 20:17:03, djsollen wrote:
> > why do we need to call this out here?  Won't we need this for now if the
> WebView
> > wanted to use the system copy of Skia?
> 
> Removed.

It turns out we *do* need this here*. All the negatives are confusing, but
here's how it works:

If you define this flag, SkFloatToScalar is not defined.

Android still uses SkFloatToScalar:
https://cs.corp.google.com/#search/&q=skfloattoscalar&sq=package:android

Here I'm changing it so we do *not* define this flag, meaning that
SkFloatToScalar *is* defined.

Defining SkFloatToScalar will not interfere with building WebView.

*Until we stop calling SkFloatToScalar in frameworks/base.

[djsollen, 2014-03-28 20:54:52]

On 2014/03/28 17:39:27, scroggo wrote:
> https://codereview.chromium.org/198063002/diff/20001/gyp/common.gypi
> File gyp/common.gypi (right):
> 
> https://codereview.chromium.org/198063002/diff/20001/gyp/common.gypi#newcode93
> gyp/common.gypi:93: 'SK_SCALAR_TO_FLOAT_EXCLUDED',
> On 2014/03/27 19:33:00, scroggo wrote:
> > On 2014/03/26 20:17:03, djsollen wrote:
> > > why do we need to call this out here?  Won't we need this for now if the
> > WebView
> > > wanted to use the system copy of Skia?
> > 
> > Removed.
> 
> It turns out we *do* need this here*. All the negatives are confusing, but
> here's how it works:
> 
> If you define this flag, SkFloatToScalar is not defined.
> 
> Android still uses SkFloatToScalar:
> https://cs.corp.google.com/#search/&q=skfloattoscalar&sq=package:android
> 
> Here I'm changing it so we do *not* define this flag, meaning that
> SkFloatToScalar *is* defined.
> 
> Defining SkFloatToScalar will not interfere with building WebView.
> 
> *Until we stop calling SkFloatToScalar in frameworks/base.

Thanks for the info. Lets file an bug in Android and go ahead and clean that up.