Add forward_variables_from() and target() to GN

Add forward_variables_from() and target() to GN

forward_variables_from is syntactic sugar for:
  if (defined(invoker.foo)) {
    foo = invoker.foo
  }
For a list of variables or for all variables in a scope. I was originally
resisting adding new primitives for this kind of thing, but in some
places (especially the Android build) this is so common that forcing the
more verbose expression seems to be hurting readability.

It will also allow us to make the component and test templates trivial and
more easily up-to-date with new variables.

The target() function allows a target to be defined with a user-defined
type:
  target("source_set", "foo")
is equivalent to:
  source_set("foo")
This makes certain constructs easier to express where previously people were
forced to do excessive code duplication.

Renames mark_used to mark_dest_used in the scope options because I found the
old name confusing when writing this patch.

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

[brettw, 2015-08-03 00:34:29]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-03 00:34:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1263053003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1263053003/1

[commit-bot: I haz the power, 2015-08-03 01:02:11]

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

[commit-bot: I haz the power, 2015-08-03 01:02:12]

Dry run: This issue passed the CQ dry run.

[brettw, 2015-08-03 04:50:12]

brettw@chromium.org changed reviewers:
+ dpranke@google.com

[brettw, 2015-08-03 04:50:17]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-03 04:50:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1263053003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1263053003/20001

[commit-bot: I haz the power, 2015-08-03 07:00:39]

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

[commit-bot: I haz the power, 2015-08-03 07:00:40]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2015-08-03 15:11:41]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2015-08-03 15:11:42]

initial comments, just on the features themselves ...

is it possible to refer to any scope other than 'invoker'? In what case would
you do that? Does it make sense to just call it
`forward_variables_from_invoker`?

As far as `target` goes, it makes me a bit sad that there's now two ways to
declare targets, but it's clear that this is a useful construct when defining
templates or working around things that don't yet build properly on some
platforms.

I considered the idea of changing everything to always be target(type, name) and
getting rid of type(name), but it seems like that would actually hurt
readability in the common case (in addition to being a lot of cleanup work). Is
that your thinking also?

(Will start reviewing the code itself shortly).

[brettw, 2015-08-03 16:22:23]

On 2015/08/03 15:11:42, Dirk Pranke wrote:
> initial comments, just on the features themselves ...
> 
> is it possible to refer to any scope other than 'invoker'? In what case would
> you do that? Does it make sense to just call it
> `forward_variables_from_invoker`?

The other place that you can end up with scopes is in read_file and exec_script,
which can return scopes. It's conceivable you would want to use this function
for that case, and over time we've expanded the capabilities of scope variables
so I want to preserve flexibility in the future. I named it like this so it read
like "forward variables from invoker" while still being general.


> I considered the idea of changing everything to always be target(type, name)
and
> getting rid of type(name), but it seems like that would actually hurt
> readability in the common case (in addition to being a lot of cleanup work).
Is
> that your thinking also?

Yeah.

[Dirk Pranke, 2015-08-03 19:07:20]

mostly lgtm with a few comments ...

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/function_forwa...
File tools/gn/function_forward_variables_from.cc (right):

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from.cc:43: base::StringPiece storage_key =
source->GetStorageKey(cur.string_value());
Am I understanding this correctly that the main value to GetStorageKey() is
figuring out that whether the value is programmatic or not, and apart from that
you're just saving yourself a copy? Or is it actually really important to avoid
the copy? 

(it seems like the memory savings is likely small).

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from.cc:45: continue;  // Programatic value,
don't copy.
typo: programmatic

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from.cc:78: "  directly on the referenced
scope. Otherwise it will copy all kinds of\n"
suggest putting the last sentence in parentheses and changing "it will copy" to
"it would copy".

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/function_forwa...
File tools/gn/function_forward_variables_from_unittest.cc (right):

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from_unittest.cc:57: }
Maybe add a test where one of the values isn't defined, and a test or two for
the typechecking errors (single non-star string, non string-or-list)?

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/scope.h
File tools/gn/scope.h (right):

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/scope.h#newcod...
tools/gn/scope.h:159: // will have the same contents as "indent" passed in, but
may point to a
typo: ident, not indent

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/scope.h#newcod...
tools/gn/scope.h:163: // string for programatic and nonexistant values.
typo: programmatic

[Dirk Pranke, 2015-08-03 19:50:29]

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/function_forwa...
File tools/gn/function_forward_variables_from.cc (right):

https://codereview.chromium.org/1263053003/diff/20001/tools/gn/function_forwa...
tools/gn/function_forward_variables_from.cc:45: continue;  // Programatic value,
don't copy.
This should probably be an error rather than just skipped (as per our
discussion).

[brettw, 2015-08-03 21:09:07]

The new patch throws an error on programmatic values and adds tests for the
error cases. I had to tweak the test set up a bit since the programmatic values
weren't hooked up to it before.

[Dirk Pranke, 2015-08-03 21:12:24]

lgtm w/ typos fixed.

https://codereview.chromium.org/1263053003/diff/40001/tools/gn/scope.h
File tools/gn/scope.h (right):

https://codereview.chromium.org/1263053003/diff/40001/tools/gn/scope.h#newcod...
tools/gn/scope.h:163: // string for programatic and nonexistant values.
still have the "programmatic" typo

https://codereview.chromium.org/1263053003/diff/40001/tools/gn/test_with_scope.h
File tools/gn/test_with_scope.h (right):

https://codereview.chromium.org/1263053003/diff/40001/tools/gn/test_with_scop...
tools/gn/test_with_scope.h:64: ScopePerFileProvider scope_progammatic_provider_;
typo: "programmatic" (missing an 'r').

[brettw, 2015-08-03 21:14:57]

The CQ bit was checked by brettw@chromium.org

[brettw, 2015-08-03 21:14:58]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1263053003/#ps60001
(title: "More spelling fixes")

[commit-bot: I haz the power, 2015-08-03 21:15:38]

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

[commit-bot: I haz the power, 2015-08-03 21:54:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-03 21:55:11]

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