[Mac/iOS/GN] Place a version in the command of toolchain tools implemented using scripts.

[Mac/iOS/GN] Place a version in the command of toolchain tools implemented using scripts.

When a tool is implemented as a script, modification of the script does not
cause any dependent build steps to be dirtied and rebuilt, since the script
isn't listed as an input. To hack around this, use another script to get the
tool script's modification time and use that as the command line tool version.
This still won't cause rebuild if just the script changes, but when the build
files are regenerated (like after apply_patch and generate_build_files on the
bots), the dependent build steps will get rebuilt.

BUG=619083
R=brettw@chromium.org, sdefresne@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/f2eb108ab1e65962104bfde275743369b0e7665b

[sdefresne, 2016-06-16 18:15:25]

https://codereview.chromium.org/2075703002/diff/1/build/toolchain/mac/BUILD.gn
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/2075703002/diff/1/build/toolchain/mac/BUILD.g...
build/toolchain/mac/BUILD.gn:154: tool_version = exec_script(get_tool_mtime, [
script ], "trim string")
I'm not really sure about using exec_script because it mean that if I change the
script, but not any BUILD.gn file, ninja will not re-run "gn gen" and thus not
regenerate the build files, the command-line will not be updated and thus the
TOOL_VERSION value unchanged ...

So, sadly I need that if we go that route we need to manually update the
TOOL_VERSION (i.e. even more important to update gn to turn those dependencies
in real dependencies).

[Robert Sesek, 2016-06-16 18:26:16]

https://codereview.chromium.org/2075703002/diff/1/build/toolchain/mac/BUILD.gn
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/2075703002/diff/1/build/toolchain/mac/BUILD.g...
build/toolchain/mac/BUILD.gn:154: tool_version = exec_script(get_tool_mtime, [
script ], "trim string")
On 2016/06/16 18:15:25, sdefresne wrote:
> I'm not really sure about using exec_script because it mean that if I change
the
> script, but not any BUILD.gn file, ninja will not re-run "gn gen" and thus not
> regenerate the build files, the command-line will not be updated and thus the
> TOOL_VERSION value unchanged ...

Right. I wrote that in both the comment above and the CL description ;)

> So, sadly I need that if we go that route we need to manually update the
> TOOL_VERSION (i.e. even more important to update gn to turn those dependencies
> in real dependencies).

I think this is better than manually modifying the version. I don't trust that I
or others will remember to bump this; the only proper fix is to have tool
scripts properly specified as an input. I think this approach is better than
manual version because this will at least catch issues on the bots without
requiring a manual version bump. And locally there's not much difference between
manually modifying the version number or manually re-running `gn gen`.

[brettw, 2016-06-17 17:29:43]

https://codereview.chromium.org/2075703002/diff/1/build/toolchain/mac/BUILD.gn
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/2075703002/diff/1/build/toolchain/mac/BUILD.g...
build/toolchain/mac/BUILD.gn:62: # When implementing tools using Python scripts,
a TOOL_VERSION=N env
I just filed https://bugs.chromium.org/p/chromium/issues/detail?id=621119 for
this, can you reference that bug in a TODO here?

https://codereview.chromium.org/2075703002/diff/1/build/toolchain/mac/BUILD.g...
build/toolchain/mac/BUILD.gn:306: _tool_version = exec_script(get_tool_mtime, [
_tool ], "trim string")
It looks like the shortest exec_script time I see on my Mac is 42ms (probably
process launch overhead) . This file is already the long pole for the build and
delays all file writing (all other build files are loaded in less time than this
file takes to execute).

Then these calls are inside the template which is executed 7 times, so I'm
getting this patch will make GN runs 1.1 seconds slower

So you adding 3 calls to exec script will make GN runs >120ms slower every time.

Can we express this instead as one script call at the top of the file that takes
all the scripts as arguments and returns the modification times in one
exec_script call?

[Robert Sesek, 2016-06-17 18:07:53]

https://codereview.chromium.org/2075703002/diff/1/build/toolchain/mac/BUILD.gn
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/2075703002/diff/1/build/toolchain/mac/BUILD.g...
build/toolchain/mac/BUILD.gn:62: # When implementing tools using Python scripts,
a TOOL_VERSION=N env
On 2016/06/17 17:29:43, brettw wrote:
> I just filed https://bugs.chromium.org/p/chromium/issues/detail?id=621119 for
> this, can you reference that bug in a TODO here?

Done.

https://codereview.chromium.org/2075703002/diff/1/build/toolchain/mac/BUILD.g...
build/toolchain/mac/BUILD.gn:306: _tool_version = exec_script(get_tool_mtime, [
_tool ], "trim string")
On 2016/06/17 17:29:43, brettw wrote:
> It looks like the shortest exec_script time I see on my Mac is 42ms (probably
> process launch overhead) . This file is already the long pole for the build
and
> delays all file writing (all other build files are loaded in less time than
this
> file takes to execute).
> 
> Then these calls are inside the template which is executed 7 times, so I'm
> getting this patch will make GN runs 1.1 seconds slower
> 
> So you adding 3 calls to exec script will make GN runs >120ms slower every
time.
> 
> Can we express this instead as one script call at the top of the file that
takes
> all the scripts as arguments and returns the modification times in one
> exec_script call?

Done. Some numbers for reference:

Before this change: 1505ms
Old patchet (exec_script for each tool version): 3431ms
Latest patch set: 1568ms

[brettw, 2016-06-17 20:43:30]

lgtm

https://codereview.chromium.org/2075703002/diff/20001/build/toolchain/mac/BUI...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/2075703002/diff/20001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:73: exec_script("get_tool_mtime.py",
Can you move this up above the template() definition? This is still getting
called more than once for inputs that are constant.

[Robert Sesek, 2016-06-17 20:53:32]

https://codereview.chromium.org/2075703002/diff/20001/build/toolchain/mac/BUI...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/2075703002/diff/20001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:73: exec_script("get_tool_mtime.py",
On 2016/06/17 20:43:30, brettw wrote:
> Can you move this up above the template() definition? This is still getting
> called more than once for inputs that are constant.

Done.

[commit-bot: I haz the power, 2016-06-17 22:05:21]

Description was changed from

==========
[Mac/iOS/GN] Place a version in the command of toolchain tools implemented using
scripts.

When a tool is implemented as a script, modification of the script does not
cause any dependent build steps to be dirtied and rebuilt, since the script
isn't listed as an input. To hack around this, use another script to get the
tool script's modification time and use that as the command line tool version.
This still won't cause rebuild if just the script changes, but when the build
files are regenerated (like after apply_patch and generate_build_files on the
bots), the dependent build steps will get rebuilt.

BUG=619083
R=brettw@chromium.org,sdefresne@chromium.org
==========

to

==========
[Mac/iOS/GN] Place a version in the command of toolchain tools implemented using
scripts.

When a tool is implemented as a script, modification of the script does not
cause any dependent build steps to be dirtied and rebuilt, since the script
isn't listed as an input. To hack around this, use another script to get the
tool script's modification time and use that as the command line tool version.
This still won't cause rebuild if just the script changes, but when the build
files are regenerated (like after apply_patch and generate_build_files on the
bots), the dependent build steps will get rebuilt.

BUG=619083
R=brettw@chromium.org, sdefresne@chromium.org

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

[commit-bot: I haz the power, 2016-06-17 22:05:22]

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

[Robert Sesek, 2016-06-17 22:06:47]

Description was changed from

==========
[Mac/iOS/GN] Place a version in the command of toolchain tools implemented using
scripts.

When a tool is implemented as a script, modification of the script does not
cause any dependent build steps to be dirtied and rebuilt, since the script
isn't listed as an input. To hack around this, use another script to get the
tool script's modification time and use that as the command line tool version.
This still won't cause rebuild if just the script changes, but when the build
files are regenerated (like after apply_patch and generate_build_files on the
bots), the dependent build steps will get rebuilt.

BUG=619083
R=brettw@chromium.org, sdefresne@chromium.org

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

to

==========
[Mac/iOS/GN] Place a version in the command of toolchain tools implemented using
scripts.

When a tool is implemented as a script, modification of the script does not
cause any dependent build steps to be dirtied and rebuilt, since the script
isn't listed as an input. To hack around this, use another script to get the
tool script's modification time and use that as the command line tool version.
This still won't cause rebuild if just the script changes, but when the build
files are regenerated (like after apply_patch and generate_build_files on the
bots), the dependent build steps will get rebuilt.

BUG=619083
R=brettw@chromium.org, sdefresne@chromium.org

Committed:
https://chromium.googlesource.com/chromium/src/+/f2eb108ab1e65962104bfde27574...
==========

[Robert Sesek, 2016-06-17 22:06:49]

Committed patchset #3 (id:40001) manually as
f2eb108ab1e65962104bfde275743369b0e7665b (presubmit successful).