MSVS: Fix to ensure that actions are re-run when the action command changes

MSVS: Fix to ensure that actions are re-run when the action command changes

There was some code to re-run an action, but it only worked:

 1) If the command change came from a change to a .gyp file.  It
    didn't work if the command change came from a change to a .gypi
    file.  Fix this by having a dependency on the generated .vcproj
    file instead of on the .gyp file.

 2) If the target had an empty "inputs" list.  Fix this by adding the
    dependency unconditionally.

BUG=http://code.google.com/p/gyp/issues/detail?id=262
TEST=test/actions-args-change

[Mark Seaborn, 2012-05-02 22:40:41]

I'm looking into the trybot failures, but I think they are pre-existing failures
and not caused by this change.  Certainly the new test passed everywhere.

[scottmg, 2012-05-02 22:44:25]

On 2012/05/02 22:40:41, Mark Seaborn wrote:
> I'm looking into the trybot failures, but I think they are pre-existing
failures
> and not caused by this change.

They don't look familiar, not impossible though.

[Ryan Sleevi, 2012-05-02 22:57:33]

On the upside, it looks like this might fix the Webkit-requires-clobber issue
when WebKit feature flags change (
http://code.google.com/p/chromium/issues/detail?id=85789 ), but it seems to have
the risk of invaliding all parts of the 'webcore_bindings', rather than just
specific targets, thus forcing much more WebKit rebuilding.

At the risk of asking for a pony, could you first check and see if/how often
that will come up, and if/how this change would impact the Chrome buildtimes
between WebKit rolls?

[jeanluc1, 2012-05-02 23:13:50]

https://chromiumcodereview.appspot.com/10306013/diff/9/pylib/gyp/generator/ms...
File pylib/gyp/generator/msvs.py (right):

https://chromiumcodereview.appspot.com/10306013/diff/9/pylib/gyp/generator/ms...
pylib/gyp/generator/msvs.py:2916: _AddActions(actions_to_add, spec,
project.path)
I'm not sure if there's a reason why MSVS uses relative_path_of_vcproj_file and
MSBuild uses project.path

https://chromiumcodereview.appspot.com/10306013/diff/9/test/actions-args-chan...
File test/actions-args-change/gyptest-all.py (right):

https://chromiumcodereview.appspot.com/10306013/diff/9/test/actions-args-chan...
test/actions-args-change/gyptest-all.py:42: WriteFile(gypi_file,
data.replace('ARG_VALUE_1', 'ARG_VALUE_2'))
Are you sure you'll be able to overwrite this file?  Could be set as read-only
in some case as it is extracted from the version control system?  I'd be more
comfortable if you write the file twice (as _1 and as _2) to a path different
than what is in the version control system.

[Mark Seaborn, 2012-05-02 23:38:32]

https://chromiumcodereview.appspot.com/10306013/diff/9/pylib/gyp/generator/ms...
File pylib/gyp/generator/msvs.py (right):

https://chromiumcodereview.appspot.com/10306013/diff/9/pylib/gyp/generator/ms...
pylib/gyp/generator/msvs.py:2916: _AddActions(actions_to_add, spec,
project.path)
On 2012/05/02 23:13:52, jeanluc1 wrote:
> I'm not sure if there's a reason why MSVS uses relative_path_of_vcproj_file
> and MSBuild uses project.path

I have no idea.  I don't have MSVS 2010 installed locally so I can only test it
via the trybots.

https://chromiumcodereview.appspot.com/10306013/diff/9/test/actions-args-chan...
File test/actions-args-change/gyptest-all.py (right):

https://chromiumcodereview.appspot.com/10306013/diff/9/test/actions-args-chan...
test/actions-args-change/gyptest-all.py:42: WriteFile(gypi_file,
data.replace('ARG_VALUE_1', 'ARG_VALUE_2'))
On 2012/05/02 23:13:52, jeanluc1 wrote:
> Are you sure you'll be able to overwrite this file?  Could be set as read-only
> in some case as it is extracted from the version control system?  I'd be more
> comfortable if you write the file twice (as _1 and as _2) to a path different
> than what is in the version control system.

You mean you're concerned that the test will fail if you do

  chmod -w test/actions-args-change/src/arg-action.gypi

beforehand, because it's copying the permission bits?  It does indeed fail on
Linux if I do that.

It seems rather obnoxious to copy the permission bits like this in a test,
though.  Maybe I should change TestGyp not to do that, and only copy the
executable bit?

[jeanluc1, 2012-05-02 23:50:24]

https://chromiumcodereview.appspot.com/10306013/diff/9/pylib/gyp/generator/ms...
File pylib/gyp/generator/msvs.py (right):

https://chromiumcodereview.appspot.com/10306013/diff/9/pylib/gyp/generator/ms...
pylib/gyp/generator/msvs.py:2916: _AddActions(actions_to_add, spec,
project.path)
If the 2010 trybot is happy, that's sufficient for me (for now).

On 2012/05/02 23:38:32, Mark Seaborn wrote:
> On 2012/05/02 23:13:52, jeanluc1 wrote:
> > I'm not sure if there's a reason why MSVS uses relative_path_of_vcproj_file
> > and MSBuild uses project.path
> 
> I have no idea.  I don't have MSVS 2010 installed locally so I can only test
it
> via the trybots.

https://chromiumcodereview.appspot.com/10306013/diff/9/test/actions-args-chan...
File test/actions-args-change/gyptest-all.py (right):

https://chromiumcodereview.appspot.com/10306013/diff/9/test/actions-args-chan...
test/actions-args-change/gyptest-all.py:42: WriteFile(gypi_file,
data.replace('ARG_VALUE_1', 'ARG_VALUE_2'))
I was concerned that some version control systems by default copy file as
read-only until you specify that you want to edit the file.

On 2012/05/02 23:38:32, Mark Seaborn wrote:
> On 2012/05/02 23:13:52, jeanluc1 wrote:
> > Are you sure you'll be able to overwrite this file?  Could be set as
read-only
> > in some case as it is extracted from the version control system?  I'd be
more
> > comfortable if you write the file twice (as _1 and as _2) to a path
different
> > than what is in the version control system.
> 
> You mean you're concerned that the test will fail if you do
> 
>   chmod -w test/actions-args-change/src/arg-action.gypi
> 
> beforehand, because it's copying the permission bits?  It does indeed fail on
> Linux if I do that.
> 
> It seems rather obnoxious to copy the permission bits like this in a test,
> though.  Maybe I should change TestGyp not to do that, and only copy the
> executable bit?

https://chromiumcodereview.appspot.com/10306013/diff/14001/pylib/gyp/generato...
File pylib/gyp/generator/msvs.py (right):

https://chromiumcodereview.appspot.com/10306013/diff/14001/pylib/gyp/generato...
pylib/gyp/generator/msvs.py:1454: inputs = a.get('inputs') or
[relative_path_of_gyp_file]
I'm curious... what was the problem with only using
relative_path_of_project_file?

[Mark Seaborn, 2012-05-03 00:39:20]

https://chromiumcodereview.appspot.com/10306013/diff/9/test/actions-args-chan...
File test/actions-args-change/gyptest-all.py (right):

https://chromiumcodereview.appspot.com/10306013/diff/9/test/actions-args-chan...
test/actions-args-change/gyptest-all.py:42: WriteFile(gypi_file,
data.replace('ARG_VALUE_1', 'ARG_VALUE_2'))
On 2012/05/02 23:50:24, jeanluc1 wrote:
> I was concerned that some version control systems by default copy file as
> read-only until you specify that you want to edit the file.

Sure, I see what you mean.  Do you want me to fix the test to work in that case?
 It seems like the most general way would be to ensure that we write the file
with the writable permission bit set in copy_test_configuration() in TestGyp.py?

https://chromiumcodereview.appspot.com/10306013/diff/14001/pylib/gyp/generato...
File pylib/gyp/generator/msvs.py (right):

https://chromiumcodereview.appspot.com/10306013/diff/14001/pylib/gyp/generato...
pylib/gyp/generator/msvs.py:1454: inputs = a.get('inputs') or
[relative_path_of_gyp_file]
On 2012/05/02 23:50:24, jeanluc1 wrote:
> I'm curious... what was the problem with only using
> relative_path_of_project_file?

The trybot failures were caused by my change after all.  The original version of
the change in patchset 1 failed in the case where an action's "inputs" field is
empty, which was covered by test\intermediate_dir\gyptest-intermediate-dir.py.

The generator has to choose an input file for the action to hang off of
(inputs[0] is picked in _AddActionStep()), and the chosen file has to be a
target known by the check in AddFileConfig(), otherwise it fails like this:

Traceback (most recent call last):
  File "E:\b\build\slave\gyp\build\trunk\/gyp", line 18, in <module>
    sys.exit(gyp.main(sys.argv[1:]))
  File "E:\b\build\slave\gyp\build\trunk\pylib\gyp\__init__.py", line 480, in
main
    generator.GenerateOutput(flat_list, targets, data, params)
  File "E:\b\build\slave\gyp\build\trunk\pylib\gyp\generator\msvs.py", line
1788, in GenerateOutput
    generator_flags))
  File "E:\b\build\slave\gyp\build\trunk\pylib\gyp\generator\msvs.py", line 858,
in _GenerateProject
    return _GenerateMSVSProject(project, options, version, generator_flags)
  File "E:\b\build\slave\gyp\build\trunk\pylib\gyp\generator\msvs.py", line 918,
in _GenerateMSVSProject
    _AddAccumulatedActionsToMSVS(p, spec, actions_to_add)
  File "E:\b\build\slave\gyp\build\trunk\pylib\gyp\generator\msvs.py", line 418,
in _AddAccumulatedActionsToMSVS
    cmd=command)
  File "E:\b\build\slave\gyp\build\trunk\pylib\gyp\generator\msvs.py", line 388,
in _AddCustomBuildToolForMSVS
    _ConfigFullName(config_name, c_data), tools=[tool])
  File "E:\b\build\slave\gyp\build\trunk\pylib\gyp\MSVSProject.py", line 181, in
AddFileConfig
    raise ValueError('AddFileConfig: file "%s" not in project.' % path)
ValueError: AddFileConfig: file "target1.vcproj" not in project.

[Mark Seaborn, 2012-05-03 16:54:14]

On 2012/05/02 22:57:33, Ryan Sleevi wrote:
> On the upside, it looks like this might fix the
> Webkit-requires-clobber issue when WebKit feature flags change (
> http://code.google.com/p/chromium/issues/detail?id=85789 ), but it
> seems to have the risk of invaliding all parts of the
> 'webcore_bindings', rather than just specific targets, thus forcing
> much more WebKit rebuilding.

Wow, you've been living with having to do clobber builds manually for
a while.  I balked at the idea of doing a whitespace change to a .cc
file in order to commit http://codereview.chromium.org/10221026/ :-)

Thanks for linking to that bug.  Maruel's comment "I prefer the
following solution: Move to VS2010" suggested that VS2010 handles
re-running changed actions automatically.  And it turned out that the
Gyp test I'm adding passed (on the Gyp bots) for VS2010 without the
rest of the change.  So I've made the msvs.py change apply to VS2008
only.


> At the risk of asking for a pony, could you first check and see
> if/how often that will come up, and if/how this change would impact
> the Chrome buildtimes between WebKit rolls?

Should I just try rebuilding across a WebKit DEPS change with and
without this Gyp change?

Mark

[jeanluc1, 2012-05-03 17:12:44]

I'm glad you verified that VS2010 was handling correctly.  I'm fine with this
change.

https://chromiumcodereview.appspot.com/10306013/diff/20002/pylib/gyp/generato...
File pylib/gyp/generator/msvs.py (right):

https://chromiumcodereview.appspot.com/10306013/diff/20002/pylib/gyp/generato...
pylib/gyp/generator/msvs.py:1455: if relative_path_of_vcproj_file is not None:
if relative_path_of_vcproj_file:

is sufficient

https://chromiumcodereview.appspot.com/10306013/diff/20002/pylib/gyp/generato...
pylib/gyp/generator/msvs.py:2926: relative_path_of_vcproj_file=None)
I would just add
  , None
This way, while more explicit, makes me think there are yet other parameters.

[Ryan Sleevi, 2012-05-03 17:41:57]

On 2012/05/03 16:54:14, Mark Seaborn wrote:
> On 2012/05/02 22:57:33, Ryan Sleevi wrote:
> > On the upside, it looks like this might fix the
> > Webkit-requires-clobber issue when WebKit feature flags change (
> > http://code.google.com/p/chromium/issues/detail?id=85789 ), but it
> > seems to have the risk of invaliding all parts of the
> > 'webcore_bindings', rather than just specific targets, thus forcing
> > much more WebKit rebuilding.
> 
> Wow, you've been living with having to do clobber builds manually for
> a while.  I balked at the idea of doing a whitespace change to a .cc
> file in order to commit http://codereview.chromium.org/10221026/ :-)
> 
> Thanks for linking to that bug.  Maruel's comment "I prefer the
> following solution: Move to VS2010" suggested that VS2010 handles
> re-running changed actions automatically.  And it turned out that the
> Gyp test I'm adding passed (on the Gyp bots) for VS2010 without the
> rest of the change.  So I've made the msvs.py change apply to VS2008
> only.
> 
> 
> > At the risk of asking for a pony, could you first check and see
> > if/how often that will come up, and if/how this change would impact
> > the Chrome buildtimes between WebKit rolls?
> 
> Should I just try rebuilding across a WebKit DEPS change with and
> without this Gyp change?
> 
> Mark

Yeah. I think that'd be a fair test, mod disk-cache and the like. For compiler
changes on the Chromium side, tend to do 3x-5x compiles with a change to
normalize for disk cache and the like. Which is a real bother when you're trying
a WebKit roll =)

The test being Chromium revA, WebKit revX -> Chromium revA, WebKit revY with and
without the change.

[jeanluc1, 2012-05-03 20:37:56]

lgtm

[Mark Seaborn, 2012-05-04 20:14:37]

On 2012/05/02 22:57:33, Ryan Sleevi wrote:
> On the upside, it looks like this might fix the
> Webkit-requires-clobber issue when WebKit feature flags change (
> http://code.google.com/p/chromium/issues/detail?id=85789 ), but it
> seems to have the risk of invaliding all parts of the
> 'webcore_bindings', rather than just specific targets, thus forcing
> much more WebKit rebuilding.
>
> At the risk of asking for a pony, could you first check and see
> if/how often that will come up, and if/how this change would impact
> the Chrome buildtimes between WebKit rolls?

OK, I did an experiment to test the impact of this change.

Firstly, excessive rebuilding should only occur for Gyp targets that contain
multiple actions.  We can find those by searching for .vcproj files that contain
many 'VCCustomBuildTool' strings.  These Gyp targets are the top offenders:

624 ./third_party/WebKit/Source/WebKit/chromium/inspector_resources.vcproj
228 ./chrome/packed_resources.vcproj
134 ./third_party/WebKit/Source/WebCore/WebCore.vcproj/WebCore.vcproj
84 ./third_party/WebKit/Source/WebKit/chromium/concatenated_devtools_css.vcproj
80 ./third_party/protobuf/py_proto.vcproj
64 ./native_client/copy_headers.vcproj
52
./third_party/WebKit/Source/WebCore/WebCore.gyp/webcore_bindings_sources.vcproj
28 ./third_party/WebKit/Source/Platform/Platform.gyp/webkit_platform.vcproj
24 ./chrome/browser_tests.vcproj
16 ./third_party/yasm/yasm.vcproj
14 ./third_party/WebKit/Tools/DumpRenderTree/win/DumpRenderTree.vcproj
12 ./third_party/ffmpeg/ffmpeg.vcproj
12 ./third_party/angle/src/compiler/translator_common.vcproj
12 ./chrome/policy_templates.vcproj
10 ./third_party/WebKit/Source/WebKit/win/WebKit.vcproj/WebKit.vcproj
8 ./webkit/support/webkit_support.vcproj
8 ./third_party/yasm/generate_files.vcproj
8 ./third_party/WebKit/Source/WebKit2/win/WebKit2.vcproj
8
./third_party/WebKit/Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj
8 ./sandbox/sandbox.vcproj
8 ./remoting/remoting_webapp.vcproj
8 ./chrome/theme_resources.vcproj
8 ./chrome/packed_extra_resources.vcproj
8 ./chrome/common/extensions/api/api.vcproj

Divide by 4 to get the number of Gyp actions in each of these targets.  (That's
because if a Gyp target contains one action it will have one 'VCCustomBuildTool'
instance per "{Release,Debug}|{Win32,x64}".)


Secondly, I can test the impact of re-running Gyp actions unnecessarily just by
updating the timestamp of a .vcproj file.

I tried doing that with several of the .vcproj files above and here are the
resulting build times:

$ (for f in log-build-newgyp*; do grep -E 'touch|Build:|real' $f; wc -l $f;
echo; done)
+ touch ./sandbox/sandbox.vcproj
========== Build: 0 succeeded, 0 failed, 332 up-to-date, 0 skipped ==========
real	1m35.722s
10 log-build-newgyp2

+ touch ./native_client/copy_headers.vcproj
========== Build: 1 succeeded, 0 failed, 331 up-to-date, 0 skipped ==========
real	1m35.518s
14 log-build-newgyp3

+ touch ./third_party/protobuf/py_proto.vcproj
========== Build: 0 succeeded, 0 failed, 332 up-to-date, 0 skipped ==========
real	1m38.810s
10 log-build-newgyp4

+ touch ./chrome/packed_resources.vcproj
========== Build: 1 succeeded, 0 failed, 331 up-to-date, 0 skipped ==========
real	1m55.721s
125 log-build-newgyp5

+ touch ./third_party/WebKit/Source/WebKit/chromium/inspector_resources.vcproj
========== Build: 0 succeeded, 0 failed, 332 up-to-date, 0 skipped ==========
real	1m33.881s
10 log-build-newgyp6

+ touch
./third_party/WebKit/Source/WebCore/WebCore.gyp/webcore_bindings_sources.vcproj
========== Build: 20 succeeded, 0 failed, 312 up-to-date, 0 skipped ==========
real	28m15.073s
1771 log-build-newgyp7

+ touch ./third_party/WebKit/Source/WebCore/WebCore.vcproj/WebCore.vcproj
========== Build: 0 succeeded, 0 failed, 332 up-to-date, 0 skipped ==========
real	1m39.560s
10 log-build-newgyp8


So, only one of these, "webcore_bindings_sources" (the target you mentioned),
resulted in significant rebuilding.  I'm not sure how acceptable you would find
this?

Looking at the various actions in
third_party/WebKit/Source/WebCore/WebCore.gyp/WebCore.gyp, it seems like it
might be worthwhile to change these various scripts (css/makevalues.pl etc.) to
use write_if_changed() functions for writing these files, or wrap the commands
in a redo-ifchange script (along the lines of http://cr.yp.to/redo/atomic.html).
  As it is, they don't write the files in a fail-safe way.

Mark

[Mark Seaborn, 2012-05-09 02:04:47]

Closing: this was replaced by http://codereview.chromium.org/10375015/