Convert 'msvs_prebuild' and 'msvs_postbuild' for MSBuild (Visual C++ 2010). (issue 8229003)

[yuk...@google.com]

Reviewers: jeanluc,

Message:
Hi Jean-Luc,

Currently 'msvs_prebuild' and 'msvs_postbuild' settings in the gyp file are
ignored when -G msvs_version=2010 is specified.
Can we use <PreBuildEvent> and <PostBuildEvent> element to support them?

I made a patch for this.
Can you review it?

Description:
Translate 'msvs_prebuild' and 'msvs_postbuild' for MSBuild (Visual C++
2010).
Currently 'msvs_prebuild' and 'msvs_postbuild' settings in the gyp file are
ignored when -G msvs_version=2010 is specified.
This CL allows the msvs generator to translate these settings into
<PreBuildEvent> and <PostBuildEvent> respectively.

Please review this at http://codereview.chromium.org/8229003/

SVN Base: http://gyp.googlecode.com/svn/trunk/

Affected files:
M pylib/gyp/generator/msvs.py
A test/msvs/buildevents/buildevents.gyp
A test/msvs/buildevents/gyptest-all.py
A test/msvs/buildevents/main.cc

Index: pylib/gyp/generator/msvs.py
===================================================================
--- pylib/gyp/generator/msvs.py (revision 1067)
+++ pylib/gyp/generator/msvs.py (working copy)
@@ -2543,7 +2543,7 @@
# Visual Studio 2010 has TR1
defines = [d for d in defines if d != '_HAS_TR1=0']
# Warn of ignored settings
- ignored_settings =
['msvs_prebuild', 'msvs_postbuild', 'msvs_tool_files']
+ ignored_settings = ['msvs_tool_files']
for ignored_setting in ignored_settings:
value = configuration.get(ignored_setting)
if value:
@@ -2552,9 +2552,8 @@

defines = [_EscapeCppDefineForMSBuild(d) for d in defines]
disabled_warnings = _GetDisabledWarnings(configuration)
- # TODO(jeanluc) Validate & warn that we don't translate
- # prebuild = configuration.get('msvs_prebuild')
- # postbuild = configuration.get('msvs_postbuild')
+ prebuild = configuration.get('msvs_prebuild')
+ postbuild = configuration.get('msvs_postbuild')
def_file = _GetModuleDefinition(spec)
precompiled_header = configuration.get('msvs_precompiled_header')

@@ -2595,6 +2594,10 @@
if def_file:
_ToolAppend(msbuild_settings, 'Link', 'ModuleDefinitionFile', def_file)
configuration['finalized_msbuild_settings'] = msbuild_settings
+ if prebuild:
+ _ToolAppend(msbuild_settings, 'PreBuildEvent', 'Command', prebuild)
+ if postbuild:
+ _ToolAppend(msbuild_settings, 'PostBuildEvent', 'Command', postbuild)

def _GetValueFormattedForMSBuild(tool_name, name, value):
Index: test/msvs/buildevents/buildevents.gyp
===================================================================
--- test/msvs/buildevents/buildevents.gyp (revision 0)
+++ test/msvs/buildevents/buildevents.gyp (revision 0)
@@ -0,0 +1,14 @@
+# Copyright (c) 2011 Google Inc. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+{
+ 'targets': [
+ {
+ 'target_name': 'main',
+ 'type': 'executable',
+ 'sources': [ 'main.cc', ],
+ 'msvs_prebuild': r'echo starting',
+ 'msvs_postbuild': r'echo finished',
+ },
+ ],
+}
Index: test/msvs/buildevents/gyptest-all.py
===================================================================
--- test/msvs/buildevents/gyptest-all.py (revision 0)
+++ test/msvs/buildevents/gyptest-all.py (revision 0)
@@ -0,0 +1,23 @@
+#!/usr/bin/env python
+
+# Copyright (c) 2011 Google Inc. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""
+Verifies that precompiled headers can be specified.
+"""
+
+import TestGyp
+
+test = TestGyp.TestGyp(formats=['msvs'], workdir='workarea_all')
+
+test.run_gyp('buildevents.gyp', '-G', 'msvs_version=2008')
+test.must_contain('main.vcproj', 'Name="VCPreBuildEventTool"')
+test.must_contain('main.vcproj', 'Name="VCPostBuildEventTool"')
+
+test.run_gyp('buildevents.gyp', '-G', 'msvs_version=2010')
+test.must_contain('main.vcxproj', '<PreBuildEvent>')
+test.must_contain('main.vcxproj', '<PostBuildEvent>')
+
+test.pass_test()
Index: test/msvs/buildevents/main.cc
===================================================================
--- test/msvs/buildevents/main.cc (revision 0)
+++ test/msvs/buildevents/main.cc (revision 0)
@@ -0,0 +1,4 @@
+// Copyright (c) 2011 Google Inc. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+int main() {}

[mar...@chromium.org]

On 2011/10/11 08:45:23, yukawa wrote:
> Currently 'msvs_prebuild' and 'msvs_postbuild' settings in the gyp file
> are
> ignored when -G msvs_version=2010 is specified.
> Can we use <PreBuildEvent> and <PostBuildEvent> element to support them?

These two functionality are really bad and are a maintenance burden. I'd
prefer
them to not be implemented. I'd prefer you to use actions instead.

http://codereview.chromium.org/8229003/

[Scott Graham]

Per comments from this review http://codereview.chromium.org/7633024/#msg2 I'd like msvs_postbuild to exist too.

I understand PostBuild often causes brokenness (and isn't the right default solution to any problem), but at least in 2008 actions don't seem cover all cases. Are they improved to be more flexible in 2010?

[Marc-Antoine Ruel]

Ok.

[Bradley Nelson]

On Tue, Oct 11, 2011 at 5:56 PM, Marc-Antoine Ruel <mar...@chromium.org> wrote:

Ok.

Le 11 octobre 2011 16:33, Scott Graham <sco...@chromium.org> a écrit :

Per comments from this review http://codereview.chromium.org/7633024/#msg2 I'd like msvs_postbuild to exist too.

I understand PostBuild often causes brokenness (and isn't the right default solution to any problem), but at least in 2008 actions don't seem cover all cases. Are they improved to be more flexible in 2010?

There are two issue with them:

1. Without further embellishment incredibuild didn't use to support them (there is now an echo syntax to support them).

2. They're not support by other generators, so you get something non-cross platform.

When jeanluc was adding msbuild, I actually originally considered having him drop them.

In the corpus on codesearch.chromium.org, the only instance of msvs_postbuild is in o3d, which never supported incredibuild (and is a deprecated project).

msvs_prebuild doesn't appear anywhere.

Admittedly postbuild is kind of a fit for a strip type step, but that can be done with actions + none targets.

What's the use case?

-BradN

[Yohei Yukawa]

On Thu, Oct 13, 2011 at 9:17 AM, Bradley Nelson <bradn...@google.com> wrote:

On Tue, Oct 11, 2011 at 5:56 PM, Marc-Antoine Ruel <mar...@chromium.org> wrote:

Ok.

Le 11 octobre 2011 16:33, Scott Graham <sco...@chromium.org> a écrit :

Per comments from this review http://codereview.chromium.org/7633024/#msg2 I'd like msvs_postbuild to exist too.

I understand PostBuild often causes brokenness (and isn't the right default solution to any problem), but at least in 2008 actions don't seem cover all cases. Are they improved to be more flexible in 2010?

There are two issue with them:

1. Without further embellishment incredibuild didn't use to support them (there is now an echo syntax to support them).

2. They're not support by other generators, so you get something non-cross platform.

When jeanluc was adding msbuild, I actually originally considered having him drop them.

In the corpus on codesearch.chromium.org, the only instance of msvs_postbuild is in o3d, which never supported incredibuild (and is a deprecated project).

msvs_prebuild doesn't appear anywhere.

Admittedly postbuild is kind of a fit for a strip type step, but that can be done with actions + none targets.

What's the use case?

-BradN

Currently our project (Google Japanese Input) is using 'msvs_postbuild' mainly for code signing. Our project has a lot of executables so it is convenient for us to reuse the same 'msvs_postbuild' by including one gypi file. $(TargetPath) macro is very convenient for this scenario.

Of course we can migrate to actions + none targets solution but we have to set the executable names manually in this case because $(TargetPath) is not available in none target, right?

By the way, gyp has built-in 'postbuilds' keyword for Xcode. Is there any plan to support it for msvs generator? 

http://code.google.com/p/gyp/source/browse/trunk/test/mac/postbuilds/test.gyp