GN: Prefer toplevel targets for phony names.

GN: Prefer toplevel targets for phony names.

This rewrites the phony target generation in GN to be more scalable. Previously there were two loops and it ahd a lot of conditions in the loop for when to generate certain names. This was subtle and difficult to maintain as we added more variants.

The new scheme has many passes, each lower in priority, that generate rules only if the name hasn't already been taken by something else. This is much easier to reason about and enhance. The new code also accounts for collisions among all output files (previously only some things checked for output file collisions).

In this new scheme we now allow toplevel targets like "//:foo" to claim the name "foo" even when there are multiple things called "foo", as long as none of them are executables or duplicate an output name. This results in some minor and not-material changes in ordering and formatting in the output.

This now uses PathOutput for all file names (previously the "all" rule just wrote the names directly, which could miss some escaping). As a result, some paths have changed slightly but not in meaningful ways.

Doing basic sorting of the files before and after and removing " ./" gives the same set of rules in the current build before and after this change.

Committed: https://crrev.com/66bbbab9be0a7ecf97c124b24d2be4bc7975a0a5
Cr-Commit-Position: refs/heads/master@{#392491}

[brettw, 2016-05-09 20:39:18]

Description was changed from

==========
Prefer toplevel targets for phony names.

BUG=
==========

to

==========
GN: Prefer toplevel targets for phony names.

This rewrites the phony target generation in GN to be more scalable. Previously
there were two loops and it ahd a lot of conditions in the loop for when to
generate certain names. This was subtle and difficult to maintain as we added
more variants.

The new scheme has many passes, each lower in priority, that generate rules only
if the name hasn't already been taken by something else. This is much easier to
reason about and enhance. The new code also accounts for collisions among all
output files (previously only some things checked for output file collisions).

In this new scheme we now allow toplevel targets like "//:foo" to claim the name
"foo" even when there are multiple things called "foo", as long as none of them
are executables or duplicate an output name.
==========

[brettw, 2016-05-09 20:39:18]

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

[brettw, 2016-05-09 20:41:35]

Description was changed from

==========
GN: Prefer toplevel targets for phony names.

This rewrites the phony target generation in GN to be more scalable. Previously
there were two loops and it ahd a lot of conditions in the loop for when to
generate certain names. This was subtle and difficult to maintain as we added
more variants.

The new scheme has many passes, each lower in priority, that generate rules only
if the name hasn't already been taken by something else. This is much easier to
reason about and enhance. The new code also accounts for collisions among all
output files (previously only some things checked for output file collisions).

In this new scheme we now allow toplevel targets like "//:foo" to claim the name
"foo" even when there are multiple things called "foo", as long as none of them
are executables or duplicate an output name.
==========

to

==========
GN: Prefer toplevel targets for phony names.

This rewrites the phony target generation in GN to be more scalable. Previously
there were two loops and it ahd a lot of conditions in the loop for when to
generate certain names. This was subtle and difficult to maintain as we added
more variants.

The new scheme has many passes, each lower in priority, that generate rules only
if the name hasn't already been taken by something else. This is much easier to
reason about and enhance. The new code also accounts for collisions among all
output files (previously only some things checked for output file collisions).

In this new scheme we now allow toplevel targets like "//:foo" to claim the name
"foo" even when there are multiple things called "foo", as long as none of them
are executables or duplicate an output name.

This now uses PathOutput for all file names (previously the "all" rule just
wrote the names directly, which could miss some escaping). As a result, some
paths have changed slightly but not in meaningful ways.

Doing basic sorting of the files before and after and removing " ./" gives the
same set of rules in the current build before and after this change.
==========

[brettw, 2016-05-09 20:41:43]

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

[commit-bot: I haz the power, 2016-05-09 20:42:09]

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

[commit-bot: I haz the power, 2016-05-09 21:07:00]

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

[commit-bot: I haz the power, 2016-05-09 21:07:01]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[brettw, 2016-05-09 21:14:42]

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

[brettw, 2016-05-09 21:15:10]

Description was changed from

==========
GN: Prefer toplevel targets for phony names.

This rewrites the phony target generation in GN to be more scalable. Previously
there were two loops and it ahd a lot of conditions in the loop for when to
generate certain names. This was subtle and difficult to maintain as we added
more variants.

The new scheme has many passes, each lower in priority, that generate rules only
if the name hasn't already been taken by something else. This is much easier to
reason about and enhance. The new code also accounts for collisions among all
output files (previously only some things checked for output file collisions).

In this new scheme we now allow toplevel targets like "//:foo" to claim the name
"foo" even when there are multiple things called "foo", as long as none of them
are executables or duplicate an output name.

This now uses PathOutput for all file names (previously the "all" rule just
wrote the names directly, which could miss some escaping). As a result, some
paths have changed slightly but not in meaningful ways.

Doing basic sorting of the files before and after and removing " ./" gives the
same set of rules in the current build before and after this change.
==========

to

==========
GN: Prefer toplevel targets for phony names.

This rewrites the phony target generation in GN to be more scalable. Previously
there were two loops and it ahd a lot of conditions in the loop for when to
generate certain names. This was subtle and difficult to maintain as we added
more variants.

The new scheme has many passes, each lower in priority, that generate rules only
if the name hasn't already been taken by something else. This is much easier to
reason about and enhance. The new code also accounts for collisions among all
output files (previously only some things checked for output file collisions).

In this new scheme we now allow toplevel targets like "//:foo" to claim the name
"foo" even when there are multiple things called "foo", as long as none of them
are executables or duplicate an output name. This results in some minor and
not-material changes in ordering and formatting in the output.

This now uses PathOutput for all file names (previously the "all" rule just
wrote the names directly, which could miss some escaping). As a result, some
paths have changed slightly but not in meaningful ways.

Doing basic sorting of the files before and after and removing " ./" gives the
same set of rules in the current build before and after this change.
==========

[commit-bot: I haz the power, 2016-05-09 21:15:16]

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

[commit-bot: I haz the power, 2016-05-09 21:58:25]

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

[commit-bot: I haz the power, 2016-05-09 21:58:26]

Dry run: This issue passed the CQ dry run.

[scottmg, 2016-05-09 23:31:53]

I'm vaguely worried about a bot building something and then having that thing
change because it got overridden. But I guess that's a problem in theory now
too.

lgtm

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

https://codereview.chromium.org/1958783003/diff/80001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:220: namespace {
Unusual to have another one here instead of putting this at the top of the file.

https://codereview.chromium.org/1958783003/diff/80001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:232: }  // namesoace
"namespace"

https://codereview.chromium.org/1958783003/diff/80001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:366: // non of the higher-priority rules above
claimed it.
non->none

https://codereview.chromium.org/1958783003/diff/80001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:379: out_ << (" $\n    ");
no ()

[brettw, 2016-05-09 23:50:30]

Review comments

[brettw, 2016-05-09 23:50:31]

The CQ bit was checked by brettw@chromium.org

[brettw, 2016-05-09 23:50:32]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1958783003/#ps100001
(title: "Review comments")

[commit-bot: I haz the power, 2016-05-09 23:50:56]

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

[commit-bot: I haz the power, 2016-05-10 00:33:18]

Description was changed from

==========
GN: Prefer toplevel targets for phony names.

This rewrites the phony target generation in GN to be more scalable. Previously
there were two loops and it ahd a lot of conditions in the loop for when to
generate certain names. This was subtle and difficult to maintain as we added
more variants.

The new scheme has many passes, each lower in priority, that generate rules only
if the name hasn't already been taken by something else. This is much easier to
reason about and enhance. The new code also accounts for collisions among all
output files (previously only some things checked for output file collisions).

In this new scheme we now allow toplevel targets like "//:foo" to claim the name
"foo" even when there are multiple things called "foo", as long as none of them
are executables or duplicate an output name. This results in some minor and
not-material changes in ordering and formatting in the output.

This now uses PathOutput for all file names (previously the "all" rule just
wrote the names directly, which could miss some escaping). As a result, some
paths have changed slightly but not in meaningful ways.

Doing basic sorting of the files before and after and removing " ./" gives the
same set of rules in the current build before and after this change.
==========

to

==========
GN: Prefer toplevel targets for phony names.

This rewrites the phony target generation in GN to be more scalable. Previously
there were two loops and it ahd a lot of conditions in the loop for when to
generate certain names. This was subtle and difficult to maintain as we added
more variants.

The new scheme has many passes, each lower in priority, that generate rules only
if the name hasn't already been taken by something else. This is much easier to
reason about and enhance. The new code also accounts for collisions among all
output files (previously only some things checked for output file collisions).

In this new scheme we now allow toplevel targets like "//:foo" to claim the name
"foo" even when there are multiple things called "foo", as long as none of them
are executables or duplicate an output name. This results in some minor and
not-material changes in ordering and formatting in the output.

This now uses PathOutput for all file names (previously the "all" rule just
wrote the names directly, which could miss some escaping). As a result, some
paths have changed slightly but not in meaningful ways.

Doing basic sorting of the files before and after and removing " ./" gives the
same set of rules in the current build before and after this change.
==========

[commit-bot: I haz the power, 2016-05-10 00:33:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-10 00:35:21]

Description was changed from

==========
GN: Prefer toplevel targets for phony names.

This rewrites the phony target generation in GN to be more scalable. Previously
there were two loops and it ahd a lot of conditions in the loop for when to
generate certain names. This was subtle and difficult to maintain as we added
more variants.

The new scheme has many passes, each lower in priority, that generate rules only
if the name hasn't already been taken by something else. This is much easier to
reason about and enhance. The new code also accounts for collisions among all
output files (previously only some things checked for output file collisions).

In this new scheme we now allow toplevel targets like "//:foo" to claim the name
"foo" even when there are multiple things called "foo", as long as none of them
are executables or duplicate an output name. This results in some minor and
not-material changes in ordering and formatting in the output.

This now uses PathOutput for all file names (previously the "all" rule just
wrote the names directly, which could miss some escaping). As a result, some
paths have changed slightly but not in meaningful ways.

Doing basic sorting of the files before and after and removing " ./" gives the
same set of rules in the current build before and after this change.
==========

to

==========
GN: Prefer toplevel targets for phony names.

This rewrites the phony target generation in GN to be more scalable. Previously
there were two loops and it ahd a lot of conditions in the loop for when to
generate certain names. This was subtle and difficult to maintain as we added
more variants.

The new scheme has many passes, each lower in priority, that generate rules only
if the name hasn't already been taken by something else. This is much easier to
reason about and enhance. The new code also accounts for collisions among all
output files (previously only some things checked for output file collisions).

In this new scheme we now allow toplevel targets like "//:foo" to claim the name
"foo" even when there are multiple things called "foo", as long as none of them
are executables or duplicate an output name. This results in some minor and
not-material changes in ordering and formatting in the output.

This now uses PathOutput for all file names (previously the "all" rule just
wrote the names directly, which could miss some escaping). As a result, some
paths have changed slightly but not in meaningful ways.

Doing basic sorting of the files before and after and removing " ./" gives the
same set of rules in the current build before and after this change.

Committed: https://crrev.com/66bbbab9be0a7ecf97c124b24d2be4bc7975a0a5
Cr-Commit-Position: refs/heads/master@{#392491}
==========

[commit-bot: I haz the power, 2016-05-10 00:35:23]

Patchset 6 (id:??) landed as
https://crrev.com/66bbbab9be0a7ecf97c124b24d2be4bc7975a0a5
Cr-Commit-Position: refs/heads/master@{#392491}