Add public deps to GN

Add public deps to GN.

Renamed datadeps to data_deps for consistency.

Renamed direct_dependent_configs to public_configs for consistency with the public deps. This is also easier to understand and hopefully will encourage people to do this instead of use all_dependent_configs.

Now that there are so many types of deps, added a DepsIterator that allows easy iterating over all of them (or only the linked ones). This simplified some code.

This simplified the header checker significantly since it had complicated logic to find direct_dependent_configs and prefer paths with those forwarded.

Removed a bunch of weird group special-casing. Groups no longer have their deps copied into the target, but are dependents like everything else. For now, unless you explicitly specify public_deps, all group deps will default to public. I'd like to change this in a future pass.

Added a bool return value to the target generator functions that fill values. Since I originally did that, I've started both returning a bool and setting an Err, which makes checking for the error and early-returning easier to remember. I did this because I found yet another case of forgetting to check err, which gives very strange results.

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

[brettw, 2014-09-16 19:52:33]

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

[jamesr, 2014-09-16 20:09:25]

https://codereview.chromium.org/561273003/diff/10031/tools/gn/action_target_g...
File tools/gn/action_target_generator.cc (right):

https://codereview.chromium.org/561273003/diff/10031/tools/gn/action_target_g...
tools/gn/action_target_generator.cc:70: return true;
wait, what does 'return true' mean? here we are returning true and populating an
error

https://codereview.chromium.org/561273003/diff/10031/tools/gn/action_target_g...
tools/gn/action_target_generator.cc:79: return false;
but here we are returning *false* and populating an error

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

https://codereview.chromium.org/561273003/diff/10031/tools/gn/target.cc#newco...
tools/gn/target.cc:353: DepsIterator::DepsIterator(const Target* t) :
current_index_(0) {
odd indentation

https://codereview.chromium.org/561273003/diff/10031/tools/gn/target.h
File tools/gn/target.h (right):

https://codereview.chromium.org/561273003/diff/10031/tools/gn/target.h#newcod...
tools/gn/target.h:309: class DepsIterator {
imho this should get its own .h/.cc

https://codereview.chromium.org/561273003/diff/10031/tools/gn/target.h#newcod...
tools/gn/target.h:326: // Advance to the next position. This assumes there are
more vectors.
'more vectors' -> 'more targets' ?

https://codereview.chromium.org/561273003/diff/10031/tools/gn/target.h#newcod...
tools/gn/target.h:335: DCHECK(current_index_ < vect_stack_[0]->size());
DCHECK_LT will print the lhs and rhs when the assertion fails which is useful
for debugging

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

https://codereview.chromium.org/561273003/diff/50001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:800: "  Targets directly referencing this one will have
the configs listed in\n"
'directly referencing this' -> 'directly depending on', perhaps?

https://codereview.chromium.org/561273003/diff/50001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:802: "  current target.\n"
we have some cases where we want things to apply to dependents and not the
target itself. i guess this could be accomplished with an intermediate group

https://codereview.chromium.org/561273003/diff/50001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:827: "      to dependents (this is the same as using\n"
direct dependents

https://codereview.chromium.org/561273003/diff/50001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:851: "  shared_library(\"b\") {\n"
this example seems a little bit too terse

[brettw, 2014-09-16 21:01:33]

PTAL

https://codereview.chromium.org/561273003/diff/10031/tools/gn/action_target_g...
File tools/gn/action_target_generator.cc (right):

https://codereview.chromium.org/561273003/diff/10031/tools/gn/action_target_g...
tools/gn/action_target_generator.cc:70: return true;
Mistake

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

https://codereview.chromium.org/561273003/diff/50001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:802: "  current target.\n"
On 2014/09/16 20:09:25, jamesr wrote:
> we have some cases where we want things to apply to dependents and not the
> target itself. i guess this could be accomplished with an intermediate group

Yes, my original design had this separate (like GYP does) but in 99% of the
cases you want to apply to yourself and dependents which made things
unnecessarily verbose and I changed it. Calling it "public configs" I think
makes it even more natural that it work this way. You do need an extra group for
the case you mentioned, though. I think this is an acceptable tradeoff.

[jamesr, 2014-09-16 22:14:24]

dumping comments from ps4, will start looking at ps5...

https://codereview.chromium.org/561273003/diff/50001/tools/gn/action_target_g...
File tools/gn/action_target_generator.cc (right):

https://codereview.chromium.org/561273003/diff/50001/tools/gn/action_target_g...
tools/gn/action_target_generator.cc:70: return true;
i think this should be 'return false'

https://codereview.chromium.org/561273003/diff/50001/tools/gn/header_checker.cc
File tools/gn/header_checker.cc (right):

https://codereview.chromium.org/561273003/diff/50001/tools/gn/header_checker....
tools/gn/header_checker.cc:81: ret += "\nIt's usually best to depend directly on
the desination target.\n"
s/desination/destination/

https://codereview.chromium.org/561273003/diff/50001/tools/gn/header_checker....
tools/gn/header_checker.cc:82: "In some cases, the detination target is
considered a subcomponent\n"
s/detination/destination/

https://codereview.chromium.org/561273003/diff/50001/tools/gn/header_checker....
tools/gn/header_checker.cc:85: "to include headers. Dependency chain (there may
be others):\n";
\n before 'Dependency chain' ?

[jamesr, 2014-09-16 22:21:04]

lgtm

[brettw, 2014-09-16 23:26:17]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2014-09-16 23:28:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561273003/110001

[commit-bot: I haz the power, 2014-09-17 01:07:32]

Committed patchset #7 (id:110001) as 3b64f4132e41d08eddf2f8e9176c1fd196a2a26a

[commit-bot: I haz the power, 2014-09-17 01:08:06]

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