Add support for "else if" in gyp conditions

Add support for "else if" in gyp conditions

Right now, a gyp condition consists of exactly two or three parts:

  [cond_expr, true_dict]
  [cond_expr, true_dict, false_dict]

This means if we want to have an "else if" condition, it needs to be inside a
nested 'conditions' list inside the false_dict.  This leads to unnecessarily
deep levels of nesting in gyp files.

This commit makes it so that if the 'false_dict' is not a dict, then we will
treat it as another cond_expr.  We keep looping like this until we find a
false_dict, or until we reach the end of the list.

This means we can now have conditions that look like this:

  [cond_expr1, true_dict1, cond_expr2, true_dict2, false_dict]

or:

  [cond_expr1, true_dict1, cond_expr2, true_dict2, cond_expr3, true_dict3]

BUG=
R=scottmg@chromium.org

Committed: https://code.google.com/p/gyp/source/detail?r=2004

[Shezan Baig (Bloomberg), 2014-09-25 18:52:49]

sbaig1@bloomberg.net changed reviewers:
+ bradnelson@chromium.org

[Shezan Baig (Bloomberg), 2014-09-25 18:52:50]

Hi Brad,
I think Mark is busy right now.  Do you have time to look at this?
Thanks!  -shez-

[Shezan Baig (Bloomberg), 2014-11-14 16:58:27]

Hi, is anyone available to review this input.py change? Thanks!

[scottmg, 2014-11-14 17:09:43]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2014-11-14 17:09:44]

https://codereview.chromium.org/601353002/diff/1/pylib/gyp/input.py
File pylib/gyp/input.py (right):

https://codereview.chromium.org/601353002/diff/1/pylib/gyp/input.py#newcode1049
pylib/gyp/input.py:1049: while result == None and i < len(condition):
what does the error look like if you mix up the condition/dict pairs? (i.e. two
dicts in a row, or two conditions in a row)

https://codereview.chromium.org/601353002/diff/1/test/conditions/elseif/elsei...
File test/conditions/elseif/elseif.gyp (right):

https://codereview.chromium.org/601353002/diff/1/test/conditions/elseif/elsei...
test/conditions/elseif/elseif.gyp:19: }, 'test_var==1', {
I applaud the effort to make elseifs better, but without syntax I worry this is
making the conditions blocks even more subtle to read. (Not bad here, but when
there's longer contained blocks)

Maybe gyp-developer@ has some opinions either way.

https://codereview.chromium.org/601353002/diff/1/test/conditions/elseif/gypte...
File test/conditions/elseif/gyptest_elseif.py (right):

https://codereview.chromium.org/601353002/diff/1/test/conditions/elseif/gypte...
test/conditions/elseif/gyptest_elseif.py:8: Verify that "else-if" conditons
work.
conditons -> conditions

https://codereview.chromium.org/601353002/diff/1/test/conditions/elseif/gypte...
test/conditions/elseif/gyptest_elseif.py:13: test =
TestGyp.TestGyp(formats=['msvs', 'ninja'])
should be all generators

[Shezan Baig (Bloomberg), 2014-11-14 17:25:44]

Thanks for the review!  I'll work on getting those additional validations in.

https://codereview.chromium.org/601353002/diff/1/pylib/gyp/input.py
File pylib/gyp/input.py (right):

https://codereview.chromium.org/601353002/diff/1/pylib/gyp/input.py#newcode1049
pylib/gyp/input.py:1049: while result == None and i < len(condition):
On 2014/11/14 17:09:44, scottmg wrote:
> what does the error look like if you mix up the condition/dict pairs? (i.e.
two
> dicts in a row, or two conditions in a row)

If there are two dicts in a row, it would treat the second dict as the final
"else", and anything after that would be ignored.  I can add some validation to
ensure that all items in 'condition' were consumed at the end of this while
loop.

For two conditions in a row, I believe what would happen is no different from
what gyp does now; i.e. it would return the second condition as the 'true_dict'
and I assume this will fail somewhere in MergeDicts.  I can add some validation
here as well to ensure the types are what we expect.

https://codereview.chromium.org/601353002/diff/1/test/conditions/elseif/elsei...
File test/conditions/elseif/elseif.gyp (right):

https://codereview.chromium.org/601353002/diff/1/test/conditions/elseif/elsei...
test/conditions/elseif/elseif.gyp:19: }, 'test_var==1', {
On 2014/11/14 17:09:44, scottmg wrote:
> I applaud the effort to make elseifs better, but without syntax I worry this
is
> making the conditions blocks even more subtle to read. (Not bad here, but when
> there's longer contained blocks)
> 
> Maybe gyp-developer@ has some opinions either way.

This is true.  In our code, we've been using this patch locally and formatting
the conditions like:

    ['test_var==0', {
    },
    'test_var==1', {
    },
    'test_var==2', {
    }],

IMO this makes it a bit easier to parse mentally, and it makes sense in our code
because our internal C/C++ coding style puts 'else' statements on a new line. 
But for this test, I was trying to follow how 'else' looks using the chromium
C++ coding style.

[Dirk Pranke, 2014-11-14 23:07:18]

On 2014/11/14 17:25:44, Shezan Baig (Bloomberg) wrote:
> Thanks for the review!  I'll work on getting those additional validations in.
> 
> https://codereview.chromium.org/601353002/diff/1/pylib/gyp/input.py
> File pylib/gyp/input.py (right):
> 
>
https://codereview.chromium.org/601353002/diff/1/pylib/gyp/input.py#newcode1049
> pylib/gyp/input.py:1049: while result == None and i < len(condition):
> On 2014/11/14 17:09:44, scottmg wrote:
> > what does the error look like if you mix up the condition/dict pairs? (i.e.
> two
> > dicts in a row, or two conditions in a row)
> 
> If there are two dicts in a row, it would treat the second dict as the final
> "else", and anything after that would be ignored.  I can add some validation
to
> ensure that all items in 'condition' were consumed at the end of this while
> loop.
> 
> For two conditions in a row, I believe what would happen is no different from
> what gyp does now; i.e. it would return the second condition as the
'true_dict'
> and I assume this will fail somewhere in MergeDicts.  I can add some
validation
> here as well to ensure the types are what we expect.
> 
>
https://codereview.chromium.org/601353002/diff/1/test/conditions/elseif/elsei...
> File test/conditions/elseif/elseif.gyp (right):
> 
>
https://codereview.chromium.org/601353002/diff/1/test/conditions/elseif/elsei...
> test/conditions/elseif/elseif.gyp:19: }, 'test_var==1', {
> On 2014/11/14 17:09:44, scottmg wrote:
> > I applaud the effort to make elseifs better, but without syntax I worry this
> is
> > making the conditions blocks even more subtle to read. (Not bad here, but
when
> > there's longer contained blocks)
> > 
> > Maybe gyp-developer@ has some opinions either way.

I think the benfits of eliminating the nested blocks might outway the dangers
here.

> 
> This is true.  In our code, we've been using this patch locally and formatting
> the conditions like:
> 
>     ['test_var==0', {
>     },
>     'test_var==1', {
>     },
>     'test_var==2', {
>     }],
> 

Are you aligning the second and third conditions with the first string, or 
with the opening bracket? I would think you'd want to align all of the
strings/conditions.

[Shezan Baig (Bloomberg), 2014-11-14 23:17:01]

On 2014/11/14 23:07:18, Dirk Pranke wrote:
> > This is true.  In our code, we've been using this patch locally and
formatting
> > the conditions like:
> > 
> >     ['test_var==0', {
> >     },
> >     'test_var==1', {
> >     },
> >     'test_var==2', {
> >     }],
> > 
> 
> Are you aligning the second and third conditions with the first string, or 
> with the opening bracket? I would think you'd want to align all of the
> strings/conditions.


We align them with the brackets.  It does seem odd that the strings don't align,
but aligning the strings makes it look like:

     ['test_var==0', {
     },
      'test_var==1', {
     },
      'test_var==2', {
     }],

which looks a bit weird to me.  The root cause is that the closing '}' is
already aligned with the opening '[', and this is how it has always been in our
(and chromium's) gyp files.  If we changed it to:

     ['test_var==0', {
      },
      'test_var==1', {
      },
      'test_var==2', {
     }],

it might look better, but this is a much bigger divergence from the current
established style.

[Shezan Baig (Bloomberg), 2014-11-18 16:16:56]

I've uploaded a new patch with some additional validation

[scottmg, 2014-11-18 16:29:36]

On 2014/11/18 16:16:56, Shezan Baig (Bloomberg) wrote:
> I've uploaded a new patch with some additional validation

Could you add a test for these error cases too?

I think with Dirk's somewhat support (and no dissents), the extra validations,
and the fact that it's strictly backwards compatible (i.e. can be not used at
all), it seems OK to add this feature.

Please also update
https://code.google.com/p/gyp/wiki/InputFormatReference#conditions.

[Shezan Baig (Bloomberg), 2014-11-18 17:30:36]

On 2014/11/18 16:29:36, scottmg wrote:
> Could you add a test for these error cases too?


I've improved the tests a bit.  Previously, it ran gyp separately for each
test_var value, now it just runs gyp once.

I also added tests for the "bad" cases.  This uncovered a bug in the
implementation, where it was terminating the while loop as soon as it found a
condition that evaluated to true, which means it did not validate anything after
that.  I modified the patch so that it continues looping until it reaches the
end (but only evaluates conditions if it hasn't found a result yet).


> I think with Dirk's somewhat support (and no dissents), the extra validations,
> and the fact that it's strictly backwards compatible (i.e. can be not used at
> all), it seems OK to add this feature.
> 
> Please also update
> https://code.google.com/p/gyp/wiki/InputFormatReference#conditions.

Thanks -- will do this as soon as the commit lands.

[scottmg, 2014-11-18 17:43:46]

lgtm

[Shezan Baig (Bloomberg), 2014-11-18 18:53:46]

Hmm, I'm getting the following error when I run 'git cl dcommit'.  Not sure what
it means.  My local branch is rebased on top of origin/master, and I don't have
any local changes.  Here's the message I'm getting:

Attempting to commit more than one change while --no-rebase is enabled.
If these changes depend on each other, re-running without --no-rebase may be
required. at /usr/lib/git-core/git-svn line 872.

ERROR from SVN:
Item is out of date: File or directory 'pylib/gyp/MSVSVersion.py' is out of
date; try updating: resource out of date; try updating
ERROR: Not all changes have been committed into SVN.
Please see the above messages for details.

Failed to push. If this persists, please file a bug.

[Mark Mentovai, 2014-11-18 18:55:26]

mark@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2014-11-18 18:55:26]

“git pull -r” first to rebase atop all upstream changes.

[Mark Mentovai, 2014-11-18 18:55:37]

mark@chromium.org changed reviewers:
- mark@chromium.org

[Shezan Baig (Bloomberg), 2014-11-18 19:03:24]

On 2014/11/18 18:55:26, Mark Mentovai wrote:
> “git pull -r” first to rebase atop all upstream changes.

I'm already on top of origin/master though.  i.e:

$ git fetch origin
$ git log HEAD..origin/master | wc -l
0

[Shezan Baig (Bloomberg), 2014-11-18 20:08:35]

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