CallAndRecordIfStale(): Add knownledge of output_files, depfiles, stamp

CallAndRecordIfStale(): Add knownledge of output_files, depfiles, stamp

Added output_paths to md5_check.py since it cuts down on
force=os.path.exists() boilder-plate (plus places that were forgetting
it). It also improves the message printed when PRINT_MD5_DIFFS=1 is set.

Added a wrapper in build_utils.py that automatically handles --depfile
and --stamp options, which further cuts down on boilerplate in the
clients.

Removed colorama to eliminate circular dependency between build_utils
and md5_check.

BUG=533442
Committed: https://crrev.com/dea0f30983bc18874ac8c49443fb5079d4c1b798
Cr-Commit-Position: refs/heads/master@{#350599}

[agrieve, 2015-09-18 15:06:19]

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

[jbudorick, 2015-09-18 18:19:38]

https://codereview.chromium.org/1356873003/diff/1/build/android/gyp/util/buil...
File build/android/gyp/util/build_utils.py (right):

https://codereview.chromium.org/1356873003/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_utils.py:405: def CallAndRecordIfStale(function,
options, record_path=None, input_paths=None,
curious: why'd you put this in here rather than in md5_check, and why would a
client call md5_check.CallAndRecordIfStale vs build_utils.CallAndRecordIfStale?

https://codereview.chromium.org/1356873003/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_utils.py:419: if hasattr(options, 'depfile') and
options.depfile:
I'm not crazy about locking option names across scripts, tbh.

[agrieve, 2015-09-18 18:46:19]

https://codereview.chromium.org/1356873003/diff/1/build/android/gyp/util/buil...
File build/android/gyp/util/build_utils.py (right):

https://codereview.chromium.org/1356873003/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_utils.py:405: def CallAndRecordIfStale(function,
options, record_path=None, input_paths=None,
On 2015/09/18 18:19:38, jbudorick wrote:
> curious: why'd you put this in here rather than in md5_check, and why would a
> client call md5_check.CallAndRecordIfStale vs
build_utils.CallAndRecordIfStale?

My reasoning:
- md5_check felt a bit like a generic module, builddeps is very
our-build-system-specific. 
- The purpose of md5_check.CallAndRecordIfStale is to check md5s. & the purpose
of this wrapper is just to eliminate copy & paste surrounding our usage of it.

I'm open to slicing it another way, but that was my thinking. You'd use
md5_check.py directly if you don't want the builddeps logic.

https://codereview.chromium.org/1356873003/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_utils.py:419: if hasattr(options, 'depfile') and
options.depfile:
On 2015/09/18 18:19:38, jbudorick wrote:
> I'm not crazy about locking option names across scripts, tbh.

All scripts already use build_utils.AddDepfileOption() to add this option, so
the name will be consistent.
As for .stamp, there's not a helper for it, but it's on the way out anyways (not
used in GN).

[jbudorick, 2015-09-23 14:56:37]

https://codereview.chromium.org/1356873003/diff/1/build/android/gyp/util/buil...
File build/android/gyp/util/build_utils.py (right):

https://codereview.chromium.org/1356873003/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_utils.py:405: def CallAndRecordIfStale(function,
options, record_path=None, input_paths=None,
On 2015/09/18 at 18:46:19, agrieve wrote:
> On 2015/09/18 18:19:38, jbudorick wrote:
> > curious: why'd you put this in here rather than in md5_check, and why would
a
> > client call md5_check.CallAndRecordIfStale vs
build_utils.CallAndRecordIfStale?
> 
> My reasoning:
> - md5_check felt a bit like a generic module, builddeps is very
our-build-system-specific. 
> - The purpose of md5_check.CallAndRecordIfStale is to check md5s. & the
purpose of this wrapper is just to eliminate copy & paste surrounding our usage
of it.
> 
> I'm open to slicing it another way, but that was my thinking. You'd use
md5_check.py directly if you don't want the builddeps logic.

I'm concerned about:
 - having two identically-named functions where one does A and the other does
A+B confusing people, myself included
 - having the former function only used by the latter, in which case they
probably don't need to be separate (or at least don't need to both be publicly
visible)

https://codereview.chromium.org/1356873003/diff/20001/build/android/gyp/dex.py
File build/android/gyp/dex.py (right):

https://codereview.chromium.org/1356873003/diff/20001/build/android/gyp/dex.p...
build/android/gyp/dex.py:17: def
_CreateCombinedMainDexList(main_dex_list_paths):
I guess it's my turn to have an interesting rebase.

https://codereview.chromium.org/1356873003/diff/20001/build/android/gyp/dex.p...
build/android/gyp/dex.py:90: combined_main_dex_list =
tempfile.NamedTemporaryFile(suffix='.txt')
This leaves the tempfile after dexing, which I'd rather avoid.

[agrieve, 2015-09-23 17:51:49]

Renamed the wrapper to "CallAndWriteDepfileIfStale". Hopefully that'll make the
distinction more clear.

https://codereview.chromium.org/1356873003/diff/20001/build/android/gyp/dex.py
File build/android/gyp/dex.py (right):

https://codereview.chromium.org/1356873003/diff/20001/build/android/gyp/dex.p...
build/android/gyp/dex.py:90: combined_main_dex_list =
tempfile.NamedTemporaryFile(suffix='.txt')
On 2015/09/23 14:56:37, jbudorick wrote:
> This leaves the tempfile after dexing, which I'd rather avoid.

NamedTemporaryFile hooks into the runtime to delete upon exit.

[jbudorick, 2015-09-24 15:16:48]

Different function names should be fine.

https://codereview.chromium.org/1356873003/diff/40001/build/android/gyp/util/...
File build/android/gyp/util/md5_check.py (right):

https://codereview.chromium.org/1356873003/diff/40001/build/android/gyp/util/...
build/android/gyp/util/md5_check.py:44: record_path = record_path or
output_paths[0] + '.md5.stamp'
This looks like it doesn't work with the default parameters. output_paths will
be [], so output_paths[0] won't exist.

https://codereview.chromium.org/1356873003/diff/40001/build/android/gyp/util/...
build/android/gyp/util/md5_check.py:121: for s in map(str, input_strings):
nit: we tend to avoid map, filter, etc in favor of comprehensions, so this
should be

  for s in (str(i) for i in input_strings):

I think there's a lint error about this, but I'm not sure if we have it enabled
in b/a/gyp/

[agrieve, 2015-09-24 15:40:44]

https://codereview.chromium.org/1356873003/diff/40001/build/android/gyp/util/...
File build/android/gyp/util/md5_check.py (right):

https://codereview.chromium.org/1356873003/diff/40001/build/android/gyp/util/...
build/android/gyp/util/md5_check.py:44: record_path = record_path or
output_paths[0] + '.md5.stamp'
On 2015/09/24 15:16:48, jbudorick wrote:
> This looks like it doesn't work with the default parameters. output_paths will
> be [], so output_paths[0] won't exist.

Right. I want an exception to be thrown in this case. Think it'd be better as an
assert? (added one)

https://codereview.chromium.org/1356873003/diff/40001/build/android/gyp/util/...
build/android/gyp/util/md5_check.py:121: for s in map(str, input_strings):
On 2015/09/24 15:16:48, jbudorick wrote:
> nit: we tend to avoid map, filter, etc in favor of comprehensions, so this
> should be
> 
>   for s in (str(i) for i in input_strings):
> 
> I think there's a lint error about this, but I'm not sure if we have it
enabled
> in b/a/gyp/

Agree that reads better :) Done.

[jbudorick, 2015-09-24 15:42:57]

lgtm

https://codereview.chromium.org/1356873003/diff/40001/build/android/gyp/util/...
File build/android/gyp/util/md5_check.py (right):

https://codereview.chromium.org/1356873003/diff/40001/build/android/gyp/util/...
build/android/gyp/util/md5_check.py:44: record_path = record_path or
output_paths[0] + '.md5.stamp'
On 2015/09/24 at 15:40:44, agrieve wrote:
> On 2015/09/24 15:16:48, jbudorick wrote:
> > This looks like it doesn't work with the default parameters. output_paths
will
> > be [], so output_paths[0] won't exist.
> 
> Right. I want an exception to be thrown in this case. Think it'd be better as
an assert? (added one)

Yes, an assert makes it clear that it's intentional.

[agrieve, 2015-09-24 16:08:04]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-09-24 16:08:27]

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

[commit-bot: I haz the power, 2015-09-24 17:48:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-24 17:49:31]

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

[stgao, 2015-09-24 19:15:08]

stgao@chromium.org changed reviewers:
+ stgao@chromium.org

[stgao, 2015-09-24 19:15:08]

FYI:

This CL seems to have broken
http://build.chromium.org/p/chromium/builders/Android
http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%...

[dtu, 2015-09-24 19:22:53]

On 2015/09/24 19:15:08, Shuotao wrote:
> FYI:
> 
> This CL seems to have broken
> http://build.chromium.org/p/chromium/builders/Android
>
http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%...

Here, as well:
http://build.chromium.org/p/chromium.perf/builders/Android%20arm64%20Builder/...

[Cait (Slow), 2015-09-24 19:37:20]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1369583002/ by caitkp@chromium.org.

The reason for reverting is: broke Android build (see other messages for
details)..

[agrieve, 2015-09-24 20:04:04]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1362113003/ by agrieve@chromium.org.

The reason for reverting is: Fixed import path and ensure "ninja all" builds
locally.