Fix bug in ninja generator when there's no default action.

Fix bug in ninja generator when there's no default action.

In some cases, an action is only evaluated under certain conditions.
Previously this would break the ninja generator but works fine in the
make generator.

BUG=gyp:253
TEST=python gyptest.py -a test/rules/gyptest-all.py passes

r1410

[Yaron, 2012-05-15 00:51:02]

This seems to work although I don't know if it's too naive and I'm missing
something. From the test, it is processing the rule as expected and the test
failed prior to the change.

[Yaron, 2012-05-15 00:52:38]

Err I lied. I thought I had it failing before the change but I changed the test
fixture a bit too. Still looking

[Ami GONE FROM CHROMIUM, 2012-05-15 16:02:08]

Removing myself from reviewers.

(I see nothing obviously wrong here, but I also don't see the equivalent escape
hatch in make.py, which makes me think I don't really understand what's going on
here)

[Yaron, 2012-05-15 16:49:26]

Ok, through constructing the test case, I found the root cause. Ninja evaluates
rule['action'] before iterating through rule_sources (which is what make does).
If I specify an extension that matches the sources but don't have an action both
generators fail.

I'm not sure why third_party/libjpeg_turbo/libjpeg.gyp specifies an extension of
"asm" and then doesn't have any inputs. It seems like a broken rule but on the
other hand, the make generator is more robust in this case because it doesn't
fail given that no inputs match and therefore nothing should happen. I'm
inclined to match that behaviour in ninja but what are your thoughts?

[Nico, 2012-05-15 16:56:30]

On 2012/05/15 16:49:26, Yaron wrote:
> Ok, through constructing the test case, I found the root cause. Ninja
evaluates
> rule['action'] before iterating through rule_sources (which is what make
does).
> If I specify an extension that matches the sources but don't have an action
both
> generators fail.
> 
> I'm not sure why third_party/libjpeg_turbo/libjpeg.gyp specifies an extension
of
> "asm" and then doesn't have any inputs. It seems like a broken rule but on the
> other hand, the make generator is more robust in this case because it doesn't
> fail given that no inputs match and therefore nothing should happen. I'm
> inclined to match that behaviour in ninja but what are your thoughts?

Generators should behave the same. How exactly they behave for corner cases
isn't that important, as long as it matches. The test you're adding will make
sure that all generators will have consistent behavior.

(Is the CL ready for review? I patched it in, reverted the change to ninja.py,
and ran `python gyptest.py test/rules`, and no tests failed.)

[Yaron, 2012-05-15 17:12:57]

Ready for review now. Test reflects the edge case

[Nico, 2012-05-15 17:21:57]

http://codereview.chromium.org/10382161/diff/5005/test/rules/src/subdir2/no_a...
File test/rules/src/subdir2/no_action.gyp (right):

http://codereview.chromium.org/10382161/diff/5005/test/rules/src/subdir2/no_a...
test/rules/src/subdir2/no_action.gyp:5: # Test that the case where an action is
only specified under a conditional is evaulated appropriately.
80-col wrap comments

typo evaulated

http://codereview.chromium.org/10382161/diff/5005/test/rules/src/subdir2/no_a...
test/rules/src/subdir2/no_action.gyp:33: },
Reading the make generator code, it looks like it would throw for rules that
have no action but non-empty rule_sources. Ninja will ignore those silently with
your patch. Do you think there should be a test for that case? What should the
expected behavior be? Maybe ninja should only skip rules without action if not
rule.get('rule_sources', [])?

[Yaron, 2012-05-15 17:36:13]

http://codereview.chromium.org/10382161/diff/5005/test/rules/src/subdir2/no_a...
File test/rules/src/subdir2/no_action.gyp (right):

http://codereview.chromium.org/10382161/diff/5005/test/rules/src/subdir2/no_a...
test/rules/src/subdir2/no_action.gyp:5: # Test that the case where an action is
only specified under a conditional is evaulated appropriately.
On 2012/05/15 17:21:57, Nico wrote:
> 80-col wrap comments
> 
> typo evaulated

Done.

http://codereview.chromium.org/10382161/diff/5005/test/rules/src/subdir2/no_a...
test/rules/src/subdir2/no_action.gyp:33: },
On 2012/05/15 17:21:57, Nico wrote:
> Reading the make generator code, it looks like it would throw for rules that
> have no action but non-empty rule_sources. Ninja will ignore those silently
with
> your patch. Do you think there should be a test for that case? What should the
> expected behavior be? Maybe ninja should only skip rules without action if not
> rule.get('rule_sources', [])?

Done. Is there an "expect fails to gyp" option? I don't see one and catching
KeyError seems too broad.

[Nico, 2012-05-15 17:43:21]

http://codereview.chromium.org/10382161/diff/5005/test/rules/src/subdir2/no_a...
File test/rules/src/subdir2/no_action.gyp (right):

http://codereview.chromium.org/10382161/diff/5005/test/rules/src/subdir2/no_a...
test/rules/src/subdir2/no_action.gyp:33: },
On 2012/05/15 17:36:13, Yaron wrote:
> On 2012/05/15 17:21:57, Nico wrote:
> > Reading the make generator code, it looks like it would throw for rules that
> > have no action but non-empty rule_sources. Ninja will ignore those silently
> with
> > your patch. Do you think there should be a test for that case? What should
the
> > expected behavior be? Maybe ninja should only skip rules without action if
not
> > rule.get('rule_sources', [])?
> 
> Done. Is there an "expect fails to gyp" option? I don't see one and catching
> KeyError seems too broad.

test.run_gyp('my-gyp-file.gyp', chdir='src', status=1, stderr=None)

See e.g. test/same-name/gyptest-fail.py

[Yaron, 2012-05-24 22:21:20]

Added a test. Sorry for slow turnaround.

[Nico, 2012-05-25 22:43:31]

LGTM

Thanks for the test, and sorry for the slow review. I sent try jobs, and if they
come back green I'll land.

[Yaron, 2012-05-31 23:00:43]

On 2012/05/25 22:43:31, Nico wrote:
> LGTM
> 
> Thanks for the test, and sorry for the slow review. I sent try jobs, and if
they
> come back green I'll land.

Ok, updated the other generators. Seems like make was the only one who
previously allowed this.
win-64:http://build.chromium.org/p/tryserver.nacl/builders/gyp-win64/builds/393
wind32: http://build.chromium.org/p/tryserver.nacl/builders/gyp-win32/builds/394
mac:http://build.chromium.org/p/tryserver.nacl/builders/gyp-mac/builds/363
linux: queuing forever but that's where I develop and  python gyptest.py -a
test/rules/gyptest-all.py  --format=make,ninja,scons
p passes

[Nico, 2012-05-31 23:11:29]

r1410