Fixing msvs handling of actions which have overlapping inputs....

Fixing msvs handling of actions which have overlapping inputs.
Adding test related to issue 44.

BUG=gyp44
BUG=gyp32
TEST=actions-multiple



Committed: http://code.google.com/p/gyp/source/detail?r=880

[jeanluc1, 2011-02-01 02:00:07]

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py
File pylib/gyp/generator/msvs.py (right):

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:323: 
You could have done:

chosen_input = reduce(lambda x,y:len(actions_dict[x]) < len(actions_dict[y]) and
x or y, actions_dict)

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:365: dicts describing the actions attached to that
input file.
two more spaces?

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:377: description += action['description']
Another option is to pass a list for description and command, and inside of
_addCustom do a ', and also'.join().

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:553: _AddActionStep(inputs=[_FixPath(i) for i in
all_inputs],
I would expect the first argument to be actions_to_add or something similar.

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:1095: gyp_file = posixpath.split(build_file)[1]
Why posixpath?

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:1237: # Attach actions to the gyp file if nothing
else is there.
When would this happen?  What does it mean?

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:1240: inputs = [gyp_file]
Could have been:
inputs = a.get('inputs') or [gyp_file]

http://codereview.chromium.org/6250068/diff/3001/test/actions-multiple/gyptes...
File test/actions-multiple/gyptest-all.py (right):

http://codereview.chromium.org/6250068/diff/3001/test/actions-multiple/gyptes...
test/actions-multiple/gyptest-all.py:3: # Copyright (c) 2009 Google Inc. All
rights reserved.
2011

http://codereview.chromium.org/6250068/diff/3001/test/actions-multiple/src/ac...
File test/actions-multiple/src/actions.gyp (right):

http://codereview.chromium.org/6250068/diff/3001/test/actions-multiple/src/ac...
test/actions-multiple/src/actions.gyp:1: # Copyright (c) 2009 Google Inc. All
rights reserved.
2011 please.

[bradn, 2011-02-01 02:55:52]

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py
File pylib/gyp/generator/msvs.py (right):

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:377: description += action['description']
On 2011/02/01 02:00:08, jeanluc1 wrote:
> Another option is to pass a list for description and command, and inside of
> _addCustom do a ', and also'.join().

Done.

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:553: _AddActionStep(inputs=[_FixPath(i) for i in
all_inputs],
On 2011/02/01 02:00:08, jeanluc1 wrote:
> I would expect the first argument to be actions_to_add or something similar.

Done.

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:1095: gyp_file = posixpath.split(build_file)[1]
On 2011/02/01 02:00:08, jeanluc1 wrote:
> Why posixpath?

Previously the case where there are no inputs below didn't actually create an
action. So this was wrong, and never caught. At this point the build file is
still in /s.

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:1237: # Attach actions to the gyp file if nothing
else is there.
On 2011/02/01 02:00:08, jeanluc1 wrote:
> When would this happen?  What does it mean?
We have a few test cases where actions have no inputs (though technically they
should be on the script that's running).
It's not ideal that this is even supported, but it works currently, and one of
the actions tests fails without it.
This is the complement of the bit that adds the gyp file in around 1099.

Actually the more interesting question is why the null_input test in subdir3
passed before on windows.
I'm looking into that now.

http://codereview.chromium.org/6250068/diff/3001/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:1240: inputs = [gyp_file]
On 2011/02/01 02:00:08, jeanluc1 wrote:
> Could have been:
> inputs = a.get('inputs') or [gyp_file]
Duh...
Done.

[bradn, 2011-02-01 03:02:51]

http://codereview.chromium.org/6250068/diff/3001/test/actions-multiple/gyptes...
File test/actions-multiple/gyptest-all.py (right):

http://codereview.chromium.org/6250068/diff/3001/test/actions-multiple/gyptes...
test/actions-multiple/gyptest-all.py:3: # Copyright (c) 2009 Google Inc. All
rights reserved.
On 2011/02/01 02:00:08, jeanluc1 wrote:
> 2011

Done.

http://codereview.chromium.org/6250068/diff/3001/test/actions-multiple/src/ac...
File test/actions-multiple/src/actions.gyp (right):

http://codereview.chromium.org/6250068/diff/3001/test/actions-multiple/src/ac...
test/actions-multiple/src/actions.gyp:1: # Copyright (c) 2009 Google Inc. All
rights reserved.
On 2011/02/01 02:00:08, jeanluc1 wrote:
> 2011 please.

Done.

http://codereview.chromium.org/6250068/diff/1011/pylib/gyp/generator/msvs.py
File pylib/gyp/generator/msvs.py (left):

http://codereview.chromium.org/6250068/diff/1011/pylib/gyp/generator/msvs.py#...
pylib/gyp/generator/msvs.py:1049: a['inputs'] = inputs
Ah, its VERY subtle.
This line MODIFIES the spec (python references).
That's why this worked before.

[jeanluc1, 2011-02-01 07:23:47]

LGTM

http://codereview.chromium.org/6250068/diff/5012/test/actions-multiple/src/co...
File test/actions-multiple/src/copy.py (right):

http://codereview.chromium.org/6250068/diff/5012/test/actions-multiple/src/co...
test/actions-multiple/src/copy.py:1: #!/usr/bin/python
Even though it is small, we should probably put the header in here.

http://codereview.chromium.org/6250068/diff/5012/test/actions-multiple/src/fi...
File test/actions-multiple/src/filter.py (right):

http://codereview.chromium.org/6250068/diff/5012/test/actions-multiple/src/fi...
test/actions-multiple/src/filter.py:1: import sys
Even though it is small, we should probably put the header in here.

[bradn, 2011-02-01 17:41:25]

http://codereview.chromium.org/6250068/diff/5012/test/actions-multiple/src/co...
File test/actions-multiple/src/copy.py (right):

http://codereview.chromium.org/6250068/diff/5012/test/actions-multiple/src/co...
test/actions-multiple/src/copy.py:1: #!/usr/bin/python
On 2011/02/01 07:23:47, jeanluc1 wrote:
> Even though it is small, we should probably put the header in here.

Done.

http://codereview.chromium.org/6250068/diff/5012/test/actions-multiple/src/fi...
File test/actions-multiple/src/filter.py (right):

http://codereview.chromium.org/6250068/diff/5012/test/actions-multiple/src/fi...
test/actions-multiple/src/filter.py:1: import sys
On 2011/02/01 07:23:47, jeanluc1 wrote:
> Even though it is small, we should probably put the header in here.

Done.