ninja: rules chained only by output of first one failing

ninja: rules chained only by output of first one failing

Test from case in linked bug. Rule X generates .a -> .b, rule Y generates .b -> .c. Previously, Y wouldn't get run because ninja generator wasn't seeing the .b files that were only generated from running X.

BUG=127444

[scottmg, 2012-05-16 23:01:26]

(not sure who's a better reviewer... first one "wins"!)

[Nico, 2012-05-16 23:08:55]

https://chromiumcodereview.appspot.com/10381103/diff/11001/test/ninja/chained...
File test/ninja/chained-rules/gyptest-chained-rules.py (right):

https://chromiumcodereview.appspot.com/10381103/diff/11001/test/ninja/chained...
test/ninja/chained-rules/gyptest-chained-rules.py:15: test =
TestGyp.TestGyp(formats=['ninja'])
Why a ninja-only test?

[scottmg, 2012-05-16 23:10:29]

On 2012/05/16 23:08:55, Nico wrote:
>
https://chromiumcodereview.appspot.com/10381103/diff/11001/test/ninja/chained...
> File test/ninja/chained-rules/gyptest-chained-rules.py (right):
> 
>
https://chromiumcodereview.appspot.com/10381103/diff/11001/test/ninja/chained...
> test/ninja/chained-rules/gyptest-chained-rules.py:15: test =
> TestGyp.TestGyp(formats=['ninja'])
> Why a ninja-only test?

Sort of the same reason as Evan's comment :/ I'm suspecting at least one of the
other generators (make?) won't work. I guess I could easily make it a windows
test instead of a ninja test though.

[scottmg, 2012-05-18 05:24:36]

Per Evan's suggestion, gather sources as a preprocess. One complication is rules
that both process and output the same extension. Because of this, the rule
outputs are added to the sources as they are processed (this is what the msvs
generator does, I guess to handle this case).

Two new tests for action outputs being processed by a rule, and for rule outputs
being processed by a rule (and confirmed fixes linked bug).

[alexeypa (please no reviews), 2012-05-18 15:48:45]

On 2012/05/18 05:24:36, scottmg wrote:
> Per Evan's suggestion, gather sources as a preprocess. One complication is
rules
> that both process and output the same extension. Because of this, the rule
> outputs are added to the sources as they are processed (this is what the msvs
> generator does, I guess to handle this case).
> 
> Two new tests for action outputs being processed by a rule, and for rule
outputs
> being processed by a rule (and confirmed fixes linked bug).

Will it be able to handle
remoting/remoting.gyp:remoting_host_installation_unittest? It has three rules
doing this chain of transformations: foo.msi -> foo.wxs -> foo.wixobj ->
bar.msi. The original and final msi names do not match, but they both are msi
files.

[scottmg, 2012-05-18 15:57:34]

On 2012/05/18 15:48:45, alexeypa wrote:

> Will it be able to handle
> remoting/remoting.gyp:remoting_host_installation_unittest?

Yes, it works fine, but I can add another layer in the test to ensure that.

[Nico, 2012-05-18 16:07:41]

I'll leave this to Evan.

[Evan Martin, 2012-05-18 17:19:06]

LGTM

https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
File pylib/gyp/generator/ninja.py (left):

https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:521: extra_mac_bundle_resources):
It's so pleasing to see this param go, it always felt like a huge hack to me.

https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:311: for s in sources:
s/s/source/

https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:315: # Don't the outputs of this rule with this
rule, even if by
This comment accidentally a word.

https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:342: #sources.extend(action['inputs'])
Probably should delete dead code

[scottmg, 2012-05-18 18:06:30]

Thanks. I also added a 3-level-test per Alex's suggestion, and excluded make.

It looks like I need to exclude xcode too. I'm not familiar with xcode at all to
know whether it is fixable or not.

Despite this sort of being an improvement, I'm wondering now if we should have
instead just disallowed chained rules in gyp, and converted the handful of
remoting rules to a single action that runs a script.

https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:311: for s in sources:
On 2012/05/18 17:19:06, Evan Martin wrote:
> s/s/source/

Done.

https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:315: # Don't the outputs of this rule with this
rule, even if by
On 2012/05/18 17:19:06, Evan Martin wrote:
> This comment accidentally a word.

Done.

https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
pylib/gyp/generator/ninja.py:342: #sources.extend(action['inputs'])
On 2012/05/18 17:19:06, Evan Martin wrote:
> Probably should delete dead code

Done.

[scottmg, 2012-05-18 18:25:49]

On 2012/05/18 18:06:30, scottmg wrote:
> Thanks. I also added a 3-level-test per Alex's suggestion, and excluded make.
>
> It looks like I need to exclude xcode too. I'm not familiar with xcode at all
to
> know whether it is fixable or not.
> 
> Despite this sort of being an improvement, I'm wondering now if we should have
> instead just disallowed chained rules in gyp, and converted the handful of
> remoting rules to a single action that runs a script.

Hmm, the 3-level test is failing on VS2008. Do you know if
remoting_host_installation_unittest worked there somehow Alex?

e:\b\depot_tools\python_bin\python.exe
test\chained-rules\gyptest-chained-rules.py
Missing files:
`E:\b\build\slave\gyp\build\trunk\out\testworkarea\testgyp.tu15t1\Default\bar.msi'

This makes me lean further towards abandoning this and disallowing instead.

> 
>
https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
> File pylib/gyp/generator/ninja.py (right):
> 
>
https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
> pylib/gyp/generator/ninja.py:311: for s in sources:
> On 2012/05/18 17:19:06, Evan Martin wrote:
> > s/s/source/
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
> pylib/gyp/generator/ninja.py:315: # Don't the outputs of this rule with this
> rule, even if by
> On 2012/05/18 17:19:06, Evan Martin wrote:
> > This comment accidentally a word.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10381103/diff/15006/pylib/gyp/generato...
> pylib/gyp/generator/ninja.py:342: #sources.extend(action['inputs'])
> On 2012/05/18 17:19:06, Evan Martin wrote:
> > Probably should delete dead code
> 
> Done.

[scottmg, 2012-05-18 22:51:09]

On 2012/05/18 18:25:49, scottmg wrote:
> This makes me lean further towards abandoning this and disallowing instead.

Closing, and going the other way with:
https://chromiumcodereview.appspot.com/10399096/ and
https://chromiumcodereview.appspot.com/10392172/

extra_sources will live for another day. :/