Fix msvs-ninja OutputDirectory path.

Fix msvs-ninja OutputDirectory path.

_FixPath is designed to take gyp paths and convert them to msvs project paths.
This translates a <gyp_dir>"<gyp_dir_to_X>" to
<msvs_project_dir>"<msvs_project_dir_to_gyp_dir>/<gyp_dir_to_X>".

The OutputDirectory when using ninja as the external builder with the ninja
generator generated build files needs to be the path
<msvs_project_dir>"<msvs_project_dir_to_ninja_build_config>".
Since this is specified on a per target basis and will be run though _FixPath,
the external builder directory in the target must be specified as
<gyp_dir>"<gyp_dir_to_toplevel_dir>/<toplevel_dir_to_ninja_build>/<config>".

Chromium currently does not see any issue as it does not set generator_output.
When generator_output is not set, _GetPathOfProject sets fix_prefix to None.
Also, Chromium appears to be using an absolute path for
msvs_external_builder_out_dir.

This is, however, affecting Skia, which sets generator_output to 'out' and
places all of its gyp files in a 'gyp' directory. As a result Skia is seeing
the OutputDirectory set to "../../gyp/out/$(Configuration)". This change fixes
this to "../../out/$(Configuration)".

R=scottmg@chromium.org

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

[bungeman-chromium, 2014-04-22 15:33:07]

It appears that there is currently no bot coverage for external builders? The
win32 failure from my try appears to be an existing issue (I just saw an email
go by about it).

[Shezan Baig (Bloomberg), 2014-04-22 15:47:52]

On 2014/04/22 15:33:07, bungeman2 wrote:
> It appears that there is currently no bot coverage for external builders? The
> win32 failure from my try appears to be an existing issue (I just saw an email
> go by about it).

There is a test in test/msvs/external_builder

Can you modify/expand this test to cover your usecase?  Thanks!

[bungeman-chromium, 2014-04-26 14:33:52]

As a test, I have added 'msvs-ninja' to TestGyp and have it running the relevant
tests. It actually will pass most of the tests (it appears to fail ~35) but I
have only enabled the ones relevant to this change for now.

This is using MSBuild and not devenv, primarily because devenv's 'multiprocess'
configuration is user wide, but MSBuild allows a command line switch. The reason
for turning off multi-process is that we currently do not have an 'all' project
from the msvs generator, so we must instead build all the projects, and we do
not want multiple ninja processes running in the build directory at once. This
works well for hermetic builds, but non hermetic targets may run more than once.
Also, ninja does not place do-nothing targets in the top level build.ninja, so
building such projects will fail. This accounts for most of the failures, but
there may be other issues exposed as well.

Note that I am updating the ios xcode-ninja test here to match the new
distinction between 'format' and 'formats'. I would rather see an actual
xcode-ninja format in TestGyp structured like this msvs-ninja one, because then
existing tests may be used for coverage instead of creating one off
re-implementations of what should be in TestGyp. Directly changing the test's
format field is clever, but fragile.

As a note, it appears that the primary reason the Windows tests take so long is
due to GenerateEnvironmentFiles calling vsvars.bat twice, and it being called
many times. On my machine this was so bad that, while developing this, I locally
commented out the vsvars.bat calls and just ran vsvars.bat in the cmd.exe from
which I was running the tests.

[Shezan Baig (Bloomberg), 2014-04-29 21:19:15]

Sorry for the delay.  I tried the patch locally, but ran into the following
problem:

$ cat play/play.gyp
{
  'targets': [{
    'target_name': 'myplay',
    'type': 'executable',
    'sources': [ 'play.cpp' ],
  }],
}

$ cat play/play.cpp
#include <iostream>

int main()
{
    std::cout << "Hello world" << std::endl;
}

$ build/gyp/gyp -fmsvs-ninja --depth=.  play/play.gyp
Generator module: '<module 'gyp.generator' from
'build/gyp\pylib\gyp\generator\__init__.pyc'>'
Traceback (most recent call last):
  File "build/gyp/gyp_main.py", line 18, in <module>
    sys.exit(gyp.script_main())
  File "build/gyp\pylib\gyp\__init__.py", line 534, in script_main
    return main(sys.argv[1:])
  File "build/gyp\pylib\gyp\__init__.py", line 527, in main
    return gyp_main(args)
  File "build/gyp\pylib\gyp\__init__.py", line 512, in gyp_main
    generator.GenerateOutput(flat_list, targets, data, params)
  File "build/gyp\pylib\gyp\generator\msvs.py", line 1905, in GenerateOutput
    _InitNinjaFlavor(params, target_list, target_dicts)
  File "build/gyp\pylib\gyp\generator\msvs.py", line 1810, in _InitNinjaFlavor
    gyp.generator.ninja.ComputeOutputDir(params) + '\\$(Configuration)'
AttributeError: 'module' object has no attribute 'ninja'

[Shezan Baig (Bloomberg), 2014-04-29 21:20:39]

It works when I run with -fmsvs-ninja,ninja

But it fails if I run with just -fmsvs-ninja

[Shezan Baig (Bloomberg), 2014-05-23 17:17:30]

@bungeman2, did you get a chance to look at #4?  I think you just need an
"import ninja" at the top of _InitNinjaFlavor.

I'm not familiar with the test/buildbot changes, so someone else should look at
that, but the msvs.py changes look good to me (after adding the "import ninja").

[bungeman-chromium, 2014-05-23 17:43:02]

On 2014/05/23 17:17:30, Shezan Baig (Bloomberg) wrote:
> @bungeman2, did you get a chance to look at #4?  I think you just need an
> "import ninja" at the top of _InitNinjaFlavor.
> 
> I'm not familiar with the test/buildbot changes, so someone else should look
at
> that, but the msvs.py changes look good to me (after adding the "import
ninja").

I've been really busy with DirectWrite and related font stuff lately. I'll get
back to this as soon as I can. Thanks for taking a look.

[bungeman-chromium, 2014-05-28 20:06:00]

Now works when running with just msvs-ninja.

Scott, if you're available, you're might be the best one around to take a look
at the test/build changes.

[scottmg, 2014-05-30 16:41:37]

Sorry for the delay.

https://codereview.chromium.org/245923003/diff/140001/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/245923003/diff/140001/test/lib/TestGyp.py#new...
test/lib/TestGyp.py:136: realFormat = self.format.split('-')[-1]
realFormat -> real_format

https://codereview.chromium.org/245923003/diff/140001/test/lib/TestGyp.py#new...
test/lib/TestGyp.py:1013: def FindMSBuildInstallation():
This looks very similar to FindVisualStudioInstallation. Could it be shared
somehow?

[bungeman-chromium, 2014-05-30 22:18:08]

https://codereview.chromium.org/245923003/diff/140001/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/245923003/diff/140001/test/lib/TestGyp.py#new...
test/lib/TestGyp.py:136: realFormat = self.format.split('-')[-1]
On 2014/05/30 16:41:37, scottmg wrote:
> realFormat -> real_format

Done.

https://codereview.chromium.org/245923003/diff/140001/test/lib/TestGyp.py#new...
test/lib/TestGyp.py:1013: def FindMSBuildInstallation():
On 2014/05/30 16:41:37, scottmg wrote:
> This looks very similar to FindVisualStudioInstallation. Could it be shared
> somehow?

It is, but it needn't be. I think I shied away from it due to
FindVisualStudioInstallation having dead code and generally wanting to use just
MSBuild in the future and drop devenv (though we still need dumpbin, so we're
still looking for the vs path). Also, this is the wrong way to go about finding
MSBuild anyway (as described in the comments).

https://codereview.chromium.org/245923003/diff/160001/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/245923003/diff/160001/test/lib/TestGyp.py#new...
test/lib/TestGyp.py:796: if build_tool:
This code never runs, so removing it.

https://codereview.chromium.org/245923003/diff/160001/test/lib/TestGyp.py#new...
test/lib/TestGyp.py:1013: def FindMSBuildInstallation():
This is the better way to find MSBuild, not integrated with
FindVisualStudioInstallation.

[Shezan Baig (Bloomberg), 2014-05-31 05:09:55]

https://codereview.chromium.org/245923003/diff/170001/pylib/gyp/generator/msv...
File pylib/gyp/generator/msvs.py (right):

https://codereview.chromium.org/245923003/diff/170001/pylib/gyp/generator/msv...
pylib/gyp/generator/msvs.py:1814: ninja_generator.ComputeOutputDir(params) +
'\\$(Configuration)'
I ran into another issue with this when I was trying it out.  I think we need to
join this with options.toplevel_dir.  Something like:

os.path.join(
    options.toplevel_dir,
    ninja_generator.ComputeOutputDir(params)) + '\\$(Configuration)'

This simulates the code in ninja_generator.GenerateOutputForConfig where it
computes 'toplevel_build'.

[bungeman-chromium, 2014-06-02 14:55:47]

I believe that this now computes the paths correctly at PS11. ptal

https://codereview.chromium.org/245923003/diff/170001/pylib/gyp/generator/msv...
File pylib/gyp/generator/msvs.py (right):

https://codereview.chromium.org/245923003/diff/170001/pylib/gyp/generator/msv...
pylib/gyp/generator/msvs.py:1814: ninja_generator.ComputeOutputDir(params) +
'\\$(Configuration)'
On 2014/05/31 05:09:55, Shezan Baig (Bloomberg) wrote:
> I ran into another issue with this when I was trying it out.  I think we need
to
> join this with options.toplevel_dir.  Something like:
> 
> os.path.join(
>     options.toplevel_dir,
>     ninja_generator.ComputeOutputDir(params)) + '\\$(Configuration)'
> 
> This simulates the code in ninja_generator.GenerateOutputForConfig where it
> computes 'toplevel_build'.

I see now. msvs_external_builder_out_dir is always defined in the target spec
and is therefore relative to the gyp file which contains it. When running
msvs-ninja, msvs_exernal_builder_out_dir (if not already set) needs to be set to
the path from the gyp file in which the target is contained to the desired
output directory (in this case, where ninja put its output directory plus
$(Configuration)). ComputeOutputParams returns a path from the toplevel_dir to
the ninja build output directory. So this needs to be more like

spec['msvs_external_builder_out_dir'] = os.path.join(gyp_to_toplevel,
ninja_generator.ComputeOutputDir(params)) + '\\$(Configuration)'

In order to be consistent with how msvs_external_builder_out_dir would have been
defined in the target directly.

https://codereview.chromium.org/245923003/diff/170001/pylib/gyp/generator/msv...
pylib/gyp/generator/msvs.py:2703: gyp.common.RelativePath(external_out_dir,
project_dir) + '\\'
The external_out_dir is relative to the gyp file in which the target was
defined. The OutputDirectory needs to be the same path, but relative to the
msbuild project file. As a result, using _FixPath here was actually correct. The
issue was just that msvs-ninja wasn't setting the path so that this would work.

[scottmg, 2014-06-02 16:04:49]

lgtm w/ below

https://codereview.chromium.org/245923003/diff/190001/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/245923003/diff/190001/test/lib/TestGyp.py#new...
test/lib/TestGyp.py:764: import _winreg
I don't think this module is available on cygwin python so we try to avoid it in
gyp per _RegistryQueryBase in MSVSVersion.py.

https://codereview.chromium.org/245923003/diff/190001/test/lib/TestGyp.py#new...
test/lib/TestGyp.py:788: '2012': r'4.0', #really v4.0.30319 which comes with
.NET 4.5
nit; two spaces before #, one after, capital R, end with '.'.

[bungeman-chromium, 2014-06-02 17:35:30]

I have pulled in the registry query method of MSVSVersion.py with PS212. If the
test framework previously worked on cygwin, it might still work on cygwin.

https://codereview.chromium.org/245923003/diff/190001/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/245923003/diff/190001/test/lib/TestGyp.py#new...
test/lib/TestGyp.py:764: import _winreg
On 2014/06/02 16:04:49, scottmg wrote:
> I don't think this module is available on cygwin python so we try to avoid it
in
> gyp per _RegistryQueryBase in MSVSVersion.py.

I was not aware of that. Done.

https://codereview.chromium.org/245923003/diff/190001/test/lib/TestGyp.py#new...
test/lib/TestGyp.py:788: '2012': r'4.0', #really v4.0.30319 which comes with
.NET 4.5
On 2014/06/02 16:04:49, scottmg wrote:
> nit; two spaces before #, one after, capital R, end with '.'.

Done.

[bungeman-chromium, 2014-06-04 16:07:34]

Committed patchset #12 manually as r1926.

[bungeman-chromium, 2014-06-04 16:17:41]

On 2014/06/04 16:07:34, bungeman2 wrote:
> Committed patchset #12 manually as r1926.

Since I'm fairly sure this is correct I have landed this. If there are any
issues, please let me know.

There are a few things which I've learned working on this which translate into
future improvements.

1. There is now an msvs-ninja test harness, there should also be an xcode-ninja
test harness.

2. There is now an msvs-ninja test harness, but only a few select tests (those
relevant to this change) are enabled. There are currently 31 failing tests if
the test suite is run against msvs-ninja. These should be examined and decisions
made on what should be supported and fixed, and what can be ignored.

3. Calling vsvars.bat with vs2013 is really, really slow. It takes ~10s to run
on my machine. This is run at least twice per test, so running the tests on
Windows has huge overhead. In order to run the tests in a reasonable amount of
time, I disabled the actually running of vsvars.bat in msvs_emulation.py and ran
vsvars.bat from the cmd.exe from which I ran the tests.

I should probably open issues for these, but I wanted to note these points here
as well.

[Shezan Baig (Bloomberg), 2014-06-04 16:31:19]

On 2014/06/04 16:17:41, bungeman2 wrote:
> On 2014/06/04 16:07:34, bungeman2 wrote:
> > Committed patchset #12 manually as r1926.
> 
> Since I'm fairly sure this is correct I have landed this. If there are any
> issues, please let me know.
> 
> There are a few things which I've learned working on this which translate into
> future improvements.
> 
> 1. There is now an msvs-ninja test harness, there should also be an
xcode-ninja
> test harness.
> 
> 2. There is now an msvs-ninja test harness, but only a few select tests (those
> relevant to this change) are enabled. There are currently 31 failing tests if
> the test suite is run against msvs-ninja. These should be examined and
decisions
> made on what should be supported and fixed, and what can be ignored.
> 
> 3. Calling vsvars.bat with vs2013 is really, really slow. It takes ~10s to run
> on my machine. This is run at least twice per test, so running the tests on
> Windows has huge overhead. In order to run the tests in a reasonable amount of
> time, I disabled the actually running of vsvars.bat in msvs_emulation.py and
ran
> vsvars.bat from the cmd.exe from which I ran the tests.
> 
> I should probably open issues for these, but I wanted to note these points
here
> as well.


Awesome -- thanks for this change :)

I will be rebasing within the next few days, so hopefully will pick up your
latest changes.  Will let you know if I run into any issues.