Integrate ninja build with Visual Studio

Integrate ninja build with Visual Studio

This change allows plugging in external builders to the
msvs projects.  This is activated by defining the
following flags in target_defaults:

* msvs_external_builder
* msvs_external_builder_out_dir
* msvs_external_builder_build_cmd
* msvs_external_builder_clean_cmd

When msvs_external_builder is defined for a gyp target, the
following changes are made to the generated msvs project:

* The project will use the provided msvs_external_builder_build_cmd
  to build the project, and also use the provided
  msvs_external_builder_clean_cmd to clean the project.

* No dependencies between projects will be created. This is because we
  want to use ninja's dependency build instead.

* The build output directory will be msvs_external_builder_out_dir
  instead of 'src/build/$(Configuration)'.

* MSBuild rules and actions will not be generated. This is because we
  want the external builder to perform these actions
  instead.

This change has no effect if the 'msvs_external_builder' variable is
not used (or if it is an empty string).

Some caveats of these mode are:

* The 'Build/Clean Solution' commands in Visual Studio cannot be used,
  because this will invoke a separate external builder instance
  for each project in the solution.

* Hitting F5 in Visual Studio always prompts to build, claiming that
  the build target is out of date, even if it was just built.

Test: Define the following variables in target_defaults:

  'msvs_external_builder': 'ninja',
  'msvs_external_builder_out_dir': '<(DEPTH)/out/$(Configuration)',
  'msvs_external_builder_build_cmd': [
    'ninja.exe',
    '-C',
    '$(OutDir)',
    '$(ProjectName)',
  ],
  'msvs_external_builder_clean_cmd': [
    'ninja.exe',
    '-C',
    '$(OutDir)',
    '-t',
    'clean',
    '$(ProjectName)',
  ],

Alternatively, export "GYP_GENERATORS=msvs-ninja".  This
will automatically setup msvs to build with ninja.

Then run "gclient runhooks" to generate your projects. Open
the generated solution. Right-click on any project and click
the 'Build' menu item. The output window will show the ninja
build in progress.

Patch by sbaig1@bloomberg.net

R=dpranke@chromium.org, scottmg@chromium.org
BUG=

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

[scottmg, 2013-04-15 19:25:59]

"""
* Hitting F5 in Visual Studio always prompts to build, claiming that 
the build target is out of date, even if it was just built. 
"""

Is it not possible to specify the outputs for the project so this doesn't
happen?

This maintains all the include/defines/etc so that Intellisense works?

[scottmg, 2013-04-15 19:27:07]

(We'll need to think of a way to do at least basic testing around
msvs_external_builder too.)

[scottmg, 2013-04-15 19:37:58]

i'm not very familiar with the msvs generator but this looks fairly reasonable.

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.py
File pylib/gyp/generator/msvs.py (right):

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.p...
pylib/gyp/generator/msvs.py:2574: if external_builder == 'ninja':
it feels a bit odd to try to genericize to "external_builder", but then have
ninja-specific details in msvs.py

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.p...
pylib/gyp/generator/msvs.py:2632: 'loadable_module': '.dll',
this looks a lot like the table in MSVSUtil.py, could that be used instead?

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.p...
pylib/gyp/generator/msvs.py:3068: ['-j', '${NUMBER_OF_PROCESSORS_PLUS_1}'])
ninja determines -j automatically, don't specify it manually

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.p...
pylib/gyp/generator/msvs.py:3078: def _GetMSBuildNinjaTarget(spec,
msbuild_target_name, ninja_args):
docstring

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.p...
pylib/gyp/generator/msvs.py:3080: 'ninja.exe',
i don't love the hardcoding here either. perhaps a variable to at least allow a
ninja binary that's not in the path.

[Shezan Baig (Bloomberg), 2013-04-15 19:39:53]

On 2013/04/15 19:25:59, scottmg wrote:
> Is it not possible to specify the outputs for the project so this doesn't
> happen?

The output file is actually set to the .exe in src/out/$(Configuration). 
However, this doesn't prevent Visual Studio from prompting for a rebuild.

The way to prevent Visual Studio from prompting is to trick it into thinking
that is has done a build, by creating a file called
"$(IntDir)\$(ProjectName).lastbuildstatus".  However, the result of this is that
Visual Studio will not build when you make changes to one of the dependent
projects, because Visual Studio doesn't know about that dependency.

If there is a way to query ninja to see if it has any work to do for a target,
it might be possible to create the *.lastbuildstatus file if I can figure out
the right place to hook into MSBuild for this.  I'm gonna start investigating
that now.


> This maintains all the include/defines/etc so that Intellisense works?

Yup :)

[Dirk Pranke, 2013-04-16 00:41:03]

I don't have much msvs-wizardry to offer, so I defer to scottmg on that. I've
added a few other comments. Generally, looks good and it'll be cool if/when it
works.

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.py
File pylib/gyp/generator/msvs.py (right):

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.p...
pylib/gyp/generator/msvs.py:2574: if external_builder == 'ninja':
On 2013/04/15 19:37:58, scottmg wrote:
> it feels a bit odd to try to genericize to "external_builder", but then have
> ninja-specific details in msvs.py

Maybe we should at least validate the value of external_builder when we start
the generator, to bail out if it's not equal to 'ninja'? One can imagine wanting
to re-use this code for other builders like make, tup, etc.

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.p...
pylib/gyp/generator/msvs.py:2626: if external_builder != None:
Nit: this can probably be just 'if external_builder:'

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.p...
pylib/gyp/generator/msvs.py:2982: external_builder =
generator_flags.get('msvs_external_builder', None)
Nit: we should verify that external_builder == 'ninja' if it's set at all, and
return some sort of helpful error message otherwise.

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.p...
pylib/gyp/generator/msvs.py:2995: if external_builder != None:
same nit.

https://codereview.chromium.org/13867004/diff/2001/pylib/gyp/generator/msvs.p...
pylib/gyp/generator/msvs.py:3008: if external_builder != None:
same nit.

[Shezan Baig (Bloomberg), 2013-04-16 16:01:39]

Thanks for all the review comments.  I've redone the patch to be more generic,
and removed all the ninja-specific bits.  This will allow people to plug
whatever external builder they want.

These external builders will now be defined as gyp variables on the target spec,
rather than generator flags.

Regarding the popup that appears on F5, the only way I could find to disable it
was in Tools->Options->Projects and Solutions->Build and Run->On Run, when
projects are out of date

Users can change this option to "Always build" (always invoke ninja on F5) or
"Never build" (users would be expected to manually run Build).  I personally
just leave it as "Prompt to build".  After using the external builder for a
while, I have found the popup to not be too intrusive to my workflow.

I found the following links helpful in my research:

http://social.msdn.microsoft.com/Forums/en-US/msbuild/thread/58ee9ec5-03c4-43...
http://connect.microsoft.com/VisualStudio/feedback/details/677499/different-r...
http://social.msdn.microsoft.com/Forums/en-US/msbuild/thread/880f2540-50c2-44...
http://stackoverflow.com/questions/5240820/how-does-visual-studo-2010-hosts-m...

I'm continuing my research on this, but so far it seems like this is the
built-in behavior in Visual Studio (outside the scope of MSBuild), and there's
no way to hook into it from MSBuild.

[scottmg, 2013-04-16 17:23:49]

This looks good!

We need a test though. Now that the setup isn't ninja-specific it should be
possible to have a "stub" builder hooked through msvs_external_builder* for
testing?

The important things to be tested seem to be: 1) that the standard MSBuild rules
don't get generated; 2) the external build tool gets executed.

[Dirk Pranke, 2013-04-16 17:28:28]

I like the idea that you've pulled all of the ninja-specific stuff out of the
main generator.

I don't like the idea that now the user has to specify so many commands that'll
be the same all the time (even if we put the commands in a gyp file). 

Maybe we should provide a default dictionary for _out_dir, _build_cmd, and
_clean_cmd ?

[Shezan Baig (Bloomberg), 2013-04-16 17:54:57]

On 2013/04/16 17:23:49, scottmg wrote:
> This looks good!
> 
> We need a test though. Now that the setup isn't ninja-specific it should be
> possible to have a "stub" builder hooked through msvs_external_builder* for
> testing?
> 
> The important things to be tested seem to be: 1) that the standard MSBuild
rules
> don't get generated; 2) the external build tool gets executed.


Thanks, I'm familiarizing myself with the gyp test infrastructure, so hopefully
will have this ready soon.

[Shezan Baig (Bloomberg), 2013-04-16 17:57:49]

On 2013/04/16 17:28:28, Dirk Pranke wrote:
> I like the idea that you've pulled all of the ninja-specific stuff out of the
> main generator.
> 
> I don't like the idea that now the user has to specify so many commands
that'll
> be the same all the time (even if we put the commands in a gyp file). 
> 
> Maybe we should provide a default dictionary for _out_dir, _build_cmd, and
> _clean_cmd ?


I was planning to add these commands in src/build/common.gypi (in a separate
checkin), so it doesn't have to be defined in each gyp file.  The users just
need to specify "msvs_external_builder=ninja" in their GYP_DEFINES.  Does that
sound more reasonable?

[Dirk Pranke, 2013-04-16 18:01:02]

On 2013/04/16 17:57:49, Shezan Baig (Bloomberg) wrote:
> On 2013/04/16 17:28:28, Dirk Pranke wrote:
> > I like the idea that you've pulled all of the ninja-specific stuff out of
the
> > main generator.
> > 
> > I don't like the idea that now the user has to specify so many commands
> that'll
> > be the same all the time (even if we put the commands in a gyp file). 
> > 
> > Maybe we should provide a default dictionary for _out_dir, _build_cmd, and
> > _clean_cmd ?
> 
> 
> I was planning to add these commands in src/build/common.gypi (in a separate
> checkin), so it doesn't have to be defined in each gyp file.  The users just
> need to specify "msvs_external_builder=ninja" in their GYP_DEFINES.  Does that
> sound more reasonable?

I'm fine w/ the msvs_external_builder=ninja setting, but it would be nice to
have the defaults applied for non-chromium projects as well. It feels like it
should just work out of the box to me.

Or maybe we should add a 'msvs-ninja' generator format that sets these values
and then calls into msvs ? Maybe this is a message for gyp-developer@ to see if
there's other thinking?

[scottmg, 2013-04-16 18:02:53]

On 2013/04/16 18:01:02, Dirk Pranke wrote:
> On 2013/04/16 17:57:49, Shezan Baig (Bloomberg) wrote:
> > On 2013/04/16 17:28:28, Dirk Pranke wrote:
> > > I like the idea that you've pulled all of the ninja-specific stuff out of
> the
> > > main generator.
> > > 
> > > I don't like the idea that now the user has to specify so many commands
> > that'll
> > > be the same all the time (even if we put the commands in a gyp file). 
> > > 
> > > Maybe we should provide a default dictionary for _out_dir, _build_cmd, and
> > > _clean_cmd ?
> > 
> > 
> > I was planning to add these commands in src/build/common.gypi (in a separate
> > checkin), so it doesn't have to be defined in each gyp file.  The users just
> > need to specify "msvs_external_builder=ninja" in their GYP_DEFINES.  Does
that
> > sound more reasonable?
> 
> I'm fine w/ the msvs_external_builder=ninja setting, but it would be nice to
> have the defaults applied for non-chromium projects as well. It feels like it
> should just work out of the box to me.
> 
> Or maybe we should add a 'msvs-ninja' generator format that sets these values

I like that solution.

[Dirk Pranke, 2013-04-16 19:00:06]

On 2013/04/16 18:02:53, scottmg wrote:
> On 2013/04/16 18:01:02, Dirk Pranke wrote:
> > On 2013/04/16 17:57:49, Shezan Baig (Bloomberg) wrote:
> > > On 2013/04/16 17:28:28, Dirk Pranke wrote:
> > > > I like the idea that you've pulled all of the ninja-specific stuff out
of
> > the
> > > > main generator.
> > > > 
> > > > I don't like the idea that now the user has to specify so many commands
> > > that'll
> > > > be the same all the time (even if we put the commands in a gyp file). 
> > > > 
> > > > Maybe we should provide a default dictionary for _out_dir, _build_cmd,
and
> > > > _clean_cmd ?
> > > 
> > > 
> > > I was planning to add these commands in src/build/common.gypi (in a
separate
> > > checkin), so it doesn't have to be defined in each gyp file.  The users
just
> > > need to specify "msvs_external_builder=ninja" in their GYP_DEFINES.  Does
> that
> > > sound more reasonable?
> > 
> > I'm fine w/ the msvs_external_builder=ninja setting, but it would be nice to
> > have the defaults applied for non-chromium projects as well. It feels like
it
> > should just work out of the box to me.
> > 
> > Or maybe we should add a 'msvs-ninja' generator format that sets these
values
> 
> I like that solution.

Okay. Maybe we land this patch as-is and land an msvs_ninja.py as a separate
change?

[Shezan Baig (Bloomberg), 2013-04-16 19:02:44]

On 2013/04/16 19:00:06, Dirk Pranke wrote:
> Okay. Maybe we land this patch as-is and land an msvs_ninja.py as a separate
> change?

I'm currently working on a test for this, so I'd wait for that.  I think the
msvs_ninja.py change would quite trivial, so I'll probably include that as well
with my test patch.

[Shezan Baig (Bloomberg), 2013-04-16 22:24:43]

The latest patch includes tests, and now supports running gyp with
GYP_GENERATORS=msvs-ninja

When using the above export, it will automatically populate all the necessary
'msvs_external_builder_*' variables in the gyp targets.

[scottmg, 2013-04-16 22:48:00]

lgtm mod copyright and nits.

Dirk, do you have any idea on copyright? A grep doesn't show any copyrights
other than "Chromium" and "Google Inc" in gyp.

https://codereview.chromium.org/13867004/diff/19001/pylib/gyp/generator/msvs.py
File pylib/gyp/generator/msvs.py (right):

https://codereview.chromium.org/13867004/diff/19001/pylib/gyp/generator/msvs....
pylib/gyp/generator/msvs.py:1711: def _InitNinjaFlavor(options, target_list,
target_dicts):
nit; 2 line space

https://codereview.chromium.org/13867004/diff/19001/pylib/gyp/generator/msvs....
pylib/gyp/generator/msvs.py:1750: ]
nit; 2 line space

https://codereview.chromium.org/13867004/diff/19001/pylib/gyp/generator/msvs....
pylib/gyp/generator/msvs.py:3096: def _GetMSBuildExternalBuilderTargets(spec):
nit; 2 line space

https://codereview.chromium.org/13867004/diff/19001/pylib/gyp/generator/msvs....
pylib/gyp/generator/msvs.py:3119: 
nit; 2 line space

https://codereview.chromium.org/13867004/diff/19001/test/msvs/external_builde...
File test/msvs/external_builder/external.gyp (right):

https://codereview.chromium.org/13867004/diff/19001/test/msvs/external_builde...
test/msvs/external_builder/external.gyp:1: # Copyright (c) 2013 Bloomberg LP.
All rights reserved.
I think copyright is supposed to be assigned to Google Inc. (Actually, I have no
idea, but I know the presubmit looks for that). If that's a problem let me know,
and I'll find out what The Rules are.

[Dirk Pranke, 2013-04-16 22:49:55]

On 2013/04/16 22:48:00, scottmg wrote:
> lgtm mod copyright and nits.
> 
> Dirk, do you have any idea on copyright? A grep doesn't show any copyrights
> other than "Chromium" and "Google Inc" in gyp.
> 

We shouldn't be asking Shezan to assign copyright to google for this. I'll find
out what the right thing to do is.

-- Dirk

[Dirk Pranke, 2013-04-17 01:36:11]

On 2013/04/16 22:49:55, Dirk Pranke wrote:
> On 2013/04/16 22:48:00, scottmg wrote:
> > lgtm mod copyright and nits.
> > 
> > Dirk, do you have any idea on copyright? A grep doesn't show any copyrights
> > other than "Chromium" and "Google Inc" in gyp.
> > 
> 
> We shouldn't be asking Shezan to assign copyright to google for this. I'll
find
> out what the right thing to do is.
> 

Okay, apparently I was totally wrong and misinformed :). Copyright Google is
correct. Bloomberg is already listed in the AUTHORS file for GYP. This is just
how the CLA works for contributions.

-- Dirk

[Shezan Baig (Bloomberg), 2013-04-17 11:24:44]

Thanks!  I've updated the copyright info and also the nits.  Also, note that
this should wait for https://codereview.chromium.org/14322014/ to complete
before it can be used in the chromium repo.

[Dirk Pranke, 2013-04-19 19:26:06]

On 2013/04/17 11:24:44, Shezan Baig (Bloomberg) wrote:
> Thanks!  I've updated the copyright info and also the nits.  Also, note that
> this should wait for https://codereview.chromium.org/14322014/ to complete
> before it can be used in the chromium repo.

lgtm.

[iannucci, 2013-04-23 16:39:37]

FYI, I think some of the MSVS UI deficiencies can be worked around with macros
(i.e. making ctrl-f7 and f5 do the right things by overriding those hotkeys).
I'm not sure if there's a way to override the UI buttons/menus though (there
might be)

I presume that launching in the debugger works as you'd expect it to because the
output exe is correctly generated?

I said this in the other review, but thanks for working on this :)

[Shezan Baig (Bloomberg), 2013-04-23 16:47:45]

On 2013/04/23 16:39:37, iannucci wrote:
> FYI, I think some of the MSVS UI deficiencies can be worked around with macros
> (i.e. making ctrl-f7 and f5 do the right things by overriding those hotkeys).
> I'm not sure if there's a way to override the UI buttons/menus though (there
> might be)
> 


Yes, the popup on F5 can actually be disabled if you go to:
    Tools
    ->Options
      ->Projects and Solutions
        ->Build and Run
then set "On Run, when projects are out of date" to "Always build".

When this is set, hitting F5 will always launch ninja (even if ninja doesn't
actually have anything to do) before launching the exe.

Unfortunately, this is a Visual-Studio-wide option (i.e. it will be set for all
solutions/projects), I have yet to find a way to make this a per-solution or
per-project option.


> I presume that launching in the debugger works as you'd expect it to because
the
> output exe is correctly generated?

Yup :)


> I said this in the other review, but thanks for working on this :)


My pleasure :)

[scottmg, 2013-04-24 19:18:36]

Committed patchset #5 manually as r1618.

[scottmg, 2013-04-24 19:20:22]

On 2013/04/24 19:18:36, scottmg wrote:
> Committed patchset #5 manually as r1618.

and r1619. Can't roll into Chromium yet pending me fixing the ninja generator.