Sort "deps" and "public_deps" when running the "gn format" command.

Sort "deps" and "public_deps" when running the "gn format" command.

The "deps" and "public_deps" are sorted using the following rules:
  1. relative targets (":a", "../b") sort before absolute targets ("//c"),
  2. each group (relative and absolute) is sorted alphabetically.

BUG=554928
Committed: https://crrev.com/354261e7dd491e88fde3b3bd3fa5ef6f1c629f00
Cr-Commit-Position: refs/heads/master@{#360042}

[sdefresne, 2015-11-13 14:13:04]

sdefresne@chromium.org changed reviewers:
+ dpranke@chromium.org, scottmg@chromium.org

[sdefresne, 2015-11-13 14:13:05]

Please take a look.

[sdefresne, 2015-11-13 14:14:00]

The new test fails with the following error before the patch and pass with it:

../../tools/gn/command_format_unittest.cc:103: Failure
Value of: out
  Actual: "source_set(\"test\") {\n  deps = [\n    \"//c\",\n    \"//a\",\n   
\":b\",\n  ]\n\n  if (condition) {\n    deps += [\n      \"//z\",\n  
    \"//y\",\n      \"//x\",\n    ]\n  }\n}\n"
Expected: expected
Which is: "source_set(\"test\") {\n  deps = [\n    \"//a\",\n    \"//c\",\n   
\":b\",\n  ]\n\n  if (condition) {\n    deps += [\n      \"//x\",\n  
    \"//y\",\n      \"//z\",\n    ]\n  }\n}\n"
With diff:
@@ -1,6 +1,6 @@
 source_set(\"test\") {
   deps = [
-    \"//a\",
-    \"//c\",
+    \"//c\",
+    \"//a\",
     \":b\",
   ]
@@ -8,7 +8,7 @@
   if (condition) {
     deps += [
-      \"//x\",
+      \"//z\",
       \"//y\",
-      \"//z\",
+      \"//x\",
     ]
   }

[sdefresne, 2015-11-13 16:26:55]

Proof-of-concept application of this new formatting can be found at
https://codereview.chromium.org/1439973004.

I've sent the CL to the try bots to check that reordering the deps does not
break anything. I have no plan to land
https://codereview.chromium.org/1439973004, I think we should either split it in
multiple smaller CL or let people reformat the individual files when prompted by
PRESUBMIT checks.

[Dirk Pranke, 2015-11-13 17:31:31]

no lgtm.

the code change is fine but it produces the wrong behavior (see comment below).
We need to sort deps differently than we do sources.

https://codereview.chromium.org/1442023002/diff/1/tools/gn/format_test_data/0...
File tools/gn/format_test_data/063.golden (right):

https://codereview.chromium.org/1442023002/diff/1/tools/gn/format_test_data/0...
tools/gn/format_test_data/063.golden:5: ":b",
relative targets like ":b" and "../b" should be listed before
absolute targets, even if that's not how sort() would sort them.

[scottmg, 2015-11-13 18:46:00]

It would also be worth running this on everything first and scrutinizing the
diff to make sure it looks OK.

(Something like `git ls-files "*.gn" "*.gni" | grep -v format_test_data | sed -e
"s/^/gn format --in-place /" >script`)

[Dirk Pranke, 2015-11-13 18:48:48]

On 2015/11/13 18:46:00, scottmg wrote:
> It would also be worth running this on everything first and scrutinizing the
> diff to make sure it looks OK.
> 
> (Something like `git ls-files "*.gn" "*.gni" | grep -v format_test_data | sed
-e
> "s/^/gn format --in-place /" >script`)

I think that's more or less what he did in #4
(https://codereview.chromium.org/1439973004),
which is how I noticed the problem.

[scottmg, 2015-11-13 18:53:12]

On 2015/11/13 18:48:48, Dirk Pranke wrote:
> On 2015/11/13 18:46:00, scottmg wrote:
> > It would also be worth running this on everything first and scrutinizing the
> > diff to make sure it looks OK.
> > 
> > (Something like `git ls-files "*.gn" "*.gni" | grep -v format_test_data |
sed
> -e
> > "s/^/gn format --in-place /" >script`)
> 
> I think that's more or less what he did in #4
> (https://codereview.chromium.org/1439973004),
> which is how I noticed the problem.

Oh, sorry missed that. Agreed in the order you suggested.

[scottmg, 2015-11-13 18:54:01]

On 2015/11/13 18:53:12, scottmg wrote:
> On 2015/11/13 18:48:48, Dirk Pranke wrote:
> > On 2015/11/13 18:46:00, scottmg wrote:
> > > It would also be worth running this on everything first and scrutinizing
the
> > > diff to make sure it looks OK.
> > > 
> > > (Something like `git ls-files "*.gn" "*.gni" | grep -v format_test_data |
> sed
> > -e
> > > "s/^/gn format --in-place /" >script`)
> > 
> > I think that's more or less what he did in #4
> > (https://codereview.chromium.org/1439973004),
> > which is how I noticed the problem.
> 
> Oh, sorry missed that. Agreed in the order you suggested.

Agreed on, too.

[sdefresne, 2015-11-14 09:08:07]

On 2015/11/13 at 17:31:31, dpranke wrote:
> no lgtm.
> 
> the code change is fine but it produces the wrong behavior (see comment
below). We need to sort deps differently than we do sources.
> 
>
https://codereview.chromium.org/1442023002/diff/1/tools/gn/format_test_data/0...
> File tools/gn/format_test_data/063.golden (right):
> 
>
https://codereview.chromium.org/1442023002/diff/1/tools/gn/format_test_data/0...
> tools/gn/format_test_data/063.golden:5: ":b",
> relative targets like ":b" and "../b" should be listed before
> absolute targets, even if that's not how sort() would sort them.

Make sense. Is there a page explaining the full ordering rules?

For example, should "//a:b" sort before or after "//a_b"? Or is it just that we
should first sort reference with :, then with .. then with //?

[sdefresne, 2015-11-14 11:00:54]

I've updated the CL to use the suggested ordering, please take another look. The
dependent CL (https://codereview.chromium.org/1439973004) as been updated
accordingly to investigate the output of the new formatting rule on the Chromium
tree.

[tfarina, 2015-11-14 12:05:29]

Thanks for doing this Sylvain!

I don't know how I missed that ;)

I agree with Dirk that ':a' should become before '//b'.

[Dirk Pranke, 2015-11-14 16:55:14]

On 2015/11/14 09:08:07, sdefresne wrote:
> On 2015/11/13 at 17:31:31, dpranke wrote:
> > no lgtm.
> > 
> > the code change is fine but it produces the wrong behavior (see comment
> below). We need to sort deps differently than we do sources.
> > 
> >
>
https://codereview.chromium.org/1442023002/diff/1/tools/gn/format_test_data/0...
> > File tools/gn/format_test_data/063.golden (right):
> > 
> >
>
https://codereview.chromium.org/1442023002/diff/1/tools/gn/format_test_data/0...
> > tools/gn/format_test_data/063.golden:5: ":b",
> > relative targets like ":b" and "../b" should be listed before
> > absolute targets, even if that's not how sort() would sort them.
> 
> Make sense. Is there a page explaining the full ordering rules?

There's

https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

which has some guidelines, but isn't exhaustive.

> For example, should "//a:b" sort before or after "//a_b"? Or is it just that
we
> should first sort reference with :, then with .. then with //?

I would probably sort a:b before a_b and "../a" before "//a" 

(which what I think sort() will do anyway).

[Dirk Pranke, 2015-11-16 00:34:34]

lgtm, though scottmg should review it also. 

the example reformatted CL looks more-or-less correct also, though I only looked
at the first few files.

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

https://codereview.chromium.org/1442023002/diff/60001/tools/gn/command_format...
tools/gn/command_format.cc:132: // are sorted alphabetically while the 'deps'
are sorted puttint first the
nit: s/puttint first/putting first/

[sdefresne, 2015-11-16 09:39:39]

On 2015/11/14 at 16:55:14, dpranke wrote:
> On 2015/11/14 09:08:07, sdefresne wrote:
> > On 2015/11/13 at 17:31:31, dpranke wrote:
> > > no lgtm.
> > > 
> > > the code change is fine but it produces the wrong behavior (see comment
> > below). We need to sort deps differently than we do sources.
> > > 
> > >
> >
https://codereview.chromium.org/1442023002/diff/1/tools/gn/format_test_data/0...
> > > File tools/gn/format_test_data/063.golden (right):
> > > 
> > >
> >
https://codereview.chromium.org/1442023002/diff/1/tools/gn/format_test_data/0...
> > > tools/gn/format_test_data/063.golden:5: ":b",
> > > relative targets like ":b" and "../b" should be listed before
> > > absolute targets, even if that's not how sort() would sort them.
> > 
> > Make sense. Is there a page explaining the full ordering rules?
> 
> There's
> 
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...
> 
> which has some guidelines, but isn't exhaustive.
> 
> > For example, should "//a:b" sort before or after "//a_b"? Or is it just that
we
> > should first sort reference with :, then with .. then with //?
> 
> I would probably sort a:b before a_b and "../a" before "//a" 
> 
> (which what I think sort() will do anyway).

OK. This looks like what my CL does. Thanks.
scottmg: please take a look.

[scottmg, 2015-11-16 17:40:08]

A couple minor things noted in https://codereview.chromium.org/1439973004/.

https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc
File tools/gn/parse_tree.cc (right):

https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:22: if (!str.starts_with("\"") || !str.ends_with("\""))
When does this happen?

https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:528: if (!node->AsLiteral() && !node->AsIdentifier() &&
!node->AsAccessor()) {
Can you add a test that encounters this case?

[sdefresne, 2015-11-16 19:49:18]

On 2015/11/16 at 17:40:08, scottmg wrote:
> A couple minor things noted in https://codereview.chromium.org/1439973004/.
> 
> https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc
> File tools/gn/parse_tree.cc (right):
> 
>
https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc#...
> tools/gn/parse_tree.cc:22: if (!str.starts_with("\"") || !str.ends_with("\""))
> When does this happen?
> 
>
https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc#...
> tools/gn/parse_tree.cc:528: if (!node->AsLiteral() && !node->AsIdentifier() &&
!node->AsAccessor()) {
> Can you add a test that encounters this case?

I've addressed the comments and added unit tests. I also updated the code to
also sort the "public_deps" lists because I think otherwise it would have been
the next logical step :-)

I have a question regarding the comments in the other CL. You said that a blank
line was probably used to group the "deps". Currently my CL use the same code to
group the entries as is used for the "sources". According to
IsSortRangeSeparator, the separators are block comments or empty line followed
by a line with an attached comment. Should the implementation be updated to also
group at any empty lines? Should I split the implementation of
SortAsSourceList/SortAsDepsList to use different grouping behaviour? Should I
keep the current behaviour and instead change affected BUILD.gn to use comments
to fix the grouping?

Can you take another look?

[sdefresne, 2015-11-16 19:52:06]

https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc
File tools/gn/parse_tree.cc (right):

https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:22: if (!str.starts_with("\"") || !str.ends_with("\""))
On 2015/11/16 at 17:40:08, scottmg wrote:
> When does this happen?

This happens when the elements in the "deps" list are not string but expression,
for example,

  some_target = ":some_target"
  deps = [
    some_target,
    ":another_target",
  ]

Here the code will be invoked with to strings, "some_target" and
"\":another_target\"". I'm not sure how we should sort those, I guess this
happens rarely though. Maybe I should instead sort them separately in a third
category.

I've added a test 065.gn/065.golden for this case.

https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:528: if (!node->AsLiteral() && !node->AsIdentifier() &&
!node->AsAccessor()) {
On 2015/11/16 at 17:40:08, scottmg wrote:
> Can you add a test that encounters this case?

Sure, this is 064.gn/064.golden.

[scottmg, 2015-11-16 19:54:34]

On 2015/11/16 19:49:18, sdefresne wrote:
> On 2015/11/16 at 17:40:08, scottmg wrote:
> > A couple minor things noted in https://codereview.chromium.org/1439973004/.
> > 
> > https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc
> > File tools/gn/parse_tree.cc (right):
> > 
> >
>
https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc#...
> > tools/gn/parse_tree.cc:22: if (!str.starts_with("\"") ||
!str.ends_with("\""))
> > When does this happen?
> > 
> >
>
https://codereview.chromium.org/1442023002/diff/80001/tools/gn/parse_tree.cc#...
> > tools/gn/parse_tree.cc:528: if (!node->AsLiteral() && !node->AsIdentifier()
&&
> !node->AsAccessor()) {
> > Can you add a test that encounters this case?
> 
> I've addressed the comments and added unit tests. I also updated the code to
> also sort the "public_deps" lists because I think otherwise it would have been
> the next logical step :-)
> 
> I have a question regarding the comments in the other CL. You said that a
blank
> line was probably used to group the "deps". Currently my CL use the same code
to
> group the entries as is used for the "sources". According to
> IsSortRangeSeparator, the separators are block comments or empty line followed
> by a line with an attached comment. Should the implementation be updated to
also
> group at any empty lines? Should I split the implementation of
> SortAsSourceList/SortAsDepsList to use different grouping behaviour? Should I
> keep the current behaviour and instead change affected BUILD.gn to use
comments
> to fix the grouping?

Yeah, I'm not sure we need to do anything. The one I noted just looked like a
separate block, and then two semi-unrelated blocks got merged together, so it
looked like those were part of the mojo edk comment.

Doing nothing, or adding a comment to the second block both seem like reasonable
resolutions.

> 
> Can you take another look?

[scottmg, 2015-11-16 19:55:27]

I didn't relook at the other one for all the public_deps, but this CL lgtm with
the noted minor changes.

https://codereview.chromium.org/1442023002/diff/100001/tools/gn/command_forma...
File tools/gn/command_format.cc (right):

https://codereview.chromium.org/1442023002/diff/100001/tools/gn/command_forma...
tools/gn/command_format.cc:131: // Sort a list on the RHS if the LHS is
'sources' or 'deps'. The 'sources'
'sources', 'deps', or 'public_deps'.

https://codereview.chromium.org/1442023002/diff/100001/tools/gn/command_forma...
tools/gn/command_format.cc:132: // are sorted alphabetically while the 'deps'
are sorted putting first the
And here.

https://codereview.chromium.org/1442023002/diff/100001/tools/gn/command_forma...
tools/gn/command_format.cc:315: // TODO(scottmg): Sort more than 'sources' and
'deps'?
And here, or just delete this comment.

[sdefresne, 2015-11-16 20:38:59]

Description was changed from

==========
Sort "deps" when running the "gn format" command.

The "deps" are sorted using the following rules:
  1. relative targets (":a", "../b") sort before absolute targets ("//c"),
  2. each group (relative and absolute) is sorted alphabetically.

BUG=554928
==========

to

==========
Sort "deps" and "public_deps" when running the "gn format" command.

The "deps" and "public_deps" are sorted using the following rules:
  1. relative targets (":a", "../b") sort before absolute targets ("//c"),
  2. each group (relative and absolute) is sorted alphabetically.

BUG=554928
==========

[sdefresne, 2015-11-16 20:48:05]

Thank you for the review.

https://codereview.chromium.org/1442023002/diff/100001/tools/gn/command_forma...
File tools/gn/command_format.cc (right):

https://codereview.chromium.org/1442023002/diff/100001/tools/gn/command_forma...
tools/gn/command_format.cc:131: // Sort a list on the RHS if the LHS is
'sources' or 'deps'. The 'sources'
On 2015/11/16 at 19:55:27, scottmg wrote:
> 'sources', 'deps', or 'public_deps'.

Done.

https://codereview.chromium.org/1442023002/diff/100001/tools/gn/command_forma...
tools/gn/command_format.cc:132: // are sorted alphabetically while the 'deps'
are sorted putting first the
On 2015/11/16 at 19:55:27, scottmg wrote:
> And here.

Done.

https://codereview.chromium.org/1442023002/diff/100001/tools/gn/command_forma...
tools/gn/command_format.cc:315: // TODO(scottmg): Sort more than 'sources' and
'deps'?
On 2015/11/16 at 19:55:27, scottmg wrote:
> And here, or just delete this comment.

Done.

[sdefresne, 2015-11-17 09:11:25]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-11-17 09:11:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1442023002/#ps120001
(title: "Fix comments following suggestions")

[commit-bot: I haz the power, 2015-11-17 09:12:02]

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

[commit-bot: I haz the power, 2015-11-17 09:45:42]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-17 09:46:26]

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