Give build a --write-only-new option.

Give build a --write-only-new option.

If that's set, build will only change the mtime on files with new contents.
That allows ninja's restat logic to fire.

BUG=chromium:439182
R=newt@chromium.org

Committed: https://code.google.com/p/grit-i18n/source/detail?r=183

[Nico, 2014-12-04 21:28:37]

thakis@chromium.org changed reviewers:
+ newt@chromium.org

[Nico, 2014-12-04 22:09:33]

Please click through, I uploaded a 2nd patch set that tweaks this slightly.

[newt (away), 2014-12-04 22:09:42]

I'm all for reducing unnecessary rebuilding :)

What's the plan for using this in Chromium?

https://codereview.chromium.org/757213003/diff/1/grit/tool/build.py
File grit/tool/build.py (right):

https://codereview.chromium.org/757213003/diff/1/grit/tool/build.py#newcode112
grit/tool/build.py:112: --write-only-new  Write output files to a temporary file
first, and copy
Does this also need to be added to grit_info.py as an unused argument so that
calls to grit_info.py will succeed? E.g. I believe chrome/policy_templates.gypi
calls grit_info.py with the same arguments that it calls grit.py.

https://codereview.chromium.org/757213003/diff/1/grit/tool/build.py#newcode358
grit/tool/build.py:358: # This clashes with gyp + vstudio, which expect the
output timestamp
If this is still an issue when using visual studio, are we going to pass
--write-only-new by default, or on non-windows platforms, or when using GN?
What's the plan? :)

https://codereview.chromium.org/757213003/diff/1/grit/tool/build.py#newcode368
grit/tool/build.py:368: shutil.copy2(output.GetOutputFilename() + '.tmp',
Seems like it would be quicker to delete output.h, then move the output.h.tmp to
output.h, rather than copying file contents around. Any idea why this uses a
copy and delete instead?

https://codereview.chromium.org/757213003/diff/1/grit/tool/build_unittest.py
File grit/tool/build_unittest.py (right):

https://codereview.chromium.org/757213003/diff/1/grit/tool/build_unittest.py#...
grit/tool/build_unittest.py:264: time.sleep(mtime_resolution)
Is there a way do this without sleeping for 1 or 2 seconds? The entire grit test
suite runs in 1.3 seconds on my machine, so it's a bit sad for this one test to
take 2 or 4 seconds.

Idea: set the mtime of the file before each call to builder.Run(), then check if
the mtime was updated. Or you could try patching the time or datetime module
(http://stackoverflow.com/a/2659472/598094), or you could add a hook in grit.py.

[Nico, 2014-12-04 22:33:07]

Thanks!

https://codereview.chromium.org/757213003/diff/1/grit/tool/build.py
File grit/tool/build.py (right):

https://codereview.chromium.org/757213003/diff/1/grit/tool/build.py#newcode112
grit/tool/build.py:112: --write-only-new  Write output files to a temporary file
first, and copy
On 2014/12/04 22:09:42, newt wrote:
> Does this also need to be added to grit_info.py as an unused argument so that
> calls to grit_info.py will succeed? E.g. I believe
chrome/policy_templates.gypi
> calls grit_info.py with the same arguments that it calls grit.py.

I think policy_templates only passes the same defines, not the same flags.
buildinfo.py doesn't look at any of the other flags here either.

https://codereview.chromium.org/757213003/diff/1/grit/tool/build.py#newcode358
grit/tool/build.py:358: # This clashes with gyp + vstudio, which expect the
output timestamp
On 2014/12/04 22:09:42, newt wrote:
> If this is still an issue when using visual studio, are we going to pass
> --write-only-new by default, or on non-windows platforms, or when using GN?
> What's the plan? :)

We don't support visual studio anymore, but maybe it's still an issue for ios.

https://codereview.chromium.org/740463005 is the plan, it checks GENERATOR and
only enables it for ninja builds.

https://codereview.chromium.org/757213003/diff/1/grit/tool/build.py#newcode368
grit/tool/build.py:368: shutil.copy2(output.GetOutputFilename() + '.tmp',
On 2014/12/04 22:09:42, newt wrote:
> Seems like it would be quicker to delete output.h, then move the output.h.tmp
to
> output.h, rather than copying file contents around. Any idea why this uses a
> copy and delete instead?

That's the usual way of doing things so that if you interrupt the command you
don't end up with a partially-written file. (The usual usual way is to do rm &&
mv instead of cp, that way it's not even slower.)

https://codereview.chromium.org/757213003/diff/1/grit/tool/build_unittest.py
File grit/tool/build_unittest.py (right):

https://codereview.chromium.org/757213003/diff/1/grit/tool/build_unittest.py#...
grit/tool/build_unittest.py:264: time.sleep(mtime_resolution)
On 2014/12/04 22:09:42, newt wrote:
> Is there a way do this without sleeping for 1 or 2 seconds? The entire grit
test
> suite runs in 1.3 seconds on my machine, so it's a bit sad for this one test
to
> take 2 or 4 seconds.

Yeah, I was pretty unhappy with this too :-/

> Idea: set the mtime of the file before each call to builder.Run(), then check
if
> the mtime was updated.

That's a great idea!

> Or you could try patching the time or datetime module
> (http://stackoverflow.com/a/2659472/598094), or you could add a hook in
grit.py.

[newt (away), 2014-12-04 22:45:22]

lgtm with a question

https://codereview.chromium.org/757213003/diff/60001/grit/tool/build_unittest.py
File grit/tool/build_unittest.py (right):

https://codereview.chromium.org/757213003/diff/60001/grit/tool/build_unittest...
grit/tool/build_unittest.py:274: self.assertTrue(abs(second_mtime - first_mtime)
< 5)
What's the purpose of this line? It seems only to be checking that the host
machine isn't extremely slow.

[Nico, 2014-12-04 22:47:50]

Thanks!

https://codereview.chromium.org/757213003/diff/60001/grit/tool/build_unittest.py
File grit/tool/build_unittest.py (right):

https://codereview.chromium.org/757213003/diff/60001/grit/tool/build_unittest...
grit/tool/build_unittest.py:274: self.assertTrue(abs(second_mtime - first_mtime)
< 5)
On 2014/12/04 22:45:22, newt wrote:
> What's the purpose of this line? It seems only to be checking that the host
> machine isn't extremely slow.

Yeah, I guess it's not very different from the line above it. I'll remove it.

[Nico, 2014-12-04 22:48:17]

Committed patchset #4 (id:60001) manually as r183 (presubmit successful).