msvs: fix many actions on the same script resulting in too-long command lines

Two overflows when many actions are in one target/one script.

1. Commands were previously joined with &&, which resulted in the command line being too long
2. cygwin's setup_env.bat was called for each command. This prepends to PATH, which also grows too long.

To fix #1, join the commands by checking errorlevel at each step. To fix #2, only do the path cygwin setup on the first action.

BUG=gyp:261
Committed: https://code.google.com/p/gyp/source/detail?r=1378

[scottmg, 2012-05-16 16:42:22]

Per previous discussion here: https://chromiumcodereview.appspot.com/10254030/

[jeanluc1, 2012-05-16 17:46:26]

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

https://chromiumcodereview.appspot.com/10341003/diff/12001/pylib/gyp/generato...
pylib/gyp/generator/msvs.py:245: quote_cmd, do_setup_env=True):
I would not provide a default but force caller to specify the value.

https://chromiumcodereview.appspot.com/10341003/diff/12001/pylib/gyp/generato...
pylib/gyp/generator/msvs.py:277: if do_setup_env else ''
I don't understand what this line does.  do_setup_env is a boolean.  So if it's
True, we have
   if True else ''.
Why the continuation?

https://chromiumcodereview.appspot.com/10341003/diff/12001/pylib/gyp/generato...
pylib/gyp/generator/msvs.py:1447: # Don't setup_env every time, or when all the
actions are run together in
-->  Don't setup_env every time.  When

https://chromiumcodereview.appspot.com/10341003/diff/12001/test/many-actions/...
File test/many-actions/many-actions.gyp (right):

https://chromiumcodereview.appspot.com/10341003/diff/12001/test/many-actions/...
test/many-actions/many-actions.gyp:14: 'a.c',
There's no a.c in this CL.  Does it already exist?

[scottmg, 2012-05-16 17:56:37]

thanks

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

https://chromiumcodereview.appspot.com/10341003/diff/12001/pylib/gyp/generato...
pylib/gyp/generator/msvs.py:245: quote_cmd, do_setup_env=True):
On 2012/05/16 17:46:26, jeanluc1 wrote:
> I would not provide a default but force caller to specify the value.

Done.

https://chromiumcodereview.appspot.com/10341003/diff/12001/pylib/gyp/generato...
pylib/gyp/generator/msvs.py:277: if do_setup_env else ''
On 2012/05/16 17:46:26, jeanluc1 wrote:
> I don't understand what this line does.  do_setup_env is a boolean.  So if
it's
> True, we have
>    if True else ''.
> Why the continuation?

It's like ?: in C (in a misguided attempt to terseify initialization).
Simplified.

https://chromiumcodereview.appspot.com/10341003/diff/12001/pylib/gyp/generato...
pylib/gyp/generator/msvs.py:1447: # Don't setup_env every time, or when all the
actions are run together in
On 2012/05/16 17:46:26, jeanluc1 wrote:
> -->  Don't setup_env every time.  When

Done.

https://chromiumcodereview.appspot.com/10341003/diff/12001/test/many-actions/...
File test/many-actions/many-actions.gyp (right):

https://chromiumcodereview.appspot.com/10341003/diff/12001/test/many-actions/...
test/many-actions/many-actions.gyp:14: 'a.c',
On 2012/05/16 17:46:26, jeanluc1 wrote:
> There's no a.c in this CL.  Does it already exist?

Oops, unnecessary. Removed.

[jeanluc1, 2012-05-16 18:03:01]

https://chromiumcodereview.appspot.com/10341003/diff/17001/test/many-actions/...
File test/many-actions/gyptest-many-actions.py (right):

https://chromiumcodereview.appspot.com/10341003/diff/17001/test/many-actions/...
test/many-actions/gyptest-many-actions.py:18: test.pass_test()
It's cool that you've added a test.  How do you verify that all 200 actions have
taken effect?

[scottmg, 2012-05-16 18:19:00]

thanks (also made do_setup_env non-default one more level up)

https://chromiumcodereview.appspot.com/10341003/diff/17001/test/many-actions/...
File test/many-actions/gyptest-many-actions.py (right):

https://chromiumcodereview.appspot.com/10341003/diff/17001/test/many-actions/...
test/many-actions/gyptest-many-actions.py:18: test.pass_test()
On 2012/05/16 18:03:01, jeanluc1 wrote:
> It's cool that you've added a test.  How do you verify that all 200 actions
have
> taken effect?

I think that's fairly well verified by other actions tests, but I guess it could
silently drop some actions, so might as well check here too. Done.

[jeanluc1, 2012-05-16 18:25:06]

lgtm

https://chromiumcodereview.appspot.com/10341003/diff/6004/test/many-actions/m...
File test/many-actions/many-actions.gyp (right):

https://chromiumcodereview.appspot.com/10341003/diff/6004/test/many-actions/m...
test/many-actions/many-actions.gyp:16: 'outputs':
['<(PRODUCT_DIR)/generated_0.h'],
I hope that using PRODUCT_DIR won't cause problems on the build bots.  Bradley
would know.

[scottmg, 2012-05-16 18:27:09]

On 2012/05/16 18:25:06, jeanluc1 wrote:
> lgtm
> 
>
https://chromiumcodereview.appspot.com/10341003/diff/6004/test/many-actions/m...
> File test/many-actions/many-actions.gyp (right):
> 
>
https://chromiumcodereview.appspot.com/10341003/diff/6004/test/many-actions/m...
> test/many-actions/many-actions.gyp:16: 'outputs':
> ['<(PRODUCT_DIR)/generated_0.h'],
> I hope that using PRODUCT_DIR won't cause problems on the build bots.  Bradley
> would know.

Do you mean because of too many files, or something else? When run via
gyptest.py it ends up going somewhere like gyp/out/testworkarea/testgyp.XXXXXX
along with the vcxproj, etc. So I think it should be OK.

[scottmg, 2012-05-16 18:30:17]

On 2012/05/16 18:27:09, scottmg wrote:
> On 2012/05/16 18:25:06, jeanluc1 wrote:
> > lgtm
> > 
> >
>
https://chromiumcodereview.appspot.com/10341003/diff/6004/test/many-actions/m...
> > File test/many-actions/many-actions.gyp (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10341003/diff/6004/test/many-actions/m...
> > test/many-actions/many-actions.gyp:16: 'outputs':
> > ['<(PRODUCT_DIR)/generated_0.h'],
> > I hope that using PRODUCT_DIR won't cause problems on the build bots. 
Bradley
> > would know.
> 
> Do you mean because of too many files, or something else? When run via
> gyptest.py it ends up going somewhere like gyp/out/testworkarea/testgyp.XXXXXX
> along with the vcxproj, etc. So I think it should be OK.

In any case, I guess I'll try it and keep an eye on the bots to see if they're
sad about it. :)

[jeanluc1, 2012-05-16 18:32:17]

On 2012/05/16 18:30:17, scottmg wrote:
> On 2012/05/16 18:27:09, scottmg wrote:
> > On 2012/05/16 18:25:06, jeanluc1 wrote:
> > > lgtm
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10341003/diff/6004/test/many-actions/m...
> > > File test/many-actions/many-actions.gyp (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10341003/diff/6004/test/many-actions/m...
> > > test/many-actions/many-actions.gyp:16: 'outputs':
> > > ['<(PRODUCT_DIR)/generated_0.h'],
> > > I hope that using PRODUCT_DIR won't cause problems on the build bots. 
> Bradley
> > > would know.
> > 
> > Do you mean because of too many files, or something else? When run via
> > gyptest.py it ends up going somewhere like
gyp/out/testworkarea/testgyp.XXXXXX
> > along with the vcxproj, etc. So I think it should be OK.
> 
> In any case, I guess I'll try it and keep an eye on the bots to see if they're
> sad about it. :)

I like that approach.  Good luck and thanks for fixing this!

[scottmg, 2012-05-16 21:15:09]

Doh, this doesn't work. The actions get put into a dict along the way and
reordered so the setup_env moves the middle randomly. Nothing ever takes 5
minutes! :/