GN: Add symlink rules for dump_syms, symupload

GN: Add symlink rules for dump_syms, symupload

Creates a symlink.gni helper template to reduce copy / paste.

BUG=552040
Committed: https://crrev.com/9f815753bd4c99e2209965cf8ac724aa0803db45
Cr-Commit-Position: refs/heads/master@{#364672}

[agrieve, 2015-12-09 15:12:17]

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

[agrieve, 2015-12-09 15:12:18]

◈

[brettw, 2015-12-10 00:12:21]

👾

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gni
File build/config/symlink.gni (right):

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:1: # Copyright 2015 The Chromium Authors. All rights
reserved.
This file should go with the script, so build/symlink.gni.

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:9: template("symlink") {
I think it would be nice if this mirrored the "copy" syntax as much as possible.
So use "source" and "output" (since you only take one file, lose the plural).

This would also help my confusion about what the from and to of a link are
(since the operation creates the thing in the opposite direction of the link).

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:19: _to = invoker.to
I find the underscores weird to read and they don't do anything. The only time
to use underscores is for variables in the outer scope of a .gni file, since
they prevent exporting those symbols. Inside a template, it doesn't do anything.

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:27: # TODO(agrieve): Declaring the output as _from
triggers a
Fix: https://codereview.chromium.org/1506343003

We should really list the generated output here. If you don't want to wait for
that fix to roll in for this change to land, you can provide a different target
name as the output file which I believe will sidestep this issue.

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:54: template("host_symlink") {
I find this:
  host_sym_link("foo") {
  }
really weird. It looks like an empty group that does nothing.

My first preference would be to just not do this template. But if we're going to
be doing this a nontrivial number of times, it will definitely get tedious to do
the get_label_info dance below.

In that case, I'd propose usage look more like:
  symlink_binary("foo") {
    binary_label = "//foo/bar($host_toolchain)"
    output = "$root_build_dir/foo"
  }
which makes it really clear what's going on, is more general, and is not
annoyingly repetitive or easy to mess up. The annoying get_label_info part is
still encapsulated.

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:68: data_deps = [
I don't think you want to use data deps here. Data deps means the file is
required at runtime, and will be added to isolate files for executing tests. I
don't think you want that to happen with these binaries. Just use regular
"deps", I don't think this will unreasonable restrict the parallelism of the
build.

[agrieve, 2015-12-10 11:07:38]

Much improved, thanks!

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gni
File build/config/symlink.gni (right):

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:1: # Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/12/10 00:12:21, brettw wrote:
> This file should go with the script, so build/symlink.gni.

Done.

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:9: template("symlink") {
On 2015/12/10 00:12:21, brettw wrote:
> I think it would be nice if this mirrored the "copy" syntax as much as
possible.
> So use "source" and "output" (since you only take one file, lose the plural).
> 
> This would also help my confusion about what the from and to of a link are
> (since the operation creates the thing in the opposite direction of the link).

Done.

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:19: _to = invoker.to
On 2015/12/10 00:12:21, brettw wrote:
> I find the underscores weird to read and they don't do anything. The only time
> to use underscores is for variables in the outer scope of a .gni file, since
> they prevent exporting those symbols. Inside a template, it doesn't do
anything.

This pattern is actually fairly common, at least within Android-related files. 

I think they are a good idea because they make it clear that the variables are
not used as arguments to templates. So now if you see one without an _, you know
that it's data being passed to a template.

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:27: # TODO(agrieve): Declaring the output as _from
triggers a
On 2015/12/10 00:12:21, brettw wrote:
> Fix: https://codereview.chromium.org/1506343003
> 
> We should really list the generated output here. If you don't want to wait for
> that fix to roll in for this change to land, you can provide a different
target
> name as the output file which I believe will sidestep this issue.

Great! I cherry-picked this change into a test branch in order to unblock
testing of the building-official-with-GN. So, I'm happy to wait for the fix to
roll. Changed to list the actual output.

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:54: template("host_symlink") {
On 2015/12/10 00:12:21, brettw wrote:
> I find this:
>   host_sym_link("foo") {
>   }
> really weird. It looks like an empty group that does nothing.
> 
> My first preference would be to just not do this template. But if we're going
to
> be doing this a nontrivial number of times, it will definitely get tedious to
do
> the get_label_info dance below.
> 
> In that case, I'd propose usage look more like:
>   symlink_binary("foo") {
>     binary_label = "//foo/bar($host_toolchain)"
>     output = "$root_build_dir/foo"
>   }
> which makes it really clear what's going on, is more general, and is not
> annoyingly repetitive or easy to mess up. The annoying get_label_info part is
> still encapsulated.

Made binary_label mandatory, but left output optional (feels similar to to
_binary, where output_name is optional).

https://codereview.chromium.org/1516533002/diff/20001/build/config/symlink.gn...
build/config/symlink.gni:68: data_deps = [
On 2015/12/10 00:12:21, brettw wrote:
> I don't think you want to use data deps here. Data deps means the file is
> required at runtime, and will be added to isolate files for executing tests. I
> don't think you want that to happen with these binaries. Just use regular
> "deps", I don't think this will unreasonable restrict the parallelism of the
> build.

Done.

[brettw, 2015-12-10 18:20:04]

lgtm

https://codereview.chromium.org/1516533002/diff/40001/build/symlink.gni
File build/symlink.gni (right):

https://codereview.chromium.org/1516533002/diff/40001/build/symlink.gni#newco...
build/symlink.gni:33: # relative location.
I think it would be worth adding here how you will want to put such targets
inside a condition like:
  if (current_toolchain == default_toolchain) {
otherwise there will be multiple rules (one for each toolchain the file is
executed in) that generates the symlink.

https://codereview.chromium.org/1516533002/diff/40001/build/symlink.gni#newco...
build/symlink.gni:38: template("host_binary_symlink") {
Since callers how have to specify the toolchain in the binary label, this should
probably be called "binary_symlink" instead.

[brettw, 2015-12-10 22:53:21]

The GN roll should be in.

[agrieve, 2015-12-11 10:37:02]

https://codereview.chromium.org/1516533002/diff/40001/build/symlink.gni
File build/symlink.gni (right):

https://codereview.chromium.org/1516533002/diff/40001/build/symlink.gni#newco...
build/symlink.gni:33: # relative location.
On 2015/12/10 18:20:03, brettw wrote:
> I think it would be worth adding here how you will want to put such targets
> inside a condition like:
>   if (current_toolchain == default_toolchain) {
> otherwise there will be multiple rules (one for each toolchain the file is
> executed in) that generates the symlink.

Done.

https://codereview.chromium.org/1516533002/diff/40001/build/symlink.gni#newco...
build/symlink.gni:38: template("host_binary_symlink") {
On 2015/12/10 18:20:03, brettw wrote:
> Since callers how have to specify the toolchain in the binary label, this
should
> probably be called "binary_symlink" instead.

Done.

[agrieve, 2015-12-11 10:38:02]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-12-11 10:38:02]

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/1516533002/#ps60001
(title: "comments 2")

[commit-bot: I haz the power, 2015-12-11 10:38:19]

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

[commit-bot: I haz the power, 2015-12-11 12:05:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-11 12:05:43]

Description was changed from

==========
GN: Add symlink rules for dump_syms, symupload

Creates a symlink.gni helper template to reduce copy / paste.

BUG=552040
==========

to

==========
GN: Add symlink rules for dump_syms, symupload

Creates a symlink.gni helper template to reduce copy / paste.

BUG=552040

Committed: https://crrev.com/9f815753bd4c99e2209965cf8ac724aa0803db45
Cr-Commit-Position: refs/heads/master@{#364672}
==========

[commit-bot: I haz the power, 2015-12-11 12:05:44]

Patchset 4 (id:??) landed as
https://crrev.com/9f815753bd4c99e2209965cf8ac724aa0803db45
Cr-Commit-Position: refs/heads/master@{#364672}