diagnose_apk_bloat.py: handle more rev options.

diagnose_apk_bloat.py: handle more rev options.

This CL introduces the ability to perform builds/diffs on all commits in
a specified range by passing the --all option. Other features added
include:
  * Perform local builds on commits in a subrepo (via --subrepo)
  * Store metadata and prevent unnecessary builds/downloads/diffs

BUG=710076
Review-Url: https://codereview.chromium.org/2834103002
Cr-Commit-Position: refs/heads/master@{#466456}
Committed: https://chromium.googlesource.com/chromium/src/+/af790e8aa737385eb96d134ab18c475185671edc

[estevenson, 2017-04-21 14:58:09]

estevenson@chromium.org changed reviewers:
+ agrieve@chromium.org

[estevenson, 2017-04-21 14:58:10]

ptal Andrew.

[estevenson, 2017-04-21 16:48:03]

Description was changed from

==========
Android: handle more rev options in diagnose_apk_bloat.py.

This CL introduces the ability to perform builds/diffs on all commits in
a specified range by passing the --all option. Other features added
include:
  * Perform local builds on commits in a subrepo (via --subrepo)
  * Store metadata and prevent unnecessary builds/downloads/diffs

BUG=710076
==========

to

==========
diagnose_apk_bloat.py: handle more rev options.

This CL introduces the ability to perform builds/diffs on all commits in
a specified range by passing the --all option. Other features added
include:
  * Perform local builds on commits in a subrepo (via --subrepo)
  * Store metadata and prevent unnecessary builds/downloads/diffs

BUG=710076
==========

[agrieve, 2017-04-21 16:57:31]

Overall like the structure you've chosen. A few recommendations though.

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
File tools/binary_size/diagnose_apk_bloat.py (right):

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:24: _ALLOWED_CONSECUTIVE_BUILDS = 15
Limit seems a bit small. I've seen rolls go in that have 100s of commits. I
think the main important thing is just to have a clear log message saying how
many commits will be built, and maybe a "hit enter to continue") when the number
of commits is large.

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:40: _subrepo = None
Having the global makes the code a bit harder to follow... How about instead
have a single global called _restore_checkout_func, that gets set to a lambda at
the start and will do the restoring when called.

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:239: _EnsureDirsExist(self.dir)
nit: Generally best to make constructors as light-weight as possible. Should
move any IO to a separate function.

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:358: if not metadata['is_cloud']:
Can you add a comment saying why this check is necessary.

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:390: def _GitCmd(args, retcode=False):
I don't see retcode being used anywhere. You also don't pass
exit_on_failure=False. If you do need this functionality, might be a nicer
interface to return None for an error.

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:562: help='Build all revs from
rev_with_patch to '
I've been finding the flags a bit unintuitive, I think mainly because "with
patch" does not always mean "commit comes after" in my head. It's also a bit
confusing that commits are built in reverse order (although makes sense when you
think about it).

I've also found it a bit surprising when running without flags does things.

Suggestion:

Add a single positional argument that is the commit range. E.g. HEAD,
ababab..232323, HEAD^^^^..HEAD

git rev-parse will return a single hash for HEAD, and return start/end for a
range. e.g.:

$ git rev-parse bfce95b70e45..5ed39274a52c
5ed39274a52c6e34e35e7a9c95aff9ef64a5ca32
^bfce95b70e45ee513b3735e27660fbecc1eeea5f

[estevenson, 2017-04-21 20:15:49]

ptal Andrew.

ps3 has all comments fixed except globals.

ps3 has a global _subrepo, ps4 has your suggestion, wdyt?

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
File tools/binary_size/diagnose_apk_bloat.py (right):

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:24: _ALLOWED_CONSECUTIVE_BUILDS = 15
On 2017/04/21 16:57:31, agrieve wrote:
> Limit seems a bit small. I've seen rolls go in that have 100s of commits. I
> think the main important thing is just to have a clear log message saying how
> many commits will be built, and maybe a "hit enter to continue") when the
number
> of commits is large.

Done.

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:239: _EnsureDirsExist(self.dir)
On 2017/04/21 16:57:31, agrieve wrote:
> nit: Generally best to make constructors as light-weight as possible. Should
> move any IO to a separate function.

Done.

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:358: if not metadata['is_cloud']:
On 2017/04/21 16:57:31, agrieve wrote:
> Can you add a comment saying why this check is necessary.

Done.

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:390: def _GitCmd(args, retcode=False):
On 2017/04/21 16:57:31, agrieve wrote:
> I don't see retcode being used anywhere. You also don't pass
> exit_on_failure=False. If you do need this functionality, might be a nicer
> interface to return None for an error.

Oops, meant to use it in _GenerateRevList. Changed _GenerateRevList to use
_RunCmd directly.

https://codereview.chromium.org/2834103002/diff/20001/tools/binary_size/diagn...
tools/binary_size/diagnose_apk_bloat.py:562: help='Build all revs from
rev_with_patch to '
On 2017/04/21 16:57:31, agrieve wrote:
> I've been finding the flags a bit unintuitive, I think mainly because "with
> patch" does not always mean "commit comes after" in my head. It's also a bit
> confusing that commits are built in reverse order (although makes sense when
you
> think about it).
> 
> I've also found it a bit surprising when running without flags does things.
> 
> Suggestion:
> 
> Add a single positional argument that is the commit range. E.g. HEAD,
> ababab..232323, HEAD^^^^..HEAD
> 
> git rev-parse will return a single hash for HEAD, and return start/end for a
> range. e.g.:
> 
> $ git rev-parse bfce95b70e45..5ed39274a52c
> 5ed39274a52c6e34e35e7a9c95aff9ef64a5ca32
> ^bfce95b70e45ee513b3735e27660fbecc1eeea5f

Spoke offline a bit about this, I've change it so that:
..bloat.py -> shows help
..bloat.py bfce95b70e45 -> builds/diffs bfce95b70e45^ and bfce95b70e45
..bloat.py bfce95b70e45 --reference-rev 5ed39274a52c -> builds/diffs
5ed39274a52c and bfce95b70e45
..bloat.py bfce95b70e45 --reference-rev 5ed39274a52c --all -> builds/diffs
5ed39274a52c..bfce95b70e45

Didn't update code references to --without and --with-patch since that's still
pretty clear to me (and using just rev would be confusing). Let me know if you'd
like me to.

[agrieve, 2017-04-21 20:39:48]

lgtm

[agrieve, 2017-04-21 20:40:08]

On 2017/04/21 20:39:48, agrieve wrote:
> lgtm

actually - please also update README. But lgtm.

[estevenson, 2017-04-21 21:28:08]

The CQ bit was checked by estevenson@chromium.org

[estevenson, 2017-04-21 21:28:08]

The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2834103002/#ps100001
(title: "add missing --cloud to readme")

[commit-bot: I haz the power, 2017-04-21 21:28:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 21:45:30]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492810088008120,
"parent_rev": "7cf10b073c07ab7eb0ea156fcf1f4918109837cc", "commit_rev":
"af790e8aa737385eb96d134ab18c475185671edc"}

[commit-bot: I haz the power, 2017-04-21 21:45:38]

Description was changed from

==========
diagnose_apk_bloat.py: handle more rev options.

This CL introduces the ability to perform builds/diffs on all commits in
a specified range by passing the --all option. Other features added
include:
  * Perform local builds on commits in a subrepo (via --subrepo)
  * Store metadata and prevent unnecessary builds/downloads/diffs

BUG=710076
==========

to

==========
diagnose_apk_bloat.py: handle more rev options.

This CL introduces the ability to perform builds/diffs on all commits in
a specified range by passing the --all option. Other features added
include:
  * Perform local builds on commits in a subrepo (via --subrepo)
  * Store metadata and prevent unnecessary builds/downloads/diffs

BUG=710076

Review-Url: https://codereview.chromium.org/2834103002
Cr-Commit-Position: refs/heads/master@{#466456}
Committed:
https://chromium.googlesource.com/chromium/src/+/af790e8aa737385eb96d134ab18c...
==========

[commit-bot: I haz the power, 2017-04-21 21:45:39]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/af790e8aa737385eb96d134ab18c...