Fixes an issue where OutputDirectory and IntermediateDirectory were being

Fixes an issue where OutputDirectory and IntermediateDirectory were being
incorrectly generated without a trailing slash on MSVS 2008.

This only manifested as an issue when running non-cygwin actions under MSVS2008,
where the symptoms would be that a tool using OutDir would write to directories
such as "Releaseobj\\some\\subdir" instead of "Release\\obj\\some\\subdir".

Unfortunately, the wrong path usually wouldn't cause the tool to _fail_, except
when a subsequent step tried to read the output of the previous tool, causing the
secondary tool to fail with confusing errors (reporting that the correct path
could not, in fact, be read).
Committed: https://code.google.com/p/gyp/source/detail?r=1466

[iannucci, 2012-08-06 23:52:49]

It's always the simplest things...

[scottmg, 2012-08-06 23:54:38]

I assume the current tests pass?

Can you add a new test for this case since it was quite a battle to track down?

[iannucci, 2012-08-07 00:00:54]

On 2012/08/06 23:54:38, scottmg wrote:
> I assume the current tests pass?
> 
> Can you add a new test for this case since it was quite a battle to track
down?

Done. Trybots should start munching on it shortly

[scottmg, 2012-08-07 00:01:58]

lgtm, sorry I missed that when that test got added.

[iannucci, 2012-08-08 16:57:17]

PT(another)L...

It turns out my initial implementation doesn't work (FixPath kills a trailing
slash), and there were some places where gyp+tests were assuming that there
should be an intermediate slash between $(OutDir)\whatever, whereas under
msbuild, it's OK to do $(OutDir)whatever. Now msbuild and msvs behave the same.

I also noticed a bug (which is not fixed in this cl), where the unit tests using
shlex actually remove (!!) all backslashes, which leads to the python script
generating somefilewhichshouldhavebeenanabsolutepath.txt in the test's working
directory, which _happens_ to work, but isn't what you'd expect. The offending
test is in intermediate_dir . I'll submit a separate cl to fix it.

[scottmg, 2012-08-08 17:05:39]

lgtm, but please watch/roll in the AM so we'll be here to revert if necessary.
:)

https://chromiumcodereview.appspot.com/10831183/diff/5012/test/intermediate_d...
File test/intermediate_dir/src/test.gyp (right):

https://chromiumcodereview.appspot.com/10831183/diff/5012/test/intermediate_d...
test/intermediate_dir/src/test.gyp:15:
'<(INTERMEDIATE_DIR)intermediate_out.txt',
These are not required removals, right? There'd just end up being an extra
slash? I'm a bit worried about unintended side effects to the Chromium build,
i.e. any of
http://code.google.com/searchframe#search/&exact_package=chromium&q=intermedi...

[iannucci, 2012-08-08 21:52:49]

On 2012/08/08 17:05:39, scottmg wrote:
> lgtm, but please watch/roll in the AM so we'll be here to revert if necessary.
> :)
> 
>
https://chromiumcodereview.appspot.com/10831183/diff/5012/test/intermediate_d...
> File test/intermediate_dir/src/test.gyp (right):
> 
>
https://chromiumcodereview.appspot.com/10831183/diff/5012/test/intermediate_d...
> test/intermediate_dir/src/test.gyp:15:
> '<(INTERMEDIATE_DIR)intermediate_out.txt',
> These are not required removals, right? There'd just end up being an extra
> slash? I'm a bit worried about unintended side effects to the Chromium build,
> i.e. any of
>
http://code.google.com/searchframe#search/&exact_package=chromium&q=intermedi...

OK! So I made things MORE BETTER.

It turns out that the msbuild path was already doing some automagical
slash-ectomies, because msbuild was more sensitive to the slashes than msvs, and
so was monkeypatched.

I've added an uber-monkeypatch, which takes the (somewhat abusive) functionality
in ConvertVCMacrosToMSBuild of 'fixing' the extra slash, and turned it into a
more general-purpose function, which I add to the action path when preparing a
non-cygwin action.

Also, it's ProbablyFaster™ since it now applies a single compiled regex instead
of running over the string 4 times. Or something...

[scottmg, 2012-08-08 22:02:56]

(i'd be surprised if an "in" + a regex was faster than 4x replace, but I don't
think it's material here)

https://chromiumcodereview.appspot.com/10831183/diff/5015/pylib/gyp/MSVSSetti...
File pylib/gyp/MSVSSettings.py (right):

https://chromiumcodereview.appspot.com/10831183/diff/5015/pylib/gyp/MSVSSetti...
pylib/gyp/MSVSSettings.py:402: s = FixVCMacroSlashes(s)
Is this a change in behavior? Or just a tidy up?

https://chromiumcodereview.appspot.com/10831183/diff/5015/pylib/gyp/generator...
File pylib/gyp/generator/msvs.py (right):

https://chromiumcodereview.appspot.com/10831183/diff/5015/pylib/gyp/generator...
pylib/gyp/generator/msvs.py:1258: prepared_attrs['OutputDirectory'] =
_FixPath(output_dir)+"\\"
spaces and ' not "

https://chromiumcodereview.appspot.com/10831183/diff/5015/pylib/gyp/generator...
pylib/gyp/generator/msvs.py:1261: prepared_attrs['IntermediateDirectory'] =
_FixPath(intermediate)+"\\"
same

[iannucci, 2012-08-08 22:09:44]

Yeah it's probably not faster in reality (since these strings are all very short
anyhow) :). I don't think that it's slower though.

Since this was just nits, I'm going to skip the trybots.

https://chromiumcodereview.appspot.com/10831183/diff/5015/pylib/gyp/MSVSSetti...
File pylib/gyp/MSVSSettings.py (right):

https://chromiumcodereview.appspot.com/10831183/diff/5015/pylib/gyp/MSVSSetti...
pylib/gyp/MSVSSettings.py:402: s = FixVCMacroSlashes(s)
On 2012/08/08 22:02:56, scottmg wrote:
> Is this a change in behavior? Or just a tidy up?

Just a tidy to make sure this function's behavior doesn't change.

https://chromiumcodereview.appspot.com/10831183/diff/5015/pylib/gyp/generator...
File pylib/gyp/generator/msvs.py (right):

https://chromiumcodereview.appspot.com/10831183/diff/5015/pylib/gyp/generator...
pylib/gyp/generator/msvs.py:1258: prepared_attrs['OutputDirectory'] =
_FixPath(output_dir)+"\\"
On 2012/08/08 22:02:56, scottmg wrote:
> spaces and ' not "

Done

https://chromiumcodereview.appspot.com/10831183/diff/5015/pylib/gyp/generator...
pylib/gyp/generator/msvs.py:1261: prepared_attrs['IntermediateDirectory'] =
_FixPath(intermediate)+"\\"
On 2012/08/08 22:02:56, scottmg wrote:
> same

and done

[scottmg, 2012-08-08 22:10:39]

lgtm