GN: Use lib.unstripped rather than lib.stripped. Add a toolchain.gni

GN: Use lib.unstripped rather than lib.stripped. Add a toolchain.gni

toolchain.gni introduces:
 root_shlib_dir, shlib_prefix, and shlib_extension

The original goal of this change was to put shlibs under lib/ for Linux / Android, since that's where GYP puts them. However, the lack of support for loadable_module (or more specifically - per target output directory) in GN makes this infeasible at the moment.

This change also mitigates a subtle bug where on Android the unstripped .so is used mistakenly instead of the lib.stripped/ version. It also fixes shlib's link_output being set to the unstripped .so rather than the stripped .so (on Android).

BUG=509771
Committed: https://crrev.com/c3cd5e6fe977af4a3c8b20ccb332896b2c888354
Cr-Commit-Position: refs/heads/master@{#342697}

[agrieve, 2015-07-13 19:09:33]

agrieve@chromium.org changed reviewers:
+ brettw@chromium.org

[agrieve, 2015-07-13 19:09:34]

From what I could infer, we'll likely need to configure windows to put .dlls at
the root (since it doesn't have a -rpath equivalent), but that is now easily
configurable.

[brettw, 2015-07-13 20:00:17]

I'm confused. I don't see any "lib" director on my Mac or Windows GYP builds (I
don't have Linux now to look).

This will completely break Windows. I assume the try passed because it's using
the old DLLs (it's not a clobber builder).

We should be matching GYP.

[agrieve, 2015-07-13 20:37:11]

On 2015/07/13 20:00:17, brettw wrote:
> I'm confused. I don't see any "lib" director on my Mac or Windows GYP builds
(I
> don't have Linux now to look).
> 
> This will completely break Windows. I assume the try passed because it's using
> the old DLLs (it's not a clobber builder).
> 
> We should be matching GYP.

Aha, I suspected that might be the case (but alas - no win / mac GN bots). I've
updated the patch to apply this only for linux & android.

Double checked just checked android & linux and they both go into lib/ for me.

Did some codesearching and found where this is set for GYP:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...

[brettw, 2015-07-14 00:59:41]

On 2015/07/13 20:37:11, agrieve wrote:
> Did some codesearching and found where this is set for GYP:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...

Will check this tomorrow when I get in.

Note that win8_chromium_ng is a GN bot and is run by default (name doesn't have
GN in it for historical reasons), and you can choose mac_chromium_gn_rel in
"Choose trybots" manually.

[agrieve, 2015-07-14 01:24:29]

On 2015/07/14 00:59:41, brettw wrote:
> On 2015/07/13 20:37:11, agrieve wrote:
> > Did some codesearching and found where this is set for GYP:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
> 
> Will check this tomorrow when I get in.
> 
> Note that win8_chromium_ng is a GN bot and is run by default (name doesn't
have
> GN in it for historical reasons), and you can choose mac_chromium_gn_rel in
> "Choose trybots" manually.

Thanks! That's really useful to know :)

No rush - nothing is blocked by this.

[agrieve, 2015-07-28 14:20:12]

On 2015/07/14 01:24:29, agrieve wrote:
> On 2015/07/14 00:59:41, brettw wrote:
> > On 2015/07/13 20:37:11, agrieve wrote:
> > > Did some codesearching and found where this is set for GYP:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
> > 
> > Will check this tomorrow when I get in.
> > 
> > Note that win8_chromium_ng is a GN bot and is run by default (name doesn't
> have
> > GN in it for historical reasons), and you can choose mac_chromium_gn_rel in
> > "Choose trybots" manually.
> 
> Thanks! That's really useful to know :)
> 
> No rush - nothing is blocked by this.

bump (still no rush, but don't want it to get lost either)

[brettw, 2015-07-29 22:29:51]

So I keep opening this, thinking, not deciding what I want, closing, and then
forgetting.

I don't really want the global flag for something used by just a couple of
files.

I'm thinking maybe a new file //build/toolchain/toolchain.gni for this kind of
general flag? It wouldn't surprise me if there are a few more related things we
end up having. Maybe a comment at the top about how global toolchain-setup stuff
goes in this file.

Maybe we should rename it to be "shlib" instead of "lib" to try to make clearer
that this is where shared libraries go? static libraries go in the target dirs.

Then I'm thinking it should have a new variable, root_shlib_dir, which is either
root_out_dir (Windows) or "$root_out_dir/lib*". Most people use that and all of
the other related variables are "root_foo". This also eliminates an extra "/./"
on Windows.

[agrieve, 2015-07-30 18:35:41]

On 2015/07/29 22:29:51, brettw wrote:
> So I keep opening this, thinking, not deciding what I want, closing, and then
> forgetting.
> 
> I don't really want the global flag for something used by just a couple of
> files.
> 
> I'm thinking maybe a new file //build/toolchain/toolchain.gni for this kind of
> general flag? It wouldn't surprise me if there are a few more related things
we
> end up having. Maybe a comment at the top about how global toolchain-setup
stuff
> goes in this file.

Tried this out. PTAL. Also added a couple other vars in there that were used by
mojo_application.gni.

> 
> Maybe we should rename it to be "shlib" instead of "lib" to try to make
clearer
> that this is where shared libraries go? static libraries go in the target
dirs.

done

> 
> Then I'm thinking it should have a new variable, root_shlib_dir, which is
either
> root_out_dir (Windows) or "$root_out_dir/lib*". Most people use that and all
of
> the other related variables are "root_foo". This also eliminates an extra
"/./"
> on Windows.

Had the same thought about root_shlib_dir, but didn't want to introduce more
vars than necessary in the global scope. With them moved to toolchain.gni, I've
added this in.

[brettw, 2015-07-30 23:17:47]

LGTM, this is way nicer

https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/gcc_to...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/gcc_to...
build/toolchain/gcc_toolchain.gni:139: sofile = "{{root_out_dir}}/lib/$soname" 
# Possibly including toolchain dir.
Should this be $shlib_subdir ?

https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/toolch...
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/toolch...
build/toolchain/toolchain.gni:26: # Extension for shared library files.
Can you append "..., including the dot."

[agrieve, 2015-07-31 02:09:25]

https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/gcc_to...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/gcc_to...
build/toolchain/gcc_toolchain.gni:139: sofile = "{{root_out_dir}}/lib/$soname" 
# Possibly including toolchain dir.
On 2015/07/30 23:17:46, brettw wrote:
> Should this be $shlib_subdir ?

No - so added a comment. Problem is that on Android we need it to be "lib"
rather than "lib.stripped"

https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/toolch...
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/toolch...
build/toolchain/toolchain.gni:26: # Extension for shared library files.
On 2015/07/30 23:17:47, brettw wrote:
> Can you append "..., including the dot."

Done.

[brettw, 2015-07-31 03:59:19]

https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/gcc_to...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/gcc_to...
build/toolchain/gcc_toolchain.gni:139: sofile = "{{root_out_dir}}/lib/$soname" 
# Possibly including toolchain dir.
Ah, thanks

[agrieve, 2015-07-31 18:28:33]

On 2015/07/31 03:59:19, brettw wrote:
>
https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/gcc_to...
> File build/toolchain/gcc_toolchain.gni (right):
> 
>
https://codereview.chromium.org/1236503002/diff/220001/build/toolchain/gcc_to...
> build/toolchain/gcc_toolchain.gni:139: sofile = "{{root_out_dir}}/lib/$soname"

> # Possibly including toolchain dir.
> Ah, thanks

Hit a snag.

Not sure why this wasn't caught in #7s tryjobs, but…

GYP has both loadable_module and shared_module, and it seems loadable_module
outputs to the root, and shared_module outputs to lib/. GN doens't have this
distinction yet, so the changed path doesn't sit well with some tests looking
for loadable_modules. Some references here:
https://code.google.com/p/chromium/issues/detail?id=74216
https://codereview.chromium.org/2975016

This causes issues in GYP, that are solved by copying the .so into lib/
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mesa/m...

I've updated this CL (look at third_party/mesa/BUILD.gn) to do the reverse (copy
the file lib->root) to fix the failing tests. Probably though, this should be
fixed by adding support for loadable_module later on.

anyways, would love another lgtm if you're fine with this for now.

[brettw, 2015-08-07 19:45:46]

I never understood why we needed a loadable module type target. I think whether
you want your target should be linked to a shared library, or dynamically load
it at runtime should be a property of the dependency, rather than letting each
library control how other targets link to it. GN has a way to express both of
these (deps/data_deps) so I was not planning on adding new target types for
this.

I'm not a fan of the lib subdirectory so if it's causing problems I'm inclined
to not do it, unless there's a clear reason. I don't know why it was originally
done this way in GYP though.

[agrieve, 2015-08-10 18:59:40]

On 2015/08/07 19:45:46, brettw wrote:
> I never understood why we needed a loadable module type target. I think
whether
> you want your target should be linked to a shared library, or dynamically load
> it at runtime should be a property of the dependency, rather than letting each
> library control how other targets link to it. GN has a way to express both of
> these (deps/data_deps) so I was not planning on adding new target types for
> this.
> 
> I'm not a fan of the lib subdirectory so if it's causing problems I'm inclined
> to not do it, unless there's a clear reason. I don't know why it was
originally
> done this way in GYP though.

Alright, I think I'm going to give up on putting things in lib/ for now. I've
re-wrote the commit description here and it is still addressing the "sometimes
android uses the unstripped" problem and introducing a toolchain.gni. The
loadable_module() thing will have to be addressed on its own, but for now I
think we should continue seeing how far we can get without it.

[brettw, 2015-08-10 20:06:52]

lgtm with comment change, this is a nice cleanup

https://codereview.chromium.org/1236503002/diff/320001/build/toolchain/gcc_to...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1236503002/diff/320001/build/toolchain/gcc_to...
build/toolchain/gcc_toolchain.gni:38: #  - is_clang
Can you document the new "strip" argument and its behavior here? Should include
what it does and what the default is (seems to be "if it's defined, use the
string to ..., otherwise ...").

[agrieve, 2015-08-10 20:33:47]

https://codereview.chromium.org/1236503002/diff/320001/build/toolchain/gcc_to...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1236503002/diff/320001/build/toolchain/gcc_to...
build/toolchain/gcc_toolchain.gni:38: #  - is_clang
On 2015/08/10 20:06:51, brettw wrote:
> Can you document the new "strip" argument and its behavior here? Should
include
> what it does and what the default is (seems to be "if it's defined, use the
> string to ..., otherwise ...").

Done.

[agrieve, 2015-08-10 20:34:42]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-08-10 20:34:42]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1236503002/#ps340001
(title: "add comment")

[commit-bot: I haz the power, 2015-08-10 20:35:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236503002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1236503002/340001

[commit-bot: I haz the power, 2015-08-10 21:18:04]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-08-10 21:18:47]

Patchset 18 (id:??) landed as
https://crrev.com/c3cd5e6fe977af4a3c8b20ccb332896b2c888354
Cr-Commit-Position: refs/heads/master@{#342697}

[Bons, 2015-11-15 16:25:39]

On 2015/08/10 21:18:47, commit-bot: I haz the power wrote:
> Patchset 18 (id:??) landed as
> https://crrev.com/c3cd5e6fe977af4a3c8b20ccb332896b2c888354
> Cr-Commit-Position: refs/heads/master@{#342697}

Loadable module support has been added. Is there anything else you need to
address the TODO here:
https://code.google.com/p/chromium/codesearch#chromium/src/build/toolchain/to...