Cleanup gms update/preprocess scripts, roll android_tools

Cleaning up and refactoring gms update/preprocess scripts

Also, add a pylib.utils.argpars_utils.CustomHelpAction
to support multiple help commands

Changelog for the android_tools roll:
https://chromium.googlesource.com/android_tools/+/54492f99c84cab0826a8e656efeb33a1b1bf5a04..f4c36ad89b2696b37d9cd7ca7d984b691888b188

BUG=541727
Committed: https://crrev.com/e9ec8cc2b148f10b793615a5bf77d8b8b64632e0
Cr-Commit-Position: refs/heads/master@{#365548}

[dgn, 2015-11-23 15:26:45]

dgn@chromium.org changed reviewers:
+ jbudorick@chromium.org

[dgn, 2015-11-23 15:26:46]

PTAL. I removed some now unused files, moved some to the play_services module,
ran pep8 on them and applied some of your comments from other reviews.

This should allow closing issue 541727

https://codereview.chromium.org/1469913002/diff/1/build/android/play_services...
File build/android/play_services/preprocess.py (left):

https://codereview.chromium.org/1469913002/diff/1/build/android/play_services...
build/android/play_services/preprocess.py:32: The json config (see the -c
argument) file should provide the following fields:
The description of this is now available when running with --config-help, via
the doc from utils.ConfigParser

https://codereview.chromium.org/1469913002/diff/1/build/android/play_services...
build/android/play_services/preprocess.py:81: from util import build_utils #
pylint: disable=import-error
Got rid of that, since it's supposed to be reserved for gyp/gn

https://codereview.chromium.org/1469913002/diff/1/build/android/preprocess_go...
File build/android/preprocess_google_play_services.py (left):

https://codereview.chromium.org/1469913002/diff/1/build/android/preprocess_go...
build/android/preprocess_google_play_services.py:1: #!/usr/bin/env python
moved to play_services/preprocess.py

https://codereview.chromium.org/1469913002/diff/1/build/install-build-deps-an...
File build/install-build-deps-android.sh (left):

https://codereview.chromium.org/1469913002/diff/1/build/install-build-deps-an...
build/install-build-deps-android.sh:18: if test "$1" = "--skip-sdk-packages";
then
Didn't find any usage of this in code search.

[dgn, 2015-11-23 15:57:21]

Description was changed from

==========
Cleaning up and refactoring gms update/preprocess scripts

BUG=541727
==========

to

==========
Cleaning up and refactoring gms update/preprocess scripts

Changelog for the android_tools roll:
https://chromium.googlesource.com/android_tools/+/54492f99c84cab0826a8e656efe...

BUG=541727
==========

[dgn, 2015-12-02 15:24:36]

Friendly ping https://goo.gl/VxIirA

[jbudorick, 2015-12-02 15:26:23]

On 2015/12/02 15:24:36, dgn wrote:
> Friendly ping https://goo.gl/VxIirA

ah, sorry. I totally forgot about this over the US holiday last week. Reviewing
now.

[jbudorick, 2015-12-02 19:09:40]

Nice cleanup.

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
File build/android/play_services/config.json (right):

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
build/android/play_services/config.json:3: "version_xml_path":
"res/values/version.xml"
Should this file also have sdk_version, clients, and locale_whitelist? If not,
how do those get added?

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
File build/android/play_services/preprocess.py (left):

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
build/android/play_services/preprocess.py:263: with open(tmp_version_file_path,
'w') as version_file:
Was this unused...?

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
File build/android/play_services/preprocess.py (right):

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
build/android/play_services/preprocess.py:73: # We do this test before parsing
the argument to be able to still show the
I think I understand what you're trying to do here, but I don't think this is
the right way to do it.

I'm trying to come up with a better way to do it, though, and it may involve
extending ArgumentParser...

[dgn, 2015-12-03 16:36:57]

PTAL

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
File build/android/play_services/config.json (right):

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
build/android/play_services/config.json:3: "version_xml_path":
"res/values/version.xml"
On 2015/12/02 19:09:40, jbudorick wrote:
> Should this file also have sdk_version, clients, and locale_whitelist? If not,
> how do those get added?

This is a bit weird because it's read/write file.
Scripts can write it to update the version number when pushing a new gmscore
update, which affects the download script
People can write it to change the configuration of the update or preprocess
script

Upstream we have no use of the other arguments, since we don't do the
preprocessing. That's why they are not included in this file.

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
File build/android/play_services/preprocess.py (left):

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
build/android/play_services/preprocess.py:263: with open(tmp_version_file_path,
'w') as version_file:
On 2015/12/02 19:09:40, jbudorick wrote:
> Was this unused...?

It was a FYI file that I added because people were asking what the exact version
of gmscore was in the repo. Now it's in the config file.

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
File build/android/play_services/preprocess.py (right):

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
build/android/play_services/preprocess.py:73: # We do this test before parsing
the argument to be able to still show the
On 2015/12/02 19:09:40, jbudorick wrote:
> I think I understand what you're trying to do here, but I don't think this is
> the right way to do it.
> 
> I'm trying to come up with a better way to do it, though, and it may involve
> extending ArgumentParser...

Done, see pylib/utils/argparse_utils.py

[dgn, 2015-12-03 18:20:56]

Description was changed from

==========
Cleaning up and refactoring gms update/preprocess scripts

Changelog for the android_tools roll:
https://chromium.googlesource.com/android_tools/+/54492f99c84cab0826a8e656efe...

BUG=541727
==========

to

==========
Cleaning up and refactoring gms update/preprocess scripts

Also, add a pylib.utils.argpars_utils.CustomHelpAction
to support multiple help commands

Changelog for the android_tools roll:
https://chromium.googlesource.com/android_tools/+/54492f99c84cab0826a8e656efe...

BUG=541727
==========

[dgn, 2015-12-08 11:02:18]

Ping, I would like to be able to use this for the next play services update. I
believe it's not too far away from now.

[jbudorick, 2015-12-10 14:16:31]

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
File build/android/play_services/config.json (right):

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
build/android/play_services/config.json:3: "version_xml_path":
"res/values/version.xml"
On 2015/12/03 16:36:57, dgn wrote:
> On 2015/12/02 19:09:40, jbudorick wrote:
> > Should this file also have sdk_version, clients, and locale_whitelist? If
not,
> > how do those get added?
> 
> This is a bit weird because it's read/write file.
> Scripts can write it to update the version number when pushing a new gmscore
> update, which affects the download script
> People can write it to change the configuration of the update or preprocess
> script
> 
> Upstream we have no use of the other arguments, since we don't do the
> preprocessing. That's why they are not included in this file.
> 

So what happens if somewhat attempts to use one of these other fields via
ConfigParser?

https://codereview.chromium.org/1469913002/diff/60001/build/android/play_serv...
File build/android/play_services/utils.py (right):

https://codereview.chromium.org/1469913002/diff/60001/build/android/play_serv...
build/android/play_services/utils.py:96: return self._data['locale_whitelist']
clients, version_xml_path, and locale_whitelist don't appear to handle the
expected case where they don't exist in the config.

https://codereview.chromium.org/1469913002/diff/60001/build/android/pylib/uti...
File build/android/pylib/utils/argparse_utils.py (right):

https://codereview.chromium.org/1469913002/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/argparse_utils.py:8: class
CustomHelpAction(argparse.Action):
Sweet!

https://codereview.chromium.org/1469913002/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/argparse_utils.py:10: Allows defining custom help
actions. Help actions can run even when the
nit: docstrings should be

'''Summary sentence.

More detailed description.
'''

[dgn, 2015-12-10 14:35:19]

About the ConfigParser, it looks like you would prefer it to be more like
reading the fields and properly caching and validating the values rather than
just hiding a dictionary we lazily read from. Did I understand the desired
change correctly?

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
File build/android/play_services/config.json (right):

https://codereview.chromium.org/1469913002/diff/40001/build/android/play_serv...
build/android/play_services/config.json:3: "version_xml_path":
"res/values/version.xml"
On 2015/12/10 14:16:31, jbudorick wrote:
> On 2015/12/03 16:36:57, dgn wrote:
> > On 2015/12/02 19:09:40, jbudorick wrote:
> > > Should this file also have sdk_version, clients, and locale_whitelist? If
> not,
> > > how do those get added?
> > 
> > This is a bit weird because it's read/write file.
> > Scripts can write it to update the version number when pushing a new gmscore
> > update, which affects the download script
> > People can write it to change the configuration of the update or preprocess
> > script
> > 
> > Upstream we have no use of the other arguments, since we don't do the
> > preprocessing. That's why they are not included in this file.
> > 
> 
> So what happens if somewhat attempts to use one of these other fields via
> ConfigParser?

Currently it would raise a KeyError when requesting one of the missing fields,
but since they are looked at lazily, when trying to read the field, it sounds
okay to me as way to communicate the issue. Is it better to have placeholder
values?

https://codereview.chromium.org/1469913002/diff/60001/build/android/play_serv...
File build/android/play_services/utils.py (right):

https://codereview.chromium.org/1469913002/diff/60001/build/android/play_serv...
build/android/play_services/utils.py:96: return self._data['locale_whitelist']
On 2015/12/10 14:16:31, jbudorick wrote:
> clients, version_xml_path, and locale_whitelist don't appear to handle the
> expected case where they don't exist in the config.

But that happens only when trying to read it. If running the preprocess script,
I would expect the input config to be properly populated. In the download/update
case, we don't read those so there is no issue.

[jbudorick, 2015-12-10 14:37:27]

https://codereview.chromium.org/1469913002/diff/60001/build/android/play_serv...
File build/android/play_services/utils.py (right):

https://codereview.chromium.org/1469913002/diff/60001/build/android/play_serv...
build/android/play_services/utils.py:96: return self._data['locale_whitelist']
On 2015/12/10 14:35:19, dgn wrote:
> On 2015/12/10 14:16:31, jbudorick wrote:
> > clients, version_xml_path, and locale_whitelist don't appear to handle the
> > expected case where they don't exist in the config.
> 
> But that happens only when trying to read it. If running the preprocess
script,
> I would expect the input config to be properly populated. In the
download/update
> case, we don't read those so there is no issue.

Hm. I'm debating between raising a KeyError and returning None, and I'm leaning
toward None.

[dgn, 2015-12-10 21:12:57]

PTAL

https://codereview.chromium.org/1469913002/diff/60001/build/android/play_serv...
File build/android/play_services/utils.py (right):

https://codereview.chromium.org/1469913002/diff/60001/build/android/play_serv...
build/android/play_services/utils.py:96: return self._data['locale_whitelist']
On 2015/12/10 14:37:27, jbudorick wrote:
> On 2015/12/10 14:35:19, dgn wrote:
> > On 2015/12/10 14:16:31, jbudorick wrote:
> > > clients, version_xml_path, and locale_whitelist don't appear to handle the
> > > expected case where they don't exist in the config.
> > 
> > But that happens only when trying to read it. If running the preprocess
> script,
> > I would expect the input config to be properly populated. In the
> download/update
> > case, we don't read those so there is no issue.
> 
> Hm. I'm debating between raising a KeyError and returning None, and I'm
leaning
> toward None.

Done.

https://codereview.chromium.org/1469913002/diff/60001/build/android/pylib/uti...
File build/android/pylib/utils/argparse_utils.py (right):

https://codereview.chromium.org/1469913002/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/argparse_utils.py:10: Allows defining custom help
actions. Help actions can run even when the
On 2015/12/10 14:16:31, jbudorick wrote:
> nit: docstrings should be
> 
> '''Summary sentence.
> 
> More detailed description.
> '''

Done.

[dgn, 2015-12-16 15:17:30]

Friendly ping

[jbudorick, 2015-12-16 15:25:42]

Sorry about the delay. This is very close.

https://codereview.chromium.org/1469913002/diff/80001/build/android/play_serv...
File build/android/play_services/preprocess.py (right):

https://codereview.chromium.org/1469913002/diff/80001/build/android/play_serv...
build/android/play_services/preprocess.py:256: with zipfile.ZipFile(zip_path,
"r") as zip_file:
nit: single quotes

https://codereview.chromium.org/1469913002/diff/80001/build/android/play_serv...
File build/android/play_services/utils.py (right):

https://codereview.chromium.org/1469913002/diff/80001/build/android/play_serv...
build/android/play_services/utils.py:78: self.version_number =
self._data.get(self._VERSION_NUMBER_KEY)
Having these as public members might imply to a client that they can be modified
and that the modified values will be saved when you write the new config (which
doesn't appear to be the case). If you don't want to support modifying them,
leave them as read-only properties.

[dgn, 2015-12-16 16:10:09]

PTAL

https://codereview.chromium.org/1469913002/diff/80001/build/android/play_serv...
File build/android/play_services/preprocess.py (right):

https://codereview.chromium.org/1469913002/diff/80001/build/android/play_serv...
build/android/play_services/preprocess.py:256: with zipfile.ZipFile(zip_path,
"r") as zip_file:
On 2015/12/16 15:25:42, jbudorick wrote:
> nit: single quotes

Done.

https://codereview.chromium.org/1469913002/diff/80001/build/android/play_serv...
File build/android/play_services/utils.py (right):

https://codereview.chromium.org/1469913002/diff/80001/build/android/play_serv...
build/android/play_services/utils.py:78: self.version_number =
self._data.get(self._VERSION_NUMBER_KEY)
On 2015/12/16 15:25:42, jbudorick wrote:
> Having these as public members might imply to a client that they can be
modified
> and that the modified values will be saved when you write the new config
(which
> doesn't appear to be the case). If you don't want to support modifying them,
> leave them as read-only properties.

Done.

[jbudorick, 2015-12-16 16:10:54]

lgtm

[dgn, 2015-12-16 16:11:43]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2015-12-16 16:12:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1469913002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1469913002/100001

[dgn, 2015-12-16 16:14:23]

On 2015/12/16 16:10:54, jbudorick wrote:
> lgtm

Thanks!

[commit-bot: I haz the power, 2015-12-16 17:18:10]

Description was changed from

==========
Cleaning up and refactoring gms update/preprocess scripts

Also, add a pylib.utils.argpars_utils.CustomHelpAction
to support multiple help commands

Changelog for the android_tools roll:
https://chromium.googlesource.com/android_tools/+/54492f99c84cab0826a8e656efe...

BUG=541727
==========

to

==========
Cleaning up and refactoring gms update/preprocess scripts

Also, add a pylib.utils.argpars_utils.CustomHelpAction
to support multiple help commands

Changelog for the android_tools roll:
https://chromium.googlesource.com/android_tools/+/54492f99c84cab0826a8e656efe...

BUG=541727
==========

[commit-bot: I haz the power, 2015-12-16 17:18:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-16 17:19:03]

Description was changed from

==========
Cleaning up and refactoring gms update/preprocess scripts

Also, add a pylib.utils.argpars_utils.CustomHelpAction
to support multiple help commands

Changelog for the android_tools roll:
https://chromium.googlesource.com/android_tools/+/54492f99c84cab0826a8e656efe...

BUG=541727
==========

to

==========
Cleaning up and refactoring gms update/preprocess scripts

Also, add a pylib.utils.argpars_utils.CustomHelpAction
to support multiple help commands

Changelog for the android_tools roll:
https://chromium.googlesource.com/android_tools/+/54492f99c84cab0826a8e656efe...

BUG=541727

Committed: https://crrev.com/e9ec8cc2b148f10b793615a5bf77d8b8b64632e0
Cr-Commit-Position: refs/heads/master@{#365548}
==========

[commit-bot: I haz the power, 2015-12-16 17:19:04]

Patchset 6 (id:??) landed as
https://crrev.com/e9ec8cc2b148f10b793615a5bf77d8b8b64632e0
Cr-Commit-Position: refs/heads/master@{#365548}