Use $root:default as a "default" rule

Use $root:default as a "default" rule

When the root build file is changed in .gn file, we should check
$root:default rather than //:default when looking for "default" rule.

Review-Url: https://codereview.chromium.org/2824153002
Cr-Commit-Position: refs/heads/master@{#465758}
Committed: https://chromium.googlesource.com/chromium/src/+/ddb3babce43a642e5f4e985e2a69c741057aad3c

[Petr Hosek, 2017-04-18 08:59:51]

Description was changed from

==========
Use $root:default as a "default" rule

When the root build file is changed in .gn file, we should check
$root:default rather than //:default when looking for "default" rule.
==========

to

==========
Use $root:default as a "default" rule

When the root build file is changed in .gn file, we should check
$root:default rather than //:default when looking for "default" rule.
==========

[Petr Hosek, 2017-04-18 08:59:51]

phosek@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org

[Dirk Pranke, 2017-04-18 20:12:15]

This looks reasonable to me. 

However, looking at the documentation for "root" (setup.cc:95), it says:

root [optional]
  Label of the root build target. The GN build will start by loading the
  build file containing this target name. This defaults to "//:" which will	    
  cause the file //BUILD.gn to be loaded.

AFAIK, "//:" isn't a legal target (at least, I don't know what it would refer
to). Can we change what this says, or something, as part of this change?

[Petr Hosek, 2017-04-19 18:02:47]

On 2017/04/18 20:12:15, Dirk Pranke wrote:
> This looks reasonable to me. 

I forgot to actually write down the default target which should be fixed now.
 
> However, looking at the documentation for "root" (setup.cc:95), it says:
> 
> root [optional]
>   Label of the root build target. The GN build will start by loading the
>   build file containing this target name. This defaults to "//:" which will	  
 
>   cause the file //BUILD.gn to be loaded.
> 
> AFAIK, "//:" isn't a legal target (at least, I don't know what it would refer
> to). Can we change what this says, or something, as part of this change?

My understanding is that "//:" in this case refers to "//<empty>:<empty>" which
is a little strange but is really what GN internally does. Shall I just try to
rephrase it without explicitly mentioning the label?

[Dirk Pranke, 2017-04-19 18:18:22]

On 2017/04/19 18:02:47, Petr Hosek wrote:
> On 2017/04/18 20:12:15, Dirk Pranke wrote:
> > This looks reasonable to me. 
> 
> I forgot to actually write down the default target which should be fixed now.
>  
> > However, looking at the documentation for "root" (setup.cc:95), it says:
> > 
> > root [optional]
> >   Label of the root build target. The GN build will start by loading the
> >   build file containing this target name. This defaults to "//:" which will	
 
>  
> >   cause the file //BUILD.gn to be loaded.
> > 
> > AFAIK, "//:" isn't a legal target (at least, I don't know what it would
refer
> > to). Can we change what this says, or something, as part of this change?
> 
> My understanding is that "//:" in this case refers to "//<empty>:<empty>"
which
> is a little strange but is really what GN internally does. Shall I just try to
> rephrase it without explicitly mentioning the label?

Yeah, that's probably what I'd do.

brettw@, thoughts?

[brettw, 2017-04-19 19:49:25]

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/build_settings.cc
File tools/gn/build_settings.cc (right):

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/build_settings...
tools/gn/build_settings.cc:28: void BuildSettings::SetRootTargetLabel(const
Label& l) {
In the header you call the variable "r" which is better since it doesn't look
like a one.

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/docs/reference.md
File tools/gn/docs/reference.md (right):

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/docs/reference...
tools/gn/docs/reference.md:5864: typing "ninja"). If there is a target named
"//:default" (or $root:default in
"root" isn't a variable anywhere but the dotfile, so this text is confusing to
me. Maybe just replace this whole sentence with "If there is a target named
"default" in the root build file..."

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/ninja_build_wr...
File tools/gn/ninja_build_writer.cc (right):

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:352: typing "ninja"). If there is a target named
"//:default" (or $root:default in
Ditto

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:549: std::string name =
default_target->label().GetUserVisibleName(false);
This is making assumptions about the types of rules written. This should
reference the file associated with the target instead. This will be
default_target->dependency_output_file() and write with a PathOutput object.

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/setup.cc
File tools/gn/setup.cc (right):

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/setup.cc#newco...
tools/gn/setup.cc:98: build file containing this target name. This defaults to
label which will
This new text isn't gramatical. I think the old text is probably fine. It's what
actually happens, and I don't think people are commonly reading this
documentation and setting the root such that they would get confused by it.

[Petr Hosek, 2017-04-19 20:20:26]

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/build_settings.cc
File tools/gn/build_settings.cc (right):

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/build_settings...
tools/gn/build_settings.cc:28: void BuildSettings::SetRootTargetLabel(const
Label& l) {
On 2017/04/19 19:49:25, brettw (plz ping after 24h) wrote:
> In the header you call the variable "r" which is better since it doesn't look
> like a one.

Done.

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/docs/reference.md
File tools/gn/docs/reference.md (right):

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/docs/reference...
tools/gn/docs/reference.md:5864: typing "ninja"). If there is a target named
"//:default" (or $root:default in
On 2017/04/19 19:49:25, brettw (plz ping after 24h) wrote:
> "root" isn't a variable anywhere but the dotfile, so this text is confusing to
> me. Maybe just replace this whole sentence with "If there is a target named
> "default" in the root build file..."

Done.

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/ninja_build_wr...
File tools/gn/ninja_build_writer.cc (right):

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:352: typing "ninja"). If there is a target named
"//:default" (or $root:default in
On 2017/04/19 19:49:25, brettw (plz ping after 24h) wrote:
> Ditto

Done.

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:549: std::string name =
default_target->label().GetUserVisibleName(false);
On 2017/04/19 19:49:25, brettw (plz ping after 24h) wrote:
> This is making assumptions about the types of rules written. This should
> reference the file associated with the target instead. This will be
> default_target->dependency_output_file() and write with a PathOutput object.

Done.

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/setup.cc
File tools/gn/setup.cc (right):

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/setup.cc#newco...
tools/gn/setup.cc:98: build file containing this target name. This defaults to
label which will
On 2017/04/19 19:49:25, brettw (plz ping after 24h) wrote:
> This new text isn't gramatical. I think the old text is probably fine. It's
what
> actually happens, and I don't think people are commonly reading this
> documentation and setting the root such that they would get confused by it.

Done.

[brettw, 2017-04-19 20:44:25]

lgtm

[Petr Hosek, 2017-04-19 20:49:22]

The CQ bit was checked by phosek@chromium.org

[commit-bot: I haz the power, 2017-04-19 20:50:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 21:27:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492634962585980,
"parent_rev": "8aeea939c0f289b85181b6e8fc9c0966451e9369", "commit_rev":
"ddb3babce43a642e5f4e985e2a69c741057aad3c"}

[commit-bot: I haz the power, 2017-04-19 21:27:52]

Description was changed from

==========
Use $root:default as a "default" rule

When the root build file is changed in .gn file, we should check
$root:default rather than //:default when looking for "default" rule.
==========

to

==========
Use $root:default as a "default" rule

When the root build file is changed in .gn file, we should check
$root:default rather than //:default when looking for "default" rule.

Review-Url: https://codereview.chromium.org/2824153002
Cr-Commit-Position: refs/heads/master@{#465758}
Committed:
https://chromium.googlesource.com/chromium/src/+/ddb3babce43a642e5f4e985e2a69...
==========

[commit-bot: I haz the power, 2017-04-19 21:27:52]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ddb3babce43a642e5f4e985e2a69...

[Dirk Pranke, 2017-04-19 23:01:27]

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/setup.cc
File tools/gn/setup.cc (right):

https://codereview.chromium.org/2824153002/diff/60001/tools/gn/setup.cc#newco...
tools/gn/setup.cc:98: build file containing this target name. This defaults to
label which will
On 2017/04/19 20:20:26, Petr Hosek wrote:
> On 2017/04/19 19:49:25, brettw (plz ping after 24h) wrote:
> > This new text isn't gramatical. I think the old text is probably fine. It's
> what
> > actually happens, and I don't think people are commonly reading this
> > documentation and setting the root such that they would get confused by it.
> 
> Done.

FWIW, I still find it confusing.