gn desc: printing public_deps without --all and --tree

gn desc: printing public_deps without --all and --tree

'gn desc' merges public_deps with deps even if it doesn't merge configs
and public_configs. This CL removes this asymmetry only if the command
is invoked without --tree and --all. In that case it becomes too complex
to split the two different type of dependencies.

BUG=None

[mbonadei1, 2017-05-15 07:09:02]

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

[kjellander_chromium, 2017-05-18 06:45:34]

Description was changed from

==========
gn desc: printing public_deps without --all and --tree

'gn desc' merges public_deps with deps even if it doesn't merge configs
and public_configs. This CL removes this asymmetry only if the command
is invoked without --tree and --all. In that case it becomes too complex
to split the two different type of dependencies.

BUG=None
==========

to

==========
gn desc: printing public_deps without --all and --tree

'gn desc' merges public_deps with deps even if it doesn't merge configs
and public_configs. This CL removes this asymmetry only if the command
is invoked without --tree and --all. In that case it becomes too complex
to split the two different type of dependencies.

BUG=None
==========

[brettw, 2017-05-23 17:23:05]

Sorry I took so long, just rediscovered this.

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

https://codereview.chromium.org/2880093002/diff/1/tools/gn/command_desc.cc#ne...
tools/gn/command_desc.cc:377: If the command is invoked with --tree or --all,
deps will include all public,
Documentation formatting: Can you wrap this together in one paragraph. Separate
paragraphs should be separated by blank lines (as opposed to newlines but no
blank like you did).

https://codereview.chromium.org/2880093002/diff/1/tools/gn/command_desc.cc#ne...
tools/gn/command_desc.cc:380: private and data deps. In this case public deps
will be printed as part of
This says "deps" will include data deps, is that right? If this is right, can we
change it? Data deps are very different and aren't linked. It doesn't make sense
to include this but not public_deps.

Can we separate out data_deps as well like you did for public and private?

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

https://codereview.chromium.org/2880093002/diff/1/tools/gn/target.cc#newcode421
tools/gn/target.cc:421: if (type == DEPS_LINKED) {
With the additional enum types, I think this function would be more readable as
a switch statement. Can you change it?

[mbonadei1, 2017-05-26 09:29:44]

Thanks for the review. No worries, it isn't time critical.

I added a comment to understand if we are on the same page.
If you confirm I will implement the behaviour and I will
update the documentation.

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

https://codereview.chromium.org/2880093002/diff/1/tools/gn/command_desc.cc#ne...
tools/gn/command_desc.cc:377: If the command is invoked with --tree or --all,
deps will include all public,
On 2017/05/23 17:23:05, brettw wrote:
> Documentation formatting: Can you wrap this together in one paragraph.
Separate
> paragraphs should be separated by blank lines (as opposed to newlines but no
> blank like you did).

Done.

https://codereview.chromium.org/2880093002/diff/1/tools/gn/command_desc.cc#ne...
tools/gn/command_desc.cc:380: private and data deps. In this case public deps
will be printed as part of
On 2017/05/23 17:23:05, brettw wrote:
> This says "deps" will include data deps, is that right? If this is right, can
we
> change it? Data deps are very different and aren't linked. It doesn't make
sense
> to include this but not public_deps.
> 
> Can we separate out data_deps as well like you did for public and private?

I am not sure I am following, let me try to rephrase to be sure we
are on the same page.

If we are splitting private and public deps it does not make sense
to include data_deps in the private deps (this change will be done
in the function Target::GetDeps).

After this change we can also add another field to the target
description, this field will be called data_deps and it will always
include data deps. Is this right?

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

https://codereview.chromium.org/2880093002/diff/1/tools/gn/target.cc#newcode421
tools/gn/target.cc:421: if (type == DEPS_LINKED) {
On 2017/05/23 17:23:05, brettw wrote:
> With the additional enum types, I think this function would be more readable
as
> a switch statement. Can you change it?

Done.

[mbonadei1, 2017-06-07 08:04:59]

On 2017/05/26 09:29:44, mbonadei1 wrote:
> Thanks for the review. No worries, it isn't time critical.
> 
> I added a comment to understand if we are on the same page.
> If you confirm I will implement the behaviour and I will
> update the documentation.
> 
> https://codereview.chromium.org/2880093002/diff/1/tools/gn/command_desc.cc
> File tools/gn/command_desc.cc (right):
> 
>
https://codereview.chromium.org/2880093002/diff/1/tools/gn/command_desc.cc#ne...
> tools/gn/command_desc.cc:377: If the command is invoked with --tree or --all,
> deps will include all public,
> On 2017/05/23 17:23:05, brettw wrote:
> > Documentation formatting: Can you wrap this together in one paragraph.
> Separate
> > paragraphs should be separated by blank lines (as opposed to newlines but no
> > blank like you did).
> 
> Done.
> 
>
https://codereview.chromium.org/2880093002/diff/1/tools/gn/command_desc.cc#ne...
> tools/gn/command_desc.cc:380: private and data deps. In this case public deps
> will be printed as part of
> On 2017/05/23 17:23:05, brettw wrote:
> > This says "deps" will include data deps, is that right? If this is right,
can
> we
> > change it? Data deps are very different and aren't linked. It doesn't make
> sense
> > to include this but not public_deps.
> > 
> > Can we separate out data_deps as well like you did for public and private?
> 
> I am not sure I am following, let me try to rephrase to be sure we
> are on the same page.
> 
> If we are splitting private and public deps it does not make sense
> to include data_deps in the private deps (this change will be done
> in the function Target::GetDeps).
> 
> After this change we can also add another field to the target
> description, this field will be called data_deps and it will always
> include data deps. Is this right?
> 
> https://codereview.chromium.org/2880093002/diff/1/tools/gn/target.cc
> File tools/gn/target.cc (right):
> 
>
https://codereview.chromium.org/2880093002/diff/1/tools/gn/target.cc#newcode421
> tools/gn/target.cc:421: if (type == DEPS_LINKED) {
> On 2017/05/23 17:23:05, brettw wrote:
> > With the additional enum types, I think this function would be more readable
> as
> > a switch statement. Can you change it?
> 
> Done.

Friendly ping. :)

[brettw, 2017-06-07 19:59:15]

The weirdest thing about deps printing is not that it groups public and private
deps, but that it also includes data deps. Data deps aren't even linked like
normal, and their public configs don't apply. So including these is actively
misleading.

You split public_deps out of this, but if we're going to split this apart, it's
even more important to also split data_deps out.

If you feel like it (optional), it might be nice to add a "linked_deps" item
like we have for "runtime_deps" which will show both public and private deps in
the order they're linked. This is often what you want when debugging this stuff.
This is optional.

Sorry I didn't notice this in the previous pass. Please ping me in IM when you
upload a new version so I don't delay things longer.

https://codereview.chromium.org/2880093002/diff/20001/tools/gn/desc_builder.cc
File tools/gn/desc_builder.cc (right):

https://codereview.chromium.org/2880093002/diff/20001/tools/gn/desc_builder.c...
tools/gn/desc_builder.cc:525: std::sort(deps.begin(), deps.end());
I think we should delete this line (see below).

https://codereview.chromium.org/2880093002/diff/20001/tools/gn/desc_builder.c...
tools/gn/desc_builder.cc:540: std::sort(deps.begin(), deps.end());
I don't think we should be sorting these in either case, and I think the
existing code above is wrong. The documentation says they're printed in the
order applying which I think is what we want. The reason is that the order
affects the order that dependent configs are applied. People can always pipe
through sort if they need to.

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

https://codereview.chromium.org/2880093002/diff/20001/tools/gn/target.h#newco...
tools/gn/target.h:53: DEPS_PRIVATE,  // Iterates through private and data deps.
This shouldn't include data deps.

[mbonadei1, 2017-06-08 11:37:45]

Thanks for the detailed comment.

I've separated data_deps from private_deps and I've
introduced two new fields in the command output:

- data_deps: always present with only the content of
    data_deps
- linked_deps: always present with public_deps and 
    private_deps

If the command is invoked with --tree or --all deps will
be different from linked_deps and public_deps will not be
printed.

If the command is invoked without --tree and --all then
public_deps will be printed and linked_deps will be
identical to deps.

My questions are:

- does it make sense to always print public_deps? I don't
  remember why I decided to avoid to print it but now it
  sees to me that it is a possible source of confusion;
- is the fact that linked_deps and deps are identical if
  the command is invoked without --tree and --all correct?

Thanks!

https://codereview.chromium.org/2880093002/diff/20001/tools/gn/desc_builder.cc
File tools/gn/desc_builder.cc (right):

https://codereview.chromium.org/2880093002/diff/20001/tools/gn/desc_builder.c...
tools/gn/desc_builder.cc:525: std::sort(deps.begin(), deps.end());
On 2017/06/07 19:59:14, brettw wrote:
> I think we should delete this line (see below).

Done.

https://codereview.chromium.org/2880093002/diff/20001/tools/gn/desc_builder.c...
tools/gn/desc_builder.cc:540: std::sort(deps.begin(), deps.end());
On 2017/06/07 19:59:14, brettw wrote:
> I don't think we should be sorting these in either case, and I think the
> existing code above is wrong. The documentation says they're printed in the
> order applying which I think is what we want. The reason is that the order
> affects the order that dependent configs are applied. People can always pipe
> through sort if they need to.

Yes, it makes sense. I have removed sorting from both RenderDeps
and RenderPublicDeps.

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

https://codereview.chromium.org/2880093002/diff/20001/tools/gn/target.h#newco...
tools/gn/target.h:53: DEPS_PRIVATE,  // Iterates through private and data deps.
On 2017/06/07 19:59:15, brettw wrote:
> This shouldn't include data deps.

Done.

[brettw, 2017-06-08 17:57:22]

The separate ability to list deps, public_deps, data_deps, and linked_deps looks
good.

For the overall default summary output I think we should change the current
section that says "Direct dependencies (try also "--all", ...)". This should be:

linked_deps (Synthetic combination of deps and public_deps, try also "--all",
"--tree", "--all --tree", or explicitly request only "deps" or "public_deps")

and then it should include the new linked_deps dump you have. It shouldn't print
public_deps and deps separately.

data_deps should definitely be a separate section.

[brettw, 2017-06-08 18:02:52]

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

https://codereview.chromium.org/2880093002/diff/40001/tools/gn/command_desc.c...
tools/gn/command_desc.cc:380: - deps: if the command is invoked without --tree
and --all it just
The formatting I would use in other places to avoid ambiguity with hyphen being
for command line flags is a separate line for the heading, and then a 4-space
indent with complete sentences:

    deps
        If the command is invoked...
    public_deps
        If the...

[mbonadei1, 2017-06-09 11:16:36]

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

https://codereview.chromium.org/2880093002/diff/40001/tools/gn/command_desc.c...
tools/gn/command_desc.cc:380: - deps: if the command is invoked without --tree
and --all it just
On 2017/06/08 18:02:52, brettw wrote:
> The formatting I would use in other places to avoid ambiguity with hyphen
being
> for command line flags is a separate line for the heading, and then a 4-space
> indent with complete sentences:
> 
>     deps
>         If the command is invoked...
>     public_deps
>         If the...

Done.

[mbonadei1, 2017-06-09 11:36:44]

> The separate ability to list deps, public_deps, data_deps, and linked_deps
looks
> good.

Ok.

> For the overall default summary output I think we should change the current
> section that says "Direct dependencies (try also "--all", ...)". This should
be:
> 
> linked_deps (Synthetic combination of deps and public_deps, try also "--all",
> "--tree", "--all --tree", or explicitly request only "deps" or "public_deps")
> 
> and then it should include the new linked_deps dump you have. It shouldn't
print
> public_deps and deps separately.

I am not 100% sure. DepsHandler in command_desc.cc has a complex behaviour:

* if the command is invoked with --tree or --all, the second parameter of this
  function will be generated using the Target::DEPS_LINKED DepsIterationType;
* if the command is invoked without --tree and --all, the second parameter of
  this function will be generated using the Target::DEPS_PRIVATE
DepsIterationType.

The summary "Direct dependencies (try also "--all", ...)" is printed when --tree
and
--all are not passed by the user so in that case we are not using DEPS_LINKED
but
DEPS_PRIVATE.

So I am not sure that it is a good thing to print "linked deps" in that
situation.
Maybe we can explain that with --tree and --all the content of this section is
created using linked dependencies while without it only shows direct
dependencies
and in that case having public_deps is important because they will not show up
into
the output of this section.

I did't though about this before because I always use the json format but I
agree
with you that we have to re-think this a little bit. Let me know if I am missing
something and thanks for the deep review.

 
> data_deps should definitely be a separate section.

I will add this along with the refactoring requested in the previous point after
the
clarification.

[brettw, 2017-06-09 22:05:24]

On 2017/06/09 11:36:44, mbonadei1 wrote:
> > The separate ability to list deps, public_deps, data_deps, and linked_deps
> looks
> > good.
> 
> Ok.
> 
> > For the overall default summary output I think we should change the current
> > section that says "Direct dependencies (try also "--all", ...)". This should
> be:
> > 
> > linked_deps (Synthetic combination of deps and public_deps, try also
"--all",
> > "--tree", "--all --tree", or explicitly request only "deps" or
"public_deps")
> > 
> > and then it should include the new linked_deps dump you have. It shouldn't
> print
> > public_deps and deps separately.
> 
> I am not 100% sure. DepsHandler in command_desc.cc has a complex behaviour:
> 
> * if the command is invoked with --tree or --all, the second parameter of this
>   function will be generated using the Target::DEPS_LINKED DepsIterationType;
> * if the command is invoked without --tree and --all, the second parameter of
>   this function will be generated using the Target::DEPS_PRIVATE
> DepsIterationType.
> 
> The summary "Direct dependencies (try also "--all", ...)" is printed when
--tree
> and
> --all are not passed by the user so in that case we are not using DEPS_LINKED
> but
> DEPS_PRIVATE.
> 
> So I am not sure that it is a good thing to print "linked deps" in that
> situation.
> Maybe we can explain that with --tree and --all the content of this section is
> created using linked dependencies while without it only shows direct
> dependencies
> and in that case having public_deps is important because they will not show up
> into
> the output of this section.

"Direct" dependencies are ones listed on the target, and so isn't contradictory
with "linked" dependencies.

We should have a linked_deps output, and that should support --all and --tree. I
think this should appear in the "desc" default output rather than "deps" and
"public_deps". I don't think this is contradictory, and this is the same
behavior we have today except it wouldn't include data_deps.

There's a question about what do do if the user says "desc //base deps --tree".
There isn't a lot of value in making a tree or recursively enumerating only
private deps. There is some value in doing this for public deps. To keep things
simple, I think it's best to just say --all and -tree don't work for the
individual types.