Break lines when joining commands that share the same primary input.

Break lines when joining commands that share the same primary input.

Previously, commands for actions with the same pimary input were joined with && for MSBuild.  This can lead to extraordinarily long command lines, which blow some limit in cmd.exe.  This change joins such commands with linebreaks and error handling.

BUG=gyp:261

[grt (UTC plus 2), 2012-04-28 13:43:01]

Hi Scott,

This issue has been breaking webcore_bindings_sources for me recently.  Please
let me know if someone else is better suited to review this.  Thanks.

[scottmg, 2012-04-29 19:46:05]

On 2012/04/28 13:43:01, grt wrote:
> Hi Scott,
> 
> This issue has been breaking webcore_bindings_sources for me recently.

Could you add a test? Does it break on 2008 or ninja builds too?

[grt (UTC plus 2), 2012-04-30 14:28:39]

On 2012/04/29 19:46:05, scottmg wrote:
> On 2012/04/28 13:43:01, grt wrote:
> > Hi Scott,
> > 
> > This issue has been breaking webcore_bindings_sources for me recently.
> 
> Could you add a test?

What would you like to see in a test?  There doesn't seem to be much test code
in there for me to build upon.

> Does it break on 2008 or ninja builds too?

The MSVS code (for 2008) already joins the commands with newlines, so the bug
doesn't manifest there.  I don't see a similar problem with the ninja generator
(does it combine actions like the MSVS generator does?), but I have to say I'm
not even remotely familiar with it.

[scottmg, 2012-04-30 17:54:29]

On 2012/04/30 14:28:39, grt wrote:
> On 2012/04/29 19:46:05, scottmg wrote:
> > On 2012/04/28 13:43:01, grt wrote:
> > > Hi Scott,
> > > 
> > > This issue has been breaking webcore_bindings_sources for me recently.
> > 
> > Could you add a test?
> 
> What would you like to see in a test?  There doesn't seem to be much test code
> in there for me to build upon.

There's few unit tests but there's lots of tests in test/. e.g. A similar test
is test/win/gyptest-long-command-line.py. That only tests long lines on compiler
and linker invocations currently, not actions (assuming that's the limit that's
being exceeded).

lgtm if you want to commit the fix now, and add a test afterwards though.

> 
> > Does it break on 2008 or ninja builds too?
> 
> The MSVS code (for 2008) already joins the commands with newlines, so the bug
> doesn't manifest there.  I don't see a similar problem with the ninja
generator
> (does it combine actions like the MSVS generator does?), but I have to say I'm
> not even remotely familiar with it.

I don't know either, that's why I want to add a test. :-)

[grt (UTC plus 2), 2012-04-30 21:46:19]

On 2012/04/30 17:54:29, scottmg wrote:
> On 2012/04/30 14:28:39, grt wrote:
> > On 2012/04/29 19:46:05, scottmg wrote:
> > > On 2012/04/28 13:43:01, grt wrote:
> > > > Hi Scott,
> > > > 
> > > > This issue has been breaking webcore_bindings_sources for me recently.
> > > 
> > > Could you add a test?
> > 
> > What would you like to see in a test?  There doesn't seem to be much test
code
> > in there for me to build upon.
> 
> There's few unit tests but there's lots of tests in test/. e.g. A similar test
> is test/win/gyptest-long-command-line.py. That only tests long lines on
compiler
> and linker invocations currently, not actions (assuming that's the limit
that's
> being exceeded).

Cool, thanks for the pointer.  Does the test infrastructure automagically run
for all versions of dev studio?  Who should I both with questions about how to
write the test?

> lgtm if you want to commit the fix now, and add a test afterwards though.

That'd be great.  I don't have commit access to the project and the commit bit
in rietveld doesn't seem to be doing anything.  Is there a process somewhere I
should follow (or, could you land this for me)?

[scottmg, 2012-04-30 21:52:14]

On 2012/04/30 21:46:19, grt wrote:
> On 2012/04/30 17:54:29, scottmg wrote:
> > On 2012/04/30 14:28:39, grt wrote:
> > > On 2012/04/29 19:46:05, scottmg wrote:
> > > > On 2012/04/28 13:43:01, grt wrote:
> > > > > Hi Scott,
> > > > > 
> > > > > This issue has been breaking webcore_bindings_sources for me recently.
> > > > 
> > > > Could you add a test?
> > > 
> > > What would you like to see in a test?  There doesn't seem to be much test
> code
> > > in there for me to build upon.
> > 
> > There's few unit tests but there's lots of tests in test/. e.g. A similar
test
> > is test/win/gyptest-long-command-line.py. That only tests long lines on
> compiler
> > and linker invocations currently, not actions (assuming that's the limit
> that's
> > being exceeded).
> 
> Cool, thanks for the pointer.  Does the test infrastructure automagically run
> for all versions of dev studio?  Who should I both with questions about how to
> write the test?

You can ask gyp-developer or me, or generally just the setup of another test
nearby.

But yes, the tests run on Windows on 2008, 2010 and ninja (and whatever's
available for other platforms). You can exclude certain generators or platforms
if that makes sense. Buildbot is here http://build.chromium.org/f/client/gyp/
where you can see the output for each generator.

> 
> > lgtm if you want to commit the fix now, and add a test afterwards though.
> 
> That'd be great.  I don't have commit access to the project and the commit bit
> in rietveld doesn't seem to be doing anything.  Is there a process somewhere I
> should follow (or, could you land this for me)?

+bradnelson can add committer access (probably useful if you expect to make any
other changes)

[scottmg, 2012-04-30 21:52:54]

On 2012/04/30 21:52:14, scottmg wrote:

> +bradnelson can add committer access (probably useful if you expect to make
any
> other changes)

(I can also land it if you need me to)

[grt (UTC plus 2), 2012-05-01 19:59:13]

On 2012/04/30 21:52:54, scottmg wrote:
> (I can also land it if you need me to)

Please go ahead and do so.  Thanks.

[scottmg, 2012-05-02 19:04:06]

On 2012/05/01 19:59:13, grt wrote:
> On 2012/04/30 21:52:54, scottmg wrote:
> > (I can also land it if you need me to)
> 
> Please go ahead and do so.  Thanks.

I wrote a test that generates a 100k command line with 1000 merged actions, but
I can't get it to fail on 2008 or 2010.
https://chromiumcodereview.appspot.com/10341003

Could you try that patch to see if it fails for you (without your change
obviously)? Maybe there's something different we should be fixing (individual
command length, etc.)

You run via: python gyptest.py -f msvs test\many-actions\gyptest-many-actions.py

[grt (UTC plus 2), 2012-05-16 12:56:02]

Hi Scott.  Although I couldn't repro the problem with your test, I see someone
asking on chromium-dev today about the same build failure (search for Prawyn). 
I'd like to go ahead and land this change since I don't think it causes any
harm, and will likely prevent others from losing time if they suddenly run into
it.  What do you think?

[scottmg, 2012-05-16 14:26:20]

On 2012/05/16 12:56:02, grt wrote:
> Hi Scott.  Although I couldn't repro the problem with your test, I see someone
> asking on chromium-dev today about the same build failure (search for Prawyn).

> I'd like to go ahead and land this change since I don't think it causes any
> harm, and will likely prevent others from losing time if they suddenly run
into
> it.  What do you think?

I guess. I don't really like randomly making changes when we don't know what the
problem is, but I guess that's the uncertainty of Windows. :) Do you have
committer bit yet? (Brad's back in the office so you can ping him, or Mark)

[scottmg, 2012-05-16 14:29:49]

On 2012/05/16 14:26:20, scottmg wrote:
> On 2012/05/16 12:56:02, grt wrote:
> > Hi Scott.  Although I couldn't repro the problem with your test, I see
someone
> > asking on chromium-dev today about the same build failure (search for
Prawyn).
> 
> > I'd like to go ahead and land this change since I don't think it causes any
> > harm, and will likely prevent others from losing time if they suddenly run
> into
> > it.  What do you think?
> 
> I guess. I don't really like randomly making changes when we don't know what
the
> problem is, but I guess that's the uncertainty of Windows. :) Do you have
> committer bit yet? (Brad's back in the office so you can ping him, or Mark)

One possibility I notice from that thread is that it's using cygwin to run.
Perhaps deleting msvs_cygwin_shell: '0' from the test will cause it to fail.

[scottmg, 2012-05-16 15:23:52]

On 2012/05/16 14:29:49, scottmg wrote:
> On 2012/05/16 14:26:20, scottmg wrote:
> > On 2012/05/16 12:56:02, grt wrote:
> > > Hi Scott.  Although I couldn't repro the problem with your test, I see
> someone
> > > asking on chromium-dev today about the same build failure (search for
> Prawyn).
> > 
> > > I'd like to go ahead and land this change since I don't think it causes
any
> > > harm, and will likely prevent others from losing time if they suddenly run
> > into
> > > it.  What do you think?
> > 
> > I guess. I don't really like randomly making changes when we don't know what
> the
> > problem is, but I guess that's the uncertainty of Windows. :) Do you have
> > committer bit yet? (Brad's back in the office so you can ping him, or Mark)
> 
> One possibility I notice from that thread is that it's using cygwin to run.
> Perhaps deleting msvs_cygwin_shell: '0' from the test will cause it to fail.

Aha! Yes, that's it. I'll commit your change now along with my test.

[scottmg, 2012-05-16 15:34:10]

On 2012/05/16 15:23:52, scottmg wrote:
> On 2012/05/16 14:29:49, scottmg wrote:
> > On 2012/05/16 14:26:20, scottmg wrote:
> > > On 2012/05/16 12:56:02, grt wrote:
> > > > Hi Scott.  Although I couldn't repro the problem with your test, I see
> > someone
> > > > asking on chromium-dev today about the same build failure (search for
> > Prawyn).
> > > 
> > > > I'd like to go ahead and land this change since I don't think it causes
> any
> > > > harm, and will likely prevent others from losing time if they suddenly
run
> > > into
> > > > it.  What do you think?
> > > 
> > > I guess. I don't really like randomly making changes when we don't know
what
> > the
> > > problem is, but I guess that's the uncertainty of Windows. :) Do you have
> > > committer bit yet? (Brad's back in the office so you can ping him, or
Mark)
> > 
> > One possibility I notice from that thread is that it's using cygwin to run.
> > Perhaps deleting msvs_cygwin_shell: '0' from the test will cause it to fail.
> 
> Aha! Yes, that's it. I'll commit your change now along with my test.

Hmm, unfortunately this patch doesn't actually fix my case here
https://chromiumcodereview.appspot.com/10341003/ I'll see if I can find
something else that looks too long, sigh. :(

[scottmg, 2012-05-16 16:08:36]

On 2012/05/16 15:34:10, scottmg wrote:
> On 2012/05/16 15:23:52, scottmg wrote:
> > On 2012/05/16 14:29:49, scottmg wrote:
> > > On 2012/05/16 14:26:20, scottmg wrote:
> > > > On 2012/05/16 12:56:02, grt wrote:
> > > > > Hi Scott.  Although I couldn't repro the problem with your test, I see
> > > someone
> > > > > asking on chromium-dev today about the same build failure (search for
> > > Prawyn).
> > > > 
> > > > > I'd like to go ahead and land this change since I don't think it
causes
> > any
> > > > > harm, and will likely prevent others from losing time if they suddenly
> run
> > > > into
> > > > > it.  What do you think?
> > > > 
> > > > I guess. I don't really like randomly making changes when we don't know
> what
> > > the
> > > > problem is, but I guess that's the uncertainty of Windows. :) Do you
have
> > > > committer bit yet? (Brad's back in the office so you can ping him, or
> Mark)
> > > 
> > > One possibility I notice from that thread is that it's using cygwin to
run.
> > > Perhaps deleting msvs_cygwin_shell: '0' from the test will cause it to
fail.
> > 
> > Aha! Yes, that's it. I'll commit your change now along with my test.
> 
> Hmm, unfortunately this patch doesn't actually fix my case here
> https://chromiumcodereview.appspot.com/10341003/ I'll see if I can find
> something else that looks too long, sigh. :(

For those playing along at home, it turns out it's the cygwin\setup_env.bat
script. It does (nonlocal)

set PATH=%~dp0\bin;%~dp0\..\python_26;%PATH%

so when that's called many times in a batch file that command line gets too long
and that's what errors out. It also probably explains why only some people were
seeing it: it's not the path length to chromium, it's the
PATH=c:\joi\would\really\know\better\than\to\add\utilities\in\a\directory\named\like\this\that\reaches\all\the\way\to\iceland\for\example.

Not sure the best way to fix it now. We might be able to modify the setup for
cygwin to be factored out, but that setup also sets up other environment
variables and ... stuff. I'll see how ugly it gets I guess.

Mostly YA reason to deep-six all traces of cygwin...

[scottmg, 2012-05-16 16:36:45]

On 2012/05/16 16:08:36, scottmg wrote:

> ... might be able to modify the setup for
> cygwin to be factored out

^this^ + Greg's patch rolled into
https://chromiumcodereview.appspot.com/10341003