Avoid MSB8012 in GYP

Avoid MSB8012 on Windows

If using product extension only in build files, MSVS will complain that the
target extension does not match the product extension, MSB8012. Add gyp
support for specifying target extension to fix this warning.

Used for e.g. PyAuto/OpAuto .pyd files.

Patch from sigbjorn@opera.com.

R=scottmg@chromium.org

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

[operatest, 2013-09-10 09:47:46]

Hope you will be able to check if this patch is OK.

[Nico, 2013-09-10 14:14:30]

Can you add a test? Run `python gyptest.py -a` to run all tests. They are below
test/ .

[sigbjorn, 2013-09-11 14:36:36]

Had to install pywin32 and change devenv.com to vcexpress.exe in TestGyp.py in
order to run tests. I did not make a generic fix for this though. Note that MSB
warnings are not promotable to errors, so the test has to check the log instead.

For some reason 'git cl upload' hangs when I try to add a new file. 'git svn
info' also hangs (sometimes after a message about 'upgrading' the svn info), but
recommended online fixes of 'git config --remove-section svn-remote.svn' fails
with either 'error: could not lock config file .git/config' (when run from src)
or 'fatal: No such section!' (from src\tools\gyp). In short, I am unable to
upload my test, so adding a diff below. Suggestions for resolving my uploading
issues, or alternately applying the diff for me, would both be appreciated.

diff --git a/test/lib/TestGyp.py b/test/lib/TestGyp.py
index 930db75..bd9b9ea 100644
--- a/test/lib/TestGyp.py
+++ b/test/lib/TestGyp.py
@@ -155,6 +155,13 @@ class TestGypBase(TestCommon.TestCommon):
     """
     return self.must_not_match(self.built_file_path(name, **kw), contents)
 
+  def built_file_must_not_contain(self, name, contents, **kw):
+    """
+    Fails the test if the specified built file name contains the specified
+    contents.
+    """
+    return self.must_not_contain(self.built_file_path(name, **kw), contents)
+
   def copy_test_configuration(self, source_dir, dest_dir):
     """
     Copies the test configuration from the specified source_dir
diff --git a/test/target/gyptest-target.py b/test/target/gyptest-target.py
new file mode 100644
index 0000000..eb7bde6
--- /dev/null
+++ b/test/target/gyptest-target.py
@@ -0,0 +1,31 @@
+#!/usr/bin/env python
+
+# Copyright (C) 2013 Opera Software ASA.  All rights reserved.
+# This file is an original work developed by Opera Software ASA
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""
+Verifies simplest-possible build of a "Hello, world!" program
+using non-default extension. In particular, verifies that
+target_extension avoids MSB8012 for msvs.
+"""
+
+import TestGyp
+
+# Android does not support setting the build directory.
+test = TestGyp.TestGyp(formats=['!android'])
+
+test.run_gyp('target.gyp')
+test.build('target.gyp')
+
+# executables
+test.built_file_must_exist('hello.stuff', test.EXECUTABLE, bare=True)
+
+# check msvs log for errors
+if test.format == "msvs":
+  log_file = "obj\\hello\\hello.log"
+  test.built_file_must_exist(log_file)
+  test.built_file_must_not_contain(log_file, "MSB8012")
+
+test.pass_test()
diff --git a/test/target/hello.c b/test/target/hello.c
new file mode 100644
index 0000000..941a5e3
--- /dev/null
+++ b/test/target/hello.c
@@ -0,0 +1,8 @@
+/* Copyright (C) 2013 Opera Software ASA.  All rights reserved.
+ * This file is an original work developed by Opera Software ASA.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file. */
+
+void main(void) {
+  printf("Hello, world!\n");
+}
diff --git a/test/target/target.gyp b/test/target/target.gyp
new file mode 100644
index 0000000..592d1f4
--- /dev/null
+++ b/test/target/target.gyp
@@ -0,0 +1,18 @@
+# Copyright (C) 2013 Opera Software ASA.  All rights reserved.
+# This file is an original work developed by Opera Software ASA
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+{
+  'targets': [
+    {
+      'target_name': 'hello',
+      'product_extension': 'stuff',
+      'target_extension': 'stuff',
+      'type': 'executable',
+      'sources': [
+        'hello.c',
+      ],
+    }
+  ]
+}

[Nico, 2013-09-11 18:20:03]

scottmg might know how to upload gyp patches on windows

[scottmg, 2013-09-11 18:25:09]

On 2013/09/11 18:20:03, Nico wrote:
> scottmg might know how to upload gyp patches on windows

If you're in a chromium checkout, I put instructions for gyp here:
https://code.google.com/p/chromium/wiki/UsingGit#Working_with_repositories_in...
(near the bottom of that section)

If you're not in a chromium checkout, I tend to just use svn directly as it's
less hassle.

[sigbjorn, 2013-09-12 07:46:56]

Test added.

Turns out that 'git cl upload', new files and core.autocrlf set to true do not
work together. I had to disable core.autocrlf, and manually convert all files to
LF-endings to be able to upload.

[sigbjorn, 2013-09-17 07:25:15]

Ping. Is there something more wanted from my side?

[Nico, 2013-09-17 17:11:54]

https://codereview.chromium.org/23464051/diff/11001/test/target/hello.c
File test/target/hello.c (right):

https://codereview.chromium.org/23464051/diff/11001/test/target/hello.c#newcode1
test/target/hello.c:1: /* Copyright (C) 2013 Opera Software ASA.  All rights
reserved.
I'm not sure how we handle copyright headers in gyp. Maybe "The Chromium
Authors"? I don't know though. Mark might?

https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp
File test/target/target.gyp (right):

https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp#ne...
test/target/target.gyp:11: 'target_extension': 'stuff',
So would one always set this for every target that sets product_extension? In
that case, shouldn't product_extension just implicitly do this as a side effect
for msvc?

[Nico, 2013-09-17 17:12:18]

(And sorry for the delay! I'm somewhat clueless about Windows, so I was hoping
someone else would chime in.)

[sigbjorn, 2013-09-18 08:21:26]

https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp
File test/target/target.gyp (right):

https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp#ne...
test/target/target.gyp:11: 'target_extension': 'stuff',
MSB8012 is thrown when the two are different, so a simple fix would be to ensure
they are always the same. Though presumably there is a reason for why msvc has
the two of them separate, tying them together in GYP would make GYP less
flexible.

Note that the default setup in Opera's GYP files is that the target_extension is
set automatically based on the configuration type (e.g. Dynamic library (.dll)),
and the product extension based on the target extension. (I presume that
Chromium is similar.) So after the GYP fix, the fix was to change
"product_extension:.pyd" to "target_extension:.pyd", not to add another line, so
in a setup like that, there is no need for both of the declarations. (Although
since ninja does not support target_extension, product_extension had to be kept
in the .gyp file anyhow...)

[Nico, 2013-09-18 17:38:23]

https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp
File test/target/target.gyp (right):

https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp#ne...
test/target/target.gyp:11: 'target_extension': 'stuff',
On 2013/09/18 08:21:27, sigbjorn wrote:
> MSB8012 is thrown when the two are different, so a simple fix would be to
ensure
> they are always the same. Though presumably there is a reason for why msvc has
> the two of them separate, tying them together in GYP would make GYP less
> flexible.

Do you need this flexibility?

> 
> Note that the default setup in Opera's GYP files is that the target_extension
is
> set automatically based on the configuration type (e.g. Dynamic library
(.dll)),
> and the product extension based on the target extension. (I presume that
> Chromium is similar.) So after the GYP fix, the fix was to change
> "product_extension:.pyd" to "target_extension:.pyd", not to add another line,
so
> in a setup like that, there is no need for both of the declarations. (Although
> since ninja does not support target_extension, product_extension had to be
kept
> in the .gyp file anyhow...)

[sigbjorn, 2013-09-19 07:49:02]

https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp
File test/target/target.gyp (right):

https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp#ne...
test/target/target.gyp:11: 'target_extension': 'stuff',
On 2013/09/18 17:38:24, Nico wrote:
> On 2013/09/18 08:21:27, sigbjorn wrote:
> > MSB8012 is thrown when the two are different, so a simple fix would be to
> ensure
> > they are always the same. Though presumably there is a reason for why msvc
has
> > the two of them separate, tying them together in GYP would make GYP less
> > flexible.
> 
> Do you need this flexibility?

No. At least not right now at Opera.

[Nico, 2013-09-23 18:15:20]

On 2013/09/19 07:49:02, sigbjorn wrote:
> https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp
> File test/target/target.gyp (right):
> 
>
https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp#ne...
> test/target/target.gyp:11: 'target_extension': 'stuff',
> On 2013/09/18 17:38:24, Nico wrote:
> > On 2013/09/18 08:21:27, sigbjorn wrote:
> > > MSB8012 is thrown when the two are different, so a simple fix would be to
> > ensure
> > > they are always the same. Though presumably there is a reason for why msvc
> has
> > > the two of them separate, tying them together in GYP would make GYP less
> > > flexible.
> > 
> > Do you need this flexibility?
> 
> No. At least not right now at Opera.

Then I wouldn't expose a new gyp setting for this.

[sigbjorn, 2013-09-24 08:08:57]

On 2013/09/23 18:15:20, Nico wrote:
> On 2013/09/19 07:49:02, sigbjorn wrote:
> > https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp
> > File test/target/target.gyp (right):
> > 
> >
>
https://codereview.chromium.org/23464051/diff/11001/test/target/target.gyp#ne...
> > test/target/target.gyp:11: 'target_extension': 'stuff',
> > On 2013/09/18 17:38:24, Nico wrote:
> > > On 2013/09/18 08:21:27, sigbjorn wrote:
> > > > MSB8012 is thrown when the two are different, so a simple fix would be
to
> > > ensure
> > > > they are always the same. Though presumably there is a reason for why
msvc
> > has
> > > > the two of them separate, tying them together in GYP would make GYP less
> > > > flexible.
> > > 
> > > Do you need this flexibility?
> > 
> > No. At least not right now at Opera.
> 
> Then I wouldn't expose a new gyp setting for this.

So you would prefer a fix which explicitly sets target_extension whenever
product_extension is set? Would this work for all other projects using GYP?

I'll update the review if this is the way we want to to.

[scottmg, 2013-09-24 17:23:19]

Sorry, I don't use VS either, but I think this is fine.

lgtm

https://codereview.chromium.org/23464051/diff/11001/test/target/hello.c
File test/target/hello.c (right):

https://codereview.chromium.org/23464051/diff/11001/test/target/hello.c#newcode1
test/target/hello.c:1: /* Copyright (C) 2013 Opera Software ASA.  All rights
reserved.
ISTR it's always supposed to say Google for some reason.

[sigbjorn, 2013-09-30 15:14:36]

My apologies, I am still new to Chromium code review workflow, and I didn't find
a tutorial. The last comment is green, and has "lgtm", does that mean I should
submit somewhere?

On 2013/09/24 17:23:19, scottmg wrote:
> Sorry, I don't use VS either, but I think this is fine.

Not quite sure what you are replying to; do you think the patch as-is is fine,
or that the suggested fix to the patch sounds fine?

> https://codereview.chromium.org/23464051/diff/11001/test/target/hello.c
> File test/target/hello.c (right):
> 
>
https://codereview.chromium.org/23464051/diff/11001/test/target/hello.c#newcode1
> test/target/hello.c:1: /* Copyright (C) 2013 Opera Software ASA.  All rights
> reserved.
> ISTR it's always supposed to say Google for some reason.

But yet this was marked as lgtm, so I am a bit confused.

[scottmg, 2013-09-30 16:01:43]

On 2013/09/30 15:14:36, sigbjorn wrote:
> My apologies, I am still new to Chromium code review workflow, and I didn't
find
> a tutorial. The last comment is green, and has "lgtm", does that mean I should
> submit somewhere?
> 
> On 2013/09/24 17:23:19, scottmg wrote:
> > Sorry, I don't use VS either, but I think this is fine.
> 
> Not quite sure what you are replying to; do you think the patch as-is is fine,
> or that the suggested fix to the patch sounds fine?

Sorry for the confusion. I didn't mean to lgtm. There's an email reply that
didn't show up here which makes it more confusing.

I think we should not expose the extra setting, and just make sure the two
extensions match. Assuming that doesn't break any tests, I think we might as
well keep things simple unless we have some reason not to.

In addition, the copyrights should be Google. (I don't really know why, but I
don't pretend to understand copyright.)

Once the patch is updated, I can land the change for you.

> 
> > https://codereview.chromium.org/23464051/diff/11001/test/target/hello.c
> > File test/target/hello.c (right):
> > 
> >
>
https://codereview.chromium.org/23464051/diff/11001/test/target/hello.c#newcode1
> > test/target/hello.c:1: /* Copyright (C) 2013 Opera Software ASA.  All rights
> > reserved.
> > ISTR it's always supposed to say Google for some reason.
> 
> But yet this was marked as lgtm, so I am a bit confused.

[scottmg, 2013-09-30 16:02:23]

On 2013/09/30 16:01:43, scottmg wrote:
> On 2013/09/30 15:14:36, sigbjorn wrote:
> > My apologies, I am still new to Chromium code review workflow, and I didn't
> find
> > a tutorial. The last comment is green, and has "lgtm", does that mean I
should
> > submit somewhere?
> > 
> > On 2013/09/24 17:23:19, scottmg wrote:
> > > Sorry, I don't use VS either, but I think this is fine.
> > 
> > Not quite sure what you are replying to; do you think the patch as-is is
fine,
> > or that the suggested fix to the patch sounds fine?
> 
> Sorry for the confusion. I didn't mean to lgtm. There's an email reply that
> didn't show up here which makes it more confusing.
> 
> I think we should not expose the extra setting, and just make sure the two
> extensions match. Assuming that doesn't break any tests, I think we might as
> well keep things simple unless we have some reason not to.
> 
> In addition, the copyrights should be Google. (I don't really know why, but I
> don't pretend to understand copyright.)
> 
> Once the patch is updated, I can land the change for you.
> 
> > 
> > > https://codereview.chromium.org/23464051/diff/11001/test/target/hello.c
> > > File test/target/hello.c (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23464051/diff/11001/test/target/hello.c#newcode1
> > > test/target/hello.c:1: /* Copyright (C) 2013 Opera Software ASA.  All
rights
> > > reserved.
> > > ISTR it's always supposed to say Google for some reason.
> > 
> > But yet this was marked as lgtm, so I am a bit confused.

heh, and apparently quoting l g t m counts. :( not lgtm yet, anyway.

[sigbjorn, 2013-10-01 10:08:23]

Now using product_extension to set TargetExt, ignoring any target_extension.

input.py has been changed back, though the UI doesn't seem to reflect that, so
should be confirmed when landing.

[sigbjorn, 2013-10-04 07:20:59]

Ping

[scottmg, 2013-10-04 16:37:58]

Thanks, lgtm. I can land it for you.

[scottmg, 2013-10-04 16:56:54]

Test needs to be guarded on sys.platform in ('win32', 'cygwin') per failing
trybots.

[scottmg, 2013-10-04 22:05:19]

Committed patchset #5 manually as r1749.