homogenize msvs and ninja library_dir and libraries settings for Windows

homogenize msvs and ninja library_dir and libraries settings for Windows

Since the introduction of r1657, which added library_dirs support for
ninja, the test case did not exercise the win flavor properly, which
should translate -lfoo to foo.lib.  Thus, the generation between msvs
and ninja differed resulting in ninja being unable to link libraries
that used the '-lfoo' form of specifiying libraries.  It turns out that
a name without an extension gets translated as a .obj when using the
msvs compiler.

This patch modifies 3 things:
1) In msvs_emulation.py, translate -lfoo to foo.lib and only add the
   .lib suffix if it did not already exist (similar to _GetLibraries
   in msvs.py).

2) Update the library_dirs test case to remove the usage of:
     'libraries': [
       '<(STATIC_LIB_PREFIX)mylib<(STATIC_LIB_SUFFIX)',
     ],
   so that all test cases test against '-lmylib' to catch any
   translation errors.

   Add an additional test case to ensure that '-lmylib.lib' is
   translated to 'mylib.lib' for the win flavor of ninja.

3) While not critical, change the translation of library_dirs for the
   win flavor of ninja to use /LIBPATH instead of -LIBPATH and moved the
   QutoeShellArgument() call around the path, not the entire setting to
   keep it consistent with the msvs generator.

R=scottmg@chromium.org

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

[ericvw, 2013-09-26 19:41:12]

initial patch uploaded

[scottmg, 2013-09-26 19:50:04]

https://codereview.chromium.org/24792003/diff/1/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/24792003/diff/1/pylib/gyp/msvs_emulation.py#n...
pylib/gyp/msvs_emulation.py:205: return [lib[2:] + '.lib' if
lib.startswith('-l') else lib
Hmm, unfortunately:

https://code.google.com/p/chromium/codesearch#search/&q=\-l.*\.lib&sq=package...

So I don't think this will work as is.

[ericvw, 2013-09-26 19:57:13]

OK.  I will change it to check if the .lib extension is already there similar to
msvs.py:1159

[ericvw, 2013-09-26 20:10:29]

Uploaded new patch based on feedback.  The .lib extension is now conditionally
added if it did not already exist.

[scottmg, 2013-09-26 20:20:47]

https://codereview.chromium.org/24792003/diff/6001/test/library_dirs/subdir/t...
File test/library_dirs/subdir/test.gyp (right):

https://codereview.chromium.org/24792003/diff/6001/test/library_dirs/subdir/t...
test/library_dirs/subdir/test.gyp:55: '-lmylib',
since we're defacto supporting -lmylib.lib, please add a test for that too.

[ericvw, 2013-09-26 22:39:31]

>
https://codereview.chromium.org/24792003/diff/6001/test/library_dirs/subdir/t...
> test/library_dirs/subdir/test.gyp:55: '-lmylib',
> since we're defacto supporting -lmylib.lib, please add a test for that too.

Should that go under test/win or test/library_dirs?  If it goes under
test/library_dirs, I would need to switch based on the platform in either the
gyptest-*.py and/or the .gyp file.

Either way I am happy to write the test case, but I would appreciate some input
on where a good home for it is.  Thanks.

[scottmg, 2013-09-26 23:06:05]

On 2013/09/26 22:39:31, ericvw wrote:
> >
>
https://codereview.chromium.org/24792003/diff/6001/test/library_dirs/subdir/t...
> > test/library_dirs/subdir/test.gyp:55: '-lmylib',
> > since we're defacto supporting -lmylib.lib, please add a test for that too.
> 
> Should that go under test/win or test/library_dirs?  If it goes under
> test/library_dirs, I would need to switch based on the platform in either the
> gyptest-*.py and/or the .gyp file.
> 
> Either way I am happy to write the test case, but I would appreciate some
input
> on where a good home for it is.  Thanks.

I think keeping the test near this one, and then only running that gyp file when
"sys.platform in ('win32', 'cygwin')" in gyptest-*.py would be best.

[ericvw, 2013-09-27 16:30:46]

Uploaded patch set that tests -lmylib.lib as requested.  Currently this adds
conditionally execution in the existing gyptest-*.py file for test/library_dirs.
 I can split it out to a separate one if that is more desirable.

[scottmg, 2013-09-27 20:38:47]

https://codereview.chromium.org/24792003/diff/14001/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/24792003/diff/14001/pylib/gyp/msvs_emulation....
pylib/gyp/msvs_emulation.py:205: libs =  [lib[2:] if lib.startswith('-l') else
lib for lib in libraries]
nit; extra space before list comprehension

https://codereview.chromium.org/24792003/diff/14001/test/library_dirs/gyptest...
File test/library_dirs/gyptest-library-dirs.py (right):

https://codereview.chromium.org/24792003/diff/14001/test/library_dirs/gyptest...
test/library_dirs/gyptest-library-dirs.py:35: test.run_gyp('test-win.gyp',
nit; 2 space indent

https://codereview.chromium.org/24792003/diff/14001/test/library_dirs/subdir/...
File test/library_dirs/subdir/test-win.gyp (right):

https://codereview.chromium.org/24792003/diff/14001/test/library_dirs/subdir/...
test/library_dirs/subdir/test-win.gyp:60: 'libraries': [
delete this branch condition?

[ericvw, 2013-09-27 21:48:01]

uploaded new patch set based on most recent feedback.

https://codereview.chromium.org/24792003/diff/14001/test/library_dirs/subdir/...
File test/library_dirs/subdir/test-win.gyp (right):

https://codereview.chromium.org/24792003/diff/14001/test/library_dirs/subdir/...
test/library_dirs/subdir/test-win.gyp:60: 'libraries': [
On 2013/09/27 20:38:47, scottmg wrote:
> delete this branch condition?

I did this originally in the event it did get run on a non-win platform. 
However, since this .gyp file is only executed conditionally from
gyptest-library-dirs.py on these platforms, I will remove the condition in the
.gyp file.

[scottmg, 2013-09-27 21:51:13]

Thanks! LGTM.

[ericvw, 2013-09-27 23:39:18]

On 2013/09/27 21:51:13, scottmg wrote:
> Thanks! LGTM.

Description updated to reflect current changes.

[scottmg, 2013-09-28 03:21:45]

Committed patchset #4 manually as r1743.

[ericvw, 2013-09-28 13:14:57]

On 2013/09/28 03:21:45, scottmg wrote:
> Committed patchset #4 manually as r1743.

Oops.  I forgot the Contributed By: in the description -- next time :).  Thanks
for the commit.

[Nico, 2013-10-01 19:17:48]

This apparently broke xcode tests on mac (look for '-lmylib'):

Build settings from command line:
    SYMROOT = $SRCROOT/build

=== BUILD NATIVE TARGET libraries-search-path-test OF PROJECT test WITH THE
DEFAULT CONFIGURATION (Default) ===
Check dependencies
[WARN]warning: skipping file
'/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/-lmylib'
(unexpected file type 'text' in Frameworks & Libraries build phase)

CompileC
build/test.build/Default/libraries-search-path-test.build/Objects-normal/i386/hello.o
hello.cc normal i386 c++ com.apple.compilers.llvmgcc42
    cd
/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir
    setenv LANG en_US.US-ASCII
    /Developer/usr/bin/llvm-gcc-4.2 -x c++ -arch i386 -fmessage-length=0 -pipe
-Wno-trigraphs -fpascal-strings -Os -fasm-blocks -gdwarf-2 -fvisibility=hidden
-fvisibility-inlines-hidden
-I/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/build/test.build/Default/libraries-search-path-test.build/libraries-search-path-test.hmap
-I/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/build/Default/include
-I/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/build/test.build/Default/libraries-search-path-test.build/DerivedSources/i386
-I/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/build/test.build/Default/libraries-search-path-test.build/DerivedSources
-F/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/build/Default
-c
/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/hello.cc
-o
/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/build/test.build/Default/libraries-search-path-test.build/Objects-normal/i386/hello.o

Ld build/Default/libraries-search-path-test normal i386
    cd
/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir
    /Developer/usr/bin/llvm-g++-4.2 -arch i386
-L/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/build/Default
-L/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/secret_location
-F/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/build/Default
-filelist
/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/build/test.build/Default/libraries-search-path-test.build/Objects-normal/i386/libraries-search-path-test.LinkFileList
-o
/Volumes/data/b/build/slave/gyp/build/trunk/out/testworkarea/testgyp.Wmu5NL/subdir/build/Default/libraries-search-path-test
Undefined symbols for architecture i386:
  "my_foo(int)", referenced from:
      _main in hello.o
ld: symbol(s) not found for architecture i386

Using -lmylib on non-linux isn't correct. Can you fix this quickly?

[ericvw, 2013-10-01 19:33:55]

On 2013/10/01 19:17:48, Nico wrote:
> This apparently broke xcode tests on mac (look for '-lmylib'):
> ...
> Using -lmylib on non-linux isn't correct. Can you fix this quickly?

I will look into this today.  I expected the patch to only affect the win flavor
of ninja.

[Shezan Baig (Bloomberg), 2013-10-01 19:37:27]

On 2013/10/01 19:17:48, Nico wrote:
> Using -lmylib on non-linux isn't correct. Can you fix this quickly?

Reverting the "test/library_dirs/subdir/test.gyp" change in this CL will
probably fix the mac test.  I don't understand this comment though.  Shouldn't
we be converting '-lmylib' to whatever convention the mac platform requires? 
Otherwise gyp files would be littered with platform-specific lib declarations.

[Nico, 2013-10-01 19:45:59]

On 2013/10/01 19:37:27, Shezan Baig (Bloomberg) wrote:
> On 2013/10/01 19:17:48, Nico wrote:
> > Using -lmylib on non-linux isn't correct. Can you fix this quickly?
> 
> Reverting the "test/library_dirs/subdir/test.gyp" change in this CL will
> probably fix the mac test.  I don't understand this comment though.  Shouldn't
> we be converting '-lmylib' to whatever convention the mac platform requires? 
> Otherwise gyp files would be littered with platform-specific lib declarations.

I didn't look at the patch itself.

I think converting -lfoo is a bad idea. The property should just say "foo" and
then this should be converted to -lfoo on linux and to other things on other
platforms.

[ericvw, 2013-10-04 20:49:36]

Sorry, please ignore patch set 5.  I attempted to upload this to a new CL and
failed.