Special-case paths that appear in libs by not prefixing them with -l.

Allow GN libs to be specified by path instead of by name

Android needs to link against libgcc.a, and it makes much more sense to
do so via direct path rather than using name + search-path to do so.

This change allows paths (any string that contains a '/') to appear in
a libs list. Paths are not prefixed by -l when passed to the linker,
and follow the normal path semantics.

BUG=570406
R=brettw@chromium.org, tfarina@chromium.org

Committed: https://crrev.com/f865ab8f007295f7efc655a3685b7c4cab140635
Cr-Commit-Position: refs/heads/master@{#366530}

[agrieve, 2015-12-17 18:44:29]

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

[agrieve, 2015-12-17 18:44:30]

It wasn't obvious to me which unit tests could be changed to cover the new
behaviour here. Please advise if you have ideas here.

[tfarina, 2015-12-17 18:46:35]

Description was changed from

==========
Special-case paths that appear in libs by not prefixing them with -l.

BUG=570406
==========

to

==========
Special-case paths that appear in libs by not prefixing them with -l.

BUG=570406
R=brettw@chromium.org
==========

[tfarina, 2015-12-17 18:48:31]

Could you expand the CL description? Which are the paths you are whitelisting?

Could you also explaing in the description why they shouldn't be prefixed with
-l?

So it will be more clear what you are trying to fix here.

[tfarina, 2015-12-17 18:52:45]

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.cc
File tools/gn/lib_file.cc (right):

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.cc#newcode1
tools/gn/lib_file.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights
reserved.
no (c)

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h
File tools/gn/lib_file.h (right):

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h#newcode1
tools/gn/lib_file.h:1: // Copyright (c) 2015 The Chromium Authors. All rights
reserved.
no (c)

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h#newcode14
tools/gn/lib_file.h:14: // Represents a file within the source tree. Always
begins in a slash, never
This is too vague. A file could be anything. But the class is named Lib. Which
lib file is that? I think you are referring to libfoo.a? Does it also represent
libfoo.so?

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h#newcode22
tools/gn/lib_file.h:22: void swap(LibFile& other);
no non-const references.

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h#newcode23
tools/gn/lib_file.h:23: bool is_source_file() const { return name_.empty(); }
I would add a blank line after this.

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h#newcode36
tools/gn/lib_file.h:36: }
blank line

[agrieve, 2015-12-17 19:25:24]

Description was changed from

==========
Special-case paths that appear in libs by not prefixing them with -l.

BUG=570406
R=brettw@chromium.org
==========

to

==========
Allow GN libs to be specified by path instead of by name

Android needs to link against libgcc.a, and it makes much more sense to do so
via direct path rather than using name + search-path to do so.

This change allows paths (any string that contains a '/') to appear in a libs
list. Paths are not prefixed by -l when passed to the linker, and follow the
normal path semantics.

BUG=570406
R=brettw@chromium.org
==========

[agrieve, 2015-12-17 19:25:24]

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

[agrieve, 2015-12-17 19:26:08]

Description was changed from

==========
Allow GN libs to be specified by path instead of by name

Android needs to link against libgcc.a, and it makes much more sense to do so
via direct path rather than using name + search-path to do so.

This change allows paths (any string that contains a '/') to appear in a libs
list. Paths are not prefixed by -l when passed to the linker, and follow the
normal path semantics.

BUG=570406
R=brettw@chromium.org
==========

to

==========
Allow GN libs to be specified by path instead of by name

Android needs to link against libgcc.a, and it makes much more sense to
do so via direct path rather than using name + search-path to do so.

This change allows paths (any string that contains a '/') to appear in
a libs list. Paths are not prefixed by -l when passed to the linker,
and follow the normal path semantics.

BUG=570406
R=brettw@chromium.org, tfarina@chromium.org
==========

[agrieve, 2015-12-17 19:33:34]

Thanks! Updated cl description (agree - it was awful :P)

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.cc
File tools/gn/lib_file.cc (right):

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.cc#newcode1
tools/gn/lib_file.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights
reserved.
On 2015/12/17 18:52:44, tfarina wrote:
> no (c)

Done.

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h
File tools/gn/lib_file.h (right):

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h#newcode1
tools/gn/lib_file.h:1: // Copyright (c) 2015 The Chromium Authors. All rights
reserved.
On 2015/12/17 18:52:44, tfarina wrote:
> no (c)

Done.

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h#newcode14
tools/gn/lib_file.h:14: // Represents a file within the source tree. Always
begins in a slash, never
On 2015/12/17 18:52:44, tfarina wrote:
> This is too vague. A file could be anything. But the class is named Lib. Which
> lib file is that? I think you are referring to libfoo.a? Does it also
represent
> libfoo.so?

Done.

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h#newcode22
tools/gn/lib_file.h:22: void swap(LibFile& other);
On 2015/12/17 18:52:44, tfarina wrote:
> no non-const references.

Done.

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h#newcode23
tools/gn/lib_file.h:23: bool is_source_file() const { return name_.empty(); }
On 2015/12/17 18:52:44, tfarina wrote:
> I would add a blank line after this.

Done.

https://codereview.chromium.org/1530183005/diff/1/tools/gn/lib_file.h#newcode36
tools/gn/lib_file.h:36: }
On 2015/12/17 18:52:44, tfarina wrote:
> blank line

Done.

[brettw, 2015-12-18 00:04:55]

The main suggestion here is for a binary target writer test.

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/command_desc.cc
File tools/gn/command_desc.cc (right):

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/command_desc.c...
tools/gn/command_desc.cc:179: for (size_t i = 0; i < libs.size(); i++) {
No {} for single-line conditionals.

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/config_values_...
File tools/gn/config_values_generator.cc (right):

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/config_values_...
tools/gn/config_values_generator.cc:89: std::vector<LibFile> result;
I think you can avoid this temporary and have ExtractListOfLibs write directly
into config_values_->libs().

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/ninja_binary_t...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:755: // Libraries specified by paths.
We should get a test for this new behavior.

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:758: if (libs[i].is_source_file()) {
No {} for single-line ones (file style).

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:872: std::string value =
lib_file.value();
Can this be a const ref? I think it would also be more clear to call this
lib_value or something.

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:556: if (all_libs_[i].is_source_file()) {
No {}

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:910: "  library name (but not library paths). Your BUILD
file should not\n"
How about replacing the existing docs with:

Values containing '/' will be treated as references to files in the build. They
will be rebased to be relative to the build directory and specified in the
\"libs\" for linker tools. This facility should be used for libraries that are
checked in to the build. For libraries that are generated by the build, use
normal GN deps to link them.

Values not containing '/' will be treated as system library names. These will be
passed unmodified to the linker and prefixed with the \"lib_prefix\" attribute
of the linker tool. Generally you would set the \"lib_dirs\" so the given
library is found. Your BUILD.gn file should not specify the switch prefix (like
\"-l\"): this will be encoded in the \"lib_prefix\" of the tool.

...framework exception paragraph...

[agrieve, 2015-12-18 15:39:08]

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/command_desc.cc
File tools/gn/command_desc.cc (right):

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/command_desc.c...
tools/gn/command_desc.cc:179: for (size_t i = 0; i < libs.size(); i++) {
On 2015/12/18 00:04:55, brettw wrote:
> No {} for single-line conditionals.

Done.

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/config_values_...
File tools/gn/config_values_generator.cc (right):

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/config_values_...
tools/gn/config_values_generator.cc:89: std::vector<LibFile> result;
On 2015/12/18 00:04:55, brettw wrote:
> I think you can avoid this temporary and have ExtractListOfLibs write directly
> into config_values_->libs().

Done.

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/ninja_binary_t...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:755: // Libraries specified by paths.
On 2015/12/18 00:04:55, brettw wrote:
> We should get a test for this new behavior.

Done.

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:758: if (libs[i].is_source_file()) {
On 2015/12/18 00:04:55, brettw wrote:
> No {} for single-line ones (file style).

Done.

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:872: std::string value =
lib_file.value();
On 2015/12/18 00:04:55, brettw wrote:
> Can this be a const ref? I think it would also be more clear to call this
> lib_value or something.

Done.

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:556: if (all_libs_[i].is_source_file()) {
On 2015/12/18 00:04:55, brettw wrote:
> No {}

Done.

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/1530183005/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:910: "  library name (but not library paths). Your BUILD
file should not\n"
On 2015/12/18 00:04:55, brettw wrote:
> How about replacing the existing docs with:
> 
> Values containing '/' will be treated as references to files in the build.
They
> will be rebased to be relative to the build directory and specified in the
> \"libs\" for linker tools. This facility should be used for libraries that are
> checked in to the build. For libraries that are generated by the build, use
> normal GN deps to link them.
> 
> Values not containing '/' will be treated as system library names. These will
be
> passed unmodified to the linker and prefixed with the \"lib_prefix\" attribute
> of the linker tool. Generally you would set the \"lib_dirs\" so the given
> library is found. Your BUILD.gn file should not specify the switch prefix
(like
> \"-l\"): this will be encoded in the \"lib_prefix\" of the tool.
> 
> ...framework exception paragraph...

Sounds great! Done.

[brettw, 2015-12-21 22:39:29]

lgtm

https://codereview.chromium.org/1530183005/diff/80001/tools/gn/config_values_...
File tools/gn/config_values_generator.cc (right):

https://codereview.chromium.org/1530183005/diff/80001/tools/gn/config_values_...
tools/gn/config_values_generator.cc:86: if (libs_value)
Since this is multi-line, always use {}

https://codereview.chromium.org/1530183005/diff/80001/tools/gn/ninja_binary_t...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1530183005/diff/80001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:793: implicit_deps.push_back(
This one's multi-line so we would always use a {} no matter what.

[agrieve, 2015-12-22 01:17:05]

https://codereview.chromium.org/1530183005/diff/80001/tools/gn/config_values_...
File tools/gn/config_values_generator.cc (right):

https://codereview.chromium.org/1530183005/diff/80001/tools/gn/config_values_...
tools/gn/config_values_generator.cc:86: if (libs_value)
On 2015/12/21 22:39:28, brettw wrote:
> Since this is multi-line, always use {}

Done.

https://codereview.chromium.org/1530183005/diff/80001/tools/gn/ninja_binary_t...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1530183005/diff/80001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:793: implicit_deps.push_back(
On 2015/12/21 22:39:28, brettw wrote:
> This one's multi-line so we would always use a {} no matter what.

Done.

[agrieve, 2015-12-22 01:17:14]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-12-22 01:17:14]

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/1530183005/#ps100001
(title: "fix multi-line if braces")

[commit-bot: I haz the power, 2015-12-22 01:18:08]

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

[commit-bot: I haz the power, 2015-12-22 01:26:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 01:26:23]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[agrieve, 2015-12-22 02:09:21]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-12-22 02:09:22]

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/1530183005/#ps120001
(title: "Add lib_file.cc to gn.gyp")

[commit-bot: I haz the power, 2015-12-22 02:10:14]

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

[commit-bot: I haz the power, 2015-12-22 02:17:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-22 02:17:56]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[agrieve, 2015-12-22 02:23:14]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-12-22 02:23:15]

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/1530183005/#ps140001
(title: "Fix typo in previous patch")

[commit-bot: I haz the power, 2015-12-22 02:23:49]

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

[commit-bot: I haz the power, 2015-12-22 03:04:09]

Description was changed from

==========
Allow GN libs to be specified by path instead of by name

Android needs to link against libgcc.a, and it makes much more sense to
do so via direct path rather than using name + search-path to do so.

This change allows paths (any string that contains a '/') to appear in
a libs list. Paths are not prefixed by -l when passed to the linker,
and follow the normal path semantics.

BUG=570406
R=brettw@chromium.org, tfarina@chromium.org
==========

to

==========
Allow GN libs to be specified by path instead of by name

Android needs to link against libgcc.a, and it makes much more sense to
do so via direct path rather than using name + search-path to do so.

This change allows paths (any string that contains a '/') to appear in
a libs list. Paths are not prefixed by -l when passed to the linker,
and follow the normal path semantics.

BUG=570406
R=brettw@chromium.org, tfarina@chromium.org
==========

[commit-bot: I haz the power, 2015-12-22 03:04:09]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-22 03:04:52]

Description was changed from

==========
Allow GN libs to be specified by path instead of by name

Android needs to link against libgcc.a, and it makes much more sense to
do so via direct path rather than using name + search-path to do so.

This change allows paths (any string that contains a '/') to appear in
a libs list. Paths are not prefixed by -l when passed to the linker,
and follow the normal path semantics.

BUG=570406
R=brettw@chromium.org, tfarina@chromium.org
==========

to

==========
Allow GN libs to be specified by path instead of by name

Android needs to link against libgcc.a, and it makes much more sense to
do so via direct path rather than using name + search-path to do so.

This change allows paths (any string that contains a '/') to appear in
a libs list. Paths are not prefixed by -l when passed to the linker,
and follow the normal path semantics.

BUG=570406
R=brettw@chromium.org, tfarina@chromium.org

Committed: https://crrev.com/f865ab8f007295f7efc655a3685b7c4cab140635
Cr-Commit-Position: refs/heads/master@{#366530}
==========

[commit-bot: I haz the power, 2015-12-22 03:04:53]

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