Adds support for the library_dirs key, which appears in the documentation but was never actually im…

Adds support for the library_dirs key, which appears in the documentation but was never actually implemented.
library_dirs is essentially equivalent to adding "-Lfoo" to ldflags in Make, modifying LIBRARY_SEARCH_PATHS in Xcode, or adding AdditionalLibraryPaths to the VCLinkerTool or VCLibrarianTool in MSVS.  I have attempted to make an analogous change to the ninja generator, but have not been able to test it (ninja was not in the tree I checked out) and I am fairly sure it's wrong.  Guidance here would be appreciated.

I have created a modified version of the mac/libraries test to test this change; a cross-platform test proved impossible because I needed to copy the lib somewhere, and doing so with a copies section caused a circular dependency which caused Make to fail (though it worked fine in Xcode).  For some reason, although the implementation of library_dirs is a straightforward addition to LIBRARY_SEARCH_PATHS, simply substituting the former for the latter did not work until I removed the <(DEPTH) reference from the library_dirs list.  This may indicate a bug in my code, but I'm not certain, since neither test/mac/library_dir nor test/mac/libraries worked for me when I tried building their gypfiles directly and building the products in Xcode.

[Brian Ellis, 2012-07-17 19:53:08]

Ah, found it. :-)

[Mark Mentovai, 2012-07-17 19:58:33]

+ a couple other generator maintainers to make sure that the changes to all of
the generators are OK.

http://chromiumcodereview.appspot.com/10749020/diff/16001/pylib/gyp/input.py
File pylib/gyp/input.py (right):

http://chromiumcodereview.appspot.com/10749020/diff/16001/pylib/gyp/input.py#...
pylib/gyp/input.py:38: 'library_dirs',
It shouldn’t be neecssary to add anything ending with _dirs to this list. See
IsPathSection.

http://chromiumcodereview.appspot.com/10749020/diff/16001/test/mac/library_di...
File test/mac/library_dirs/subdir/test.gyp (right):

http://chromiumcodereview.appspot.com/10749020/diff/16001/test/mac/library_di...
test/mac/library_dirs/subdir/test.gyp:34: '${SRCROOT}/../secret_location',
Dumping stuff into the source tree is bad news. Can you come up with a way to
test this that doesn’t do that?

[Brian Ellis, 2012-07-17 20:26:52]

http://chromiumcodereview.appspot.com/10749020/diff/16001/pylib/gyp/input.py
File pylib/gyp/input.py (right):

http://chromiumcodereview.appspot.com/10749020/diff/16001/pylib/gyp/input.py#...
pylib/gyp/input.py:38: 'library_dirs',
On 2012/07/17 19:58:34, Mark Mentovai wrote:
> It shouldn’t be neecssary to add anything ending with _dirs to this list. See
> IsPathSection.

Okay.  I was surprised by the inclusion of include_dirs, which is why I added
this.  Will remove again.

http://chromiumcodereview.appspot.com/10749020/diff/16001/test/mac/library_di...
File test/mac/library_dirs/subdir/test.gyp (right):

http://chromiumcodereview.appspot.com/10749020/diff/16001/test/mac/library_di...
test/mac/library_dirs/subdir/test.gyp:34: '${SRCROOT}/../secret_location',
On 2012/07/17 19:58:34, Mark Mentovai wrote:
> Dumping stuff into the source tree is bad news. Can you come up with a way to
> test this that doesn’t do that?

Hm, most of this is copied directly from test/mac/libraries/subdir/test.gyp... 
I think the reason they did it this way is that if I put the lib in ${OBJROOT}
or the like, it'll get picked up by gyp by default even without setting the
library search paths.  It's possible we could bury it in a subdirectory of
${OBJROOT} instead, but my tests indicate that will only work with Xcode because
OBJROOT isn't in the environment otherwise (frankly I'm not sure how make or
ninja deal with ${SRCROOT} either, but it appears they do somehow).  Any
suggestions?

[bradn, 2012-07-17 21:17:31]

msvs looks ok, but it would be nice if the test got run there :-)

[Brian Ellis, 2012-07-17 21:24:42]

On 2012/07/17 21:17:31, bradn wrote:
> msvs looks ok, but it would be nice if the test got run there :-)

Agreed.  I was unable to find a platform-agnostic way of testing, though (and
the fact that the libraries test only runs on Mac means at least one other
developer had the same trouble).  If we find a solution to the
polluting-the-output-directory problem that means I can do something similar in
MSVS, I'll do that.

[Nico, 2012-07-21 19:14:48]

Very cool! Let's try to get the test working for all generators though.

http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
File pylib/gyp/generator/make.py (right):

http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
pylib/gyp/generator/make.py:1421: library_dirs = spec.get('library_dirs', [])
Do you need to grab this from |spec|? Isn't just |config| enough?

http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
File pylib/gyp/generator/ninja.py (right):

http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
pylib/gyp/generator/ninja.py:747: for l in library_dirs])
This will might clash with the variable in line 892. You probably want a new
variable for this.

Also, you need to update the 'cc', 'objcc' etc rules at the end of this file to
actually use this new variable.

http://chromiumcodereview.appspot.com/10749020/diff/23001/test/mac/library_di...
File test/mac/library_dirs/subdir/test.gyp (right):

http://chromiumcodereview.appspot.com/10749020/diff/23001/test/mac/library_di...
test/mac/library_dirs/subdir/test.gyp:34: '${SRCROOT}/../secret_location',
Can you use <(PRODUCT_DIR)/secret_subdir instead? That should work with all
generators I think.

http://chromiumcodereview.appspot.com/10749020/diff/23001/test/mac/library_di...
test/mac/library_dirs/subdir/test.gyp:41:
'${BUILT_PRODUCTS_DIR}/${EXECUTABLE_PATH}',
<(LIB_DIR)/<(STATIC_LIB_PREFIX)mylib<(STATIC_LIB_SUFFIX) should again work with
all generators

[Brian Ellis, 2012-07-23 23:30:04]

On 2012/07/21 19:14:48, Nico wrote:
> Very cool! Let's try to get the test working for all generators though.
> 
>
http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
> File pylib/gyp/generator/make.py (right):
> 
>
http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
> pylib/gyp/generator/make.py:1421: library_dirs = spec.get('library_dirs', [])
> Do you need to grab this from |spec|? Isn't just |config| enough?
> 
>
http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
> File pylib/gyp/generator/ninja.py (right):
> 
>
http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
> pylib/gyp/generator/ninja.py:747: for l in library_dirs])
> This will might clash with the variable in line 892. You probably want a new
> variable for this.
> 
> Also, you need to update the 'cc', 'objcc' etc rules at the end of this file
to
> actually use this new variable.
> 
>
http://chromiumcodereview.appspot.com/10749020/diff/23001/test/mac/library_di...
> File test/mac/library_dirs/subdir/test.gyp (right):
> 
>
http://chromiumcodereview.appspot.com/10749020/diff/23001/test/mac/library_di...
> test/mac/library_dirs/subdir/test.gyp:34: '${SRCROOT}/../secret_location',
> Can you use <(PRODUCT_DIR)/secret_subdir instead? That should work with all
> generators I think.
> 
>
http://chromiumcodereview.appspot.com/10749020/diff/23001/test/mac/library_di...
> test/mac/library_dirs/subdir/test.gyp:41:
> '${BUILT_PRODUCTS_DIR}/${EXECUTABLE_PATH}',
> <(LIB_DIR)/<(STATIC_LIB_PREFIX)mylib<(STATIC_LIB_SUFFIX) should again work
with
> all generators

Getting the test working for MSVS is going to be a pain in the butt, since the
postbuild action syntax will need to be completely different.  I'm having a lot
of trouble just getting the generic version of the postbuild working in Xcode. 
I hope I'm doing something wrong and that it isn't really this buggy.

[Brian Ellis, 2012-07-23 23:30:26]

http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
File pylib/gyp/generator/make.py (right):

http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
pylib/gyp/generator/make.py:1421: library_dirs = spec.get('library_dirs', [])
On 2012/07/21 19:14:48, Nico wrote:
> Do you need to grab this from |spec|? Isn't just |config| enough?

Probably, yeah.  I wasn't sure whether dictionary merging had been performed at
this point, but apparently so.

http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
File pylib/gyp/generator/ninja.py (right):

http://chromiumcodereview.appspot.com/10749020/diff/23001/pylib/gyp/generator...
pylib/gyp/generator/ninja.py:747: for l in library_dirs])
On 2012/07/21 19:14:48, Nico wrote:
> This will might clash with the variable in line 892. You probably want a new
> variable for this.
> 
> Also, you need to update the 'cc', 'objcc' etc rules at the end of this file
to
> actually use this new variable.

I think you mean the 'solink', 'link', etc. rules, but otherwise done.  I have
no real idea what $libs$postbuilds does, so I added my new variable there as
well; please let me know if that's wrong, or if I need to manually use /LIBPATH
on Windows, or anything like that.

http://chromiumcodereview.appspot.com/10749020/diff/23001/test/mac/library_di...
File test/mac/library_dirs/subdir/test.gyp (right):

http://chromiumcodereview.appspot.com/10749020/diff/23001/test/mac/library_di...
test/mac/library_dirs/subdir/test.gyp:34: '${SRCROOT}/../secret_location',
On 2012/07/21 19:14:48, Nico wrote:
> Can you use <(PRODUCT_DIR)/secret_subdir instead? That should work with all
> generators I think.

These changes aren't working for me.  Make is happy, but the Xcode generator is
complaining:

Script-0F6C1866549FDC3E18491E4F.sh: line 2: BUILT_PRODUCTS_DIR: command not
found

This happens whether or not I quote the pathnames using double or (escaped)
single quotes.  It sounds like the shell is getting the raw string
"$(BUILT_PRODUCTS_DIR)" and treating it as a subshell invocation.  Maybe there's
a bug in EncodePOSIXShellList?  Just to sanity check, my postbuilds look like
this:


      'postbuilds': [
        {
          'postbuild_name': 'Make a secret location',
          'action': [
            'mkdir',
            '-p',
            '"<(PRODUCT_DIR)/../secret_location"',
          ],
        },
        {
          'postbuild_name': 'Copy to secret location, with secret name',
          'action': [
            'cp',
            '"<(LIB_DIR)/<(STATIC_LIB_PREFIX)mylib<(STATIC_LIB_SUFFIX)"',
           
'"<(PRODUCT_DIR)/../secret_location/<(STATIC_LIB_PREFIX)mysecretlib<(STATIC_LIB_SUFFIX)"',
          ],
        },
      ],

[Nico, 2012-07-23 23:37:01]

http://chromiumcodereview.appspot.com/10749020/diff/30002/test/mac/library_di...
File test/mac/library_dirs/subdir/test.gyp (right):

http://chromiumcodereview.appspot.com/10749020/diff/30002/test/mac/library_di...
test/mac/library_dirs/subdir/test.gyp:38: 'postbuild_name': 'Copy to secret
location, with secret name',
postbuilds are a mac-only feature as far as I know, but you can do the same with
a 'none' target with an action

[Brian Ellis, 2012-07-23 23:45:33]

http://chromiumcodereview.appspot.com/10749020/diff/30002/test/mac/library_di...
File test/mac/library_dirs/subdir/test.gyp (right):

http://chromiumcodereview.appspot.com/10749020/diff/30002/test/mac/library_di...
test/mac/library_dirs/subdir/test.gyp:38: 'postbuild_name': 'Copy to secret
location, with secret name',
On 2012/07/23 23:37:01, Nico wrote:
> postbuilds are a mac-only feature as far as I know, but you can do the same
with
> a 'none' target with an action

Okay...  trying that.  Make is unhappy if an action produces no outputs, so I
might have to come up with a fake output somewhere in here.  A bigger problem is
that, although it gets further than with a postbuild, it appears that the
library gets deleted at some point before it's needed:


Libtool build/Default/libmylib.a normal i386
    cd
/Users/phoenix/Source/experimental/third_party/gyp/trunk/out/testworkarea/testgyp.ndSRpM/library_dirs/subdir
    /Developer/usr/bin/libtool -static -arch_only i386
-L/Users/phoenix/Source/experimental/third_party/gyp/trunk/out/testworkarea/testgyp.ndSRpM/library_dirs/subdir/build/Default
-filelist
/Users/phoenix/Source/experimental/third_party/gyp/trunk/out/testworkarea/testgyp.ndSRpM/library_dirs/subdir/build/test.build/Default/mylib.build/Objects-normal/i386/mylib.LinkFileList
-o
/Users/phoenix/Source/experimental/third_party/gyp/trunk/out/testworkarea/testgyp.ndSRpM/library_dirs/subdir/build/Default/libmylib.a


=== BUILD AGGREGATE TARGET mylib_postbuild OF PROJECT test WITH THE DEFAULT
CONFIGURATION (Default) ===
Check dependencies
PhaseScriptExecution "Action \"Make a secret location\""
build/test.build/Default/mylib_postbuild.build/Script-1ACABE0502C603E564CBE0F4.sh
    cd
/Users/phoenix/Source/experimental/third_party/gyp/trunk/out/testworkarea/testgyp.ndSRpM/library_dirs/subdir
    /bin/sh -c
/Users/phoenix/Source/experimental/third_party/gyp/trunk/out/testworkarea/testgyp.ndSRpM/library_dirs/subdir/build/test.build/Default/mylib_postbuild.build/Script-1ACABE0502C603E564CBE0F4.sh

PhaseScriptExecution "Action \"Copy to secret location, with secret name\""
build/test.build/Default/mylib_postbuild.build/Script-919D4D0B7DE0F361FEF8CAE4.sh
    cd
/Users/phoenix/Source/experimental/third_party/gyp/trunk/out/testworkarea/testgyp.ndSRpM/library_dirs/subdir
    /bin/sh -c
/Users/phoenix/Source/experimental/third_party/gyp/trunk/out/testworkarea/testgyp.ndSRpM/library_dirs/subdir/build/test.build/Default/mylib_postbuild.build/Script-919D4D0B7DE0F361FEF8CAE4.sh

cp:
"/Users/phoenix/Source/experimental/third_party/gyp/trunk/out/testworkarea/testgyp.ndSRpM/library_dirs/subdir/build/Default/libmylib.a":
No such file or directory


It looks like the correct path
(testworkarea/testgyp.ndSRpM/library_dirs/subdir/build/Default/libmylib.a) is
getting created in the Libtool step, but when the postbuild action tries to copy
it, it's gone.

[Sam Clegg, 2012-08-06 17:31:13]

Any reason why the new test is in that 'mac' folder?

[Brian Ellis, 2012-08-06 20:37:23]

On 2012/08/06 17:31:13, Sam Clegg wrote:
> Any reason why the new test is in that 'mac' folder?

It's based on the libraries test, which is also only in 'mac'.  I've had no luck
genericizing it to work on other platforms, as my last message explains.  My
window for being able to work solidly on this has passed, so it may be a while
longer before I can find the time to fight with it more.  If you have the
resources to give me a hand (for example by genericizing the libraries test upon
which this one is based), that would be extremely helpful.

[moscati, 2013-04-24 16:34:02]

On 2012/08/06 20:37:23, Brian Ellis wrote:
> On 2012/08/06 17:31:13, Sam Clegg wrote:
> > Any reason why the new test is in that 'mac' folder?
> 
> It's based on the libraries test, which is also only in 'mac'.  I've had no
luck
> genericizing it to work on other platforms, as my last message explains.  My
> window for being able to work solidly on this has passed, so it may be a while
> longer before I can find the time to fight with it more.  If you have the
> resources to give me a hand (for example by genericizing the libraries test
upon
> which this one is based), that would be extremely helpful.

Hi,

What is the status of library_dirs?

Thanks,
Angelo

[bradn, 2013-04-29 21:32:44]

Is this still alive?

[Dimi Shahbaz, 2013-04-29 21:35:46]

On 2013/04/29 21:32:44, bradn wrote:
> Is this still alive?

I've updated Brian's CL, will upload a patch as soon as I figure out how.

[Dimi Shahbaz, 2013-04-30 00:12:59]

OK, that looks like it worked. There are some changes from Brian's last patch
set, so ptal. Tested on:

mac: ninja, xcode, make
linux: ninja, make
windows: ninja, msvs

Am I missing anything?

[Brian Ellis, 2013-04-30 00:27:27]

On 2013/04/30 00:12:59, Dimi Shahbaz wrote:
> OK, that looks like it worked. There are some changes from Brian's last patch
> set, so ptal. Tested on:
> 
> mac: ninja, xcode, make
> linux: ninja, make
> windows: ninja, msvs
> 
> Am I missing anything?

Try job failures appear to be patch problems.  Not sure how to resolve that...

[Brian Ellis, 2013-04-30 00:33:19]

On 2013/04/30 00:27:27, Brian Ellis wrote:
> On 2013/04/30 00:12:59, Dimi Shahbaz wrote:
> > OK, that looks like it worked. There are some changes from Brian's last
patch
> > set, so ptal. Tested on:
> > 
> > mac: ninja, xcode, make
> > linux: ninja, make
> > windows: ninja, msvs
> > 
> > Am I missing anything?
> 
> Try job failures appear to be patch problems.  Not sure how to resolve that...

Ah, never mind, I see why it failed.  Looks like we'll need to run this from the
command line as:

gcl try --no_search -b mac,win,linux,linux_clang,mac_xcodebuild

But I think Dimi will need to be the one to do that since he's got the current
patchset merged in.

[Dimi Shahbaz, 2013-04-30 21:40:37]

On 2013/04/30 00:33:19, Brian Ellis wrote:
> On 2013/04/30 00:27:27, Brian Ellis wrote:
> > On 2013/04/30 00:12:59, Dimi Shahbaz wrote:
> > > OK, that looks like it worked. There are some changes from Brian's last
> patch
> > > set, so ptal. Tested on:
> > > 
> > > mac: ninja, xcode, make
> > > linux: ninja, make
> > > windows: ninja, msvs
> > > 
> > > Am I missing anything?
> > 
> > Try job failures appear to be patch problems.  Not sure how to resolve
that...
> 
> Ah, never mind, I see why it failed.  Looks like we'll need to run this from
the
> command line as:
> 
> gcl try --no_search -b mac,win,linux,linux_clang,mac_xcodebuild
> 
> But I think Dimi will need to be the one to do that since he's got the current
> patchset merged in.

Can someone help me come up with the right incantation for gcl?

$ gcl.py try  a5h9 -R chromiumcodereview.appspot.com/10749020 -b
mac,win,linux,linux_clang,mac_xcodebuild  --email dimator@google.com 
Results will be emailed to: dimator@google.com
Authentication realm: <svn://svn.chromium.org:3690>
d3034d56-2935-4dfe-8f09-8ccb5d06043e
Password for 'dimator': 


I don't know why it's asking about svn://svn.chromium.org:3690, since this
doesn't involve that repo? Or am I confused?

[bradn, 2013-04-30 23:12:56]

That's just the default svn port.
You probably need to cache your password.
And you'll need the svn password from chromium-access.appspot.com

[Dimi Shahbaz, 2013-05-03 08:34:11]

I think I need some help debugging why the try servers report different results
from what I do running locally.

In my gyptest script, I have:

 15 test.run_gyp('test.gyp', chdir='subdir')
 16 
 17 # Must build each target independently, since they are not in each others'
 18 # 'dependencies' (test.ALL does NOT work here for some builders).
 19 test.build('test.gyp', 'mylib', chdir='subdir')
 20 test.build('test.gyp', 'libraries-search-path-test', chdir='subdir')


This introduces a build ordering that I cannot specify via a dependency in the
gyp file (see the test.gyp file itself for why this is; it's basically necessary
for testing the required behavior).

When I run this locally:

$ python gyptest.py  test/library_dirs/gyptest-library-dirs.py

I get:

TESTGYP_FORMAT=make
/usr/bin/python test/library_dirs/gyptest-library-dirs.py
PASSED
TESTGYP_FORMAT=ninja
/usr/bin/python test/library_dirs/gyptest-library-dirs.py
PASSED


Yet the try bot logs
(http://build.chromium.org/p/tryserver.nacl/builders/gyp-linux/builds/928/step...)
show:

/usr/bin/python test/library_dirs/gyptest-library-dirs.py
None returned 2
STDOUT =========================================================================
  CXX(target) out/Default/obj.target/libraries-search-path-test/hello.o
  LINK(target) out/Default/libraries-search-path-test

STDERR =========================================================================
out/Default/../secret_location/libmylib.a: could not read symbols: No such file
or directory
collect2: ld returned 1 exit status
make: *** [out/Default/libraries-search-path-test] Error 1

FAILED test at line 423 of
/mnt/data/b/build/slave/gyp/build/trunk/test/lib/TestCommon.py (_complete)
	from line 532 of /mnt/data/b/build/slave/gyp/build/trunk/test/lib/TestCommon.py
(run)
	from line 265 of /mnt/data/b/build/slave/gyp/build/trunk/test/lib/TestGyp.py
(run)
	from line 545 of /mnt/data/b/build/slave/gyp/build/trunk/test/lib/TestGyp.py
(build)
	from line 21 of test/library_dirs/gyptest-library-dirs.py


This means it's trying to build the exe without first building the shared
library, or it's not finding the shared library where it expects. This would
happen if for example the test.build() steps are run in parallel (breaking my
strict ordering), or if the steps' built files are cleaned immediately after it
is run.  I can't see how that would be by looking at buildbot_run.py.

[Dimi Shahbaz, 2013-05-10 22:49:54]

I think maybe I have this figured out, something about the temp paths. Locally
I'm running python2.7, I don't know what the try servers are running, but that
might be enough to break things.

I've refactored the test to pass in an absolute, secret directory to where to
put the library, passed in to the run_gyp step.

I'm still seeing a failure in my gyp-mac try, but it's not my test, my test
passes.  I'll run a try with this issue's ID, so we can all see the results
here.

[Dimi Shahbaz, 2013-05-16 23:00:02]

OK! All green! The last piece that was missing was that our msvs generator patch
only addressed one of the two project styles (MSBuild vs MSVSProject) and now
we're doing both... but I'm not entirely sure if both styles are being tested by
the bots?

If someone with expertise with the msvs generator could comment, I would sleep
easier...

Thanks!

[Dimi Shahbaz, 2013-05-21 00:35:37]

ping? how to proceed?

[Dimi Shahbaz, 2013-05-29 05:35:23]

daily ping mode: enabled.

ping.

[Mark Mentovai, 2013-05-29 13:23:07]

https://chromiumcodereview.appspot.com/10749020/diff/110001/pylib/gyp/generat...
File pylib/gyp/generator/xcode.py (right):

https://chromiumcodereview.appspot.com/10749020/diff/110001/pylib/gyp/generat...
pylib/gyp/generator/xcode.py:1197: return '"%s"' % s
?

This isn’t the right way to quote something. What if it has embedded quotation
marks? I’m sure we already have a quoting function that does what you want.

But why do you need to quote a library_dir at all? AppendBuildSetting should
take care of this for you.

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
File test/library_dirs/subdir/hello.cc (right):

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
test/library_dirs/subdir/hello.cc:1: // Copyright (c) 2012 Google Inc. All
rights reserved.
2013 in new files.

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
File test/library_dirs/subdir/mylib.h (right):

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
test/library_dirs/subdir/mylib.h:5: #ifndef _TEST_LIBRARY_DIRS_SUBDIR_MYLIB_H_
Your macros shouldn’t begin with an underscore, those are reserved for the
implementation.

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
test/library_dirs/subdir/mylib.h:12: #endif //
_TEST_LIBRARY_DIRS_SUBDIR_MYLIB_H_
Two spaces before the //.

These are both covered in the style guide.

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
File test/library_dirs/subdir/test.gyp (right):

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
test/library_dirs/subdir/test.gyp:28: # It is important to NOT list the mylib as
a dependency here, because
I don’t see how any build system would ensure that mylib is built before
libraries-search-path-test, then.

[Dimi Shahbaz, 2013-05-29 21:32:40]

https://chromiumcodereview.appspot.com/10749020/diff/110001/pylib/gyp/generat...
File pylib/gyp/generator/xcode.py (right):

https://chromiumcodereview.appspot.com/10749020/diff/110001/pylib/gyp/generat...
pylib/gyp/generator/xcode.py:1197: return '"%s"' % s
On 2013/05/29 13:23:07, Mark Mentovai wrote:
> ?
> 
> This isn’t the right way to quote something. What if it has embedded quotation
> marks? I’m sure we already have a quoting function that does what you want.
> 
> But why do you need to quote a library_dir at all? AppendBuildSetting should
> take care of this for you.

Done.

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
File test/library_dirs/subdir/hello.cc (right):

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
test/library_dirs/subdir/hello.cc:1: // Copyright (c) 2012 Google Inc. All
rights reserved.
On 2013/05/29 13:23:07, Mark Mentovai wrote:
> 2013 in new files.

Done.

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
File test/library_dirs/subdir/mylib.h (right):

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
test/library_dirs/subdir/mylib.h:5: #ifndef _TEST_LIBRARY_DIRS_SUBDIR_MYLIB_H_
On 2013/05/29 13:23:07, Mark Mentovai wrote:
> Your macros shouldn’t begin with an underscore, those are reserved for the
> implementation.

Done.

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
test/library_dirs/subdir/mylib.h:12: #endif //
_TEST_LIBRARY_DIRS_SUBDIR_MYLIB_H_
On 2013/05/29 13:23:07, Mark Mentovai wrote:
> Two spaces before the //.
> 
> These are both covered in the style guide.

Done.

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
File test/library_dirs/subdir/test.gyp (right):

https://chromiumcodereview.appspot.com/10749020/diff/110001/test/library_dirs...
test/library_dirs/subdir/test.gyp:28: # It is important to NOT list the mylib as
a dependency here, because
On 2013/05/29 13:23:07, Mark Mentovai wrote:
> I don’t see how any build system would ensure that mylib is built before
> libraries-search-path-test, then.

That's the point of the weird structuring of this gyp file: the library is
intended to be built first, it must be already extant in order to test what
we're trying to test (library_dirs).

If gyp pointed the executable to the library it depends on via 'dependencies',
then it would also figure out the right link paths to use. What we're testing
here is that the 'library_dirs' directive correctly points to the library's
parent directory so that '-lmylib' works.

I'll comment this further.

[Mark Mentovai, 2013-05-29 21:40:34]

https://chromiumcodereview.appspot.com/10749020/diff/126002/pylib/gyp/generat...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/10749020/diff/126002/pylib/gyp/generat...
pylib/gyp/generator/ninja.py:777: [QuoteShellArgument('-LIBPATH:' +
self.GypPathToNinja(l), self.flavor)
Watch the 80-column limit.

https://chromiumcodereview.appspot.com/10749020/diff/126002/test/library_dirs...
File test/library_dirs/subdir/test.gyp (right):

https://chromiumcodereview.appspot.com/10749020/diff/126002/test/library_dirs...
test/library_dirs/subdir/test.gyp:41: # 'libraries-search-path-test' (see
../gyptest-library-dirs.py).
The missing bit was this reference to the test driver script. Thanks.

[Dimi Shahbaz, 2013-05-29 21:45:16]

https://chromiumcodereview.appspot.com/10749020/diff/126002/pylib/gyp/generat...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/10749020/diff/126002/pylib/gyp/generat...
pylib/gyp/generator/ninja.py:777: [QuoteShellArgument('-LIBPATH:' +
self.GypPathToNinja(l), self.flavor)
On 2013/05/29 21:40:35, Mark Mentovai wrote:
> Watch the 80-column limit.

Done.

https://chromiumcodereview.appspot.com/10749020/diff/126002/test/library_dirs...
File test/library_dirs/subdir/test.gyp (right):

https://chromiumcodereview.appspot.com/10749020/diff/126002/test/library_dirs...
test/library_dirs/subdir/test.gyp:41: # 'libraries-search-path-test' (see
../gyptest-library-dirs.py).
On 2013/05/29 21:40:35, Mark Mentovai wrote:
> The missing bit was this reference to the test driver script. Thanks.

:)

[bradn, 2013-05-29 22:11:47]

lgtm on msvc

[Dimi Shahbaz, 2013-06-19 23:19:00]

On 2013/05/29 22:11:47, bradn wrote:
> lgtm on msvc

ping? do I need lgtms elsewhere?

[Mark Mentovai, 2013-06-20 16:07:55]

LGTM

[bradn, 2013-06-20 16:25:09]

lgtm

[Dimi Shahbaz, 2013-07-02 00:50:58]

On 2013/06/20 16:25:09, bradn wrote:
> lgtm

r1657. thanks all!

[Nico, 2013-07-02 01:20:14]

https://chromiumcodereview.appspot.com/10749020/diff/171001/pylib/gyp/generat...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/10749020/diff/171001/pylib/gyp/generat...
pylib/gyp/generator/ninja.py:1714: '$libdirs $libs'}),
Is it possible to just add these flags to the "libs" variable? This way ninja
needs to do fewer variable lookups. Have you benchmarked if this change has an
impact on empty build times for chrome?

[Dimi Shahbaz, 2013-07-02 03:29:43]

On 2013/07/02 01:20:14, Nico wrote:
>
https://chromiumcodereview.appspot.com/10749020/diff/171001/pylib/gyp/generat...
> File pylib/gyp/generator/ninja.py (right):
> 
>
https://chromiumcodereview.appspot.com/10749020/diff/171001/pylib/gyp/generat...
> pylib/gyp/generator/ninja.py:1714: '$libdirs $libs'}),
> Is it possible to just add these flags to the "libs" variable? This way ninja
> needs to do fewer variable lookups. Have you benchmarked if this change has an
> impact on empty build times for chrome?

I have not benchmarked it. Having not looked at the ninja code, is variable
lookup very costly? 

I can benchmark the chrome build before/after this change if necessary?