🐣 GN: Print the import trail when parse errors occur.

GN: Print the import trail when parse errors occur.

This is especially useful when asserts fail when the file should not be
included in the first place.

Example error message:
ERROR at //build/config/android/internal_rules.gni:11:1: Assertion failed.
assert(false)
^-----
See //build/config/android/rules.gni:7:1: whence it was imported.
import("//build/config/android/internal_rules.gni")
^-------------------------------------------------
See //media/midi/BUILD.gn:13:3: whence it was imported.
  import("//build/config/android/rules.gni")
  ^----------------------------------------
See //BUILD.gn:193:7: which caused the package to be included.
      "//media/midi:midi_unittests",
      ^----------------------------

BUG=604972
Committed: https://crrev.com/dbecf02976975b93db37c3b5e3aa26d6fc92c517
Cr-Commit-Position: refs/heads/master@{#388986}

[agrieve, 2016-04-20 02:21:45]

Description was changed from

==========
GN: Print the import trail when parse errors occur.

This is especially useful when asserts fail when the file should not be
included in the first place.

Example error message:
ERROR at //build/config/android/internal_rules.gni:11:1: Assertion failed.
assert(false)
^-----
See //build/config/android/rules.gni:7:1: whence it was imported.
import("//build/config/android/internal_rules.gni")
^-------------------------------------------------
See //media/midi/BUILD.gn:13:3: whence it was imported.
  import("//build/config/android/rules.gni")
  ^----------------------------------------
See //BUILD.gn:193:7: which caused the package to be included.
      "//media/midi:midi_unittests",
      ^----------------------------

BUG=604972
==========

to

==========
GN: Print the import trail when parse errors occur.

This is especially useful when asserts fail when the file should not be
included in the first place.

Example error message:
ERROR at //build/config/android/internal_rules.gni:11:1: Assertion failed.
assert(false)
^-----
See //build/config/android/rules.gni:7:1: whence it was imported.
import("//build/config/android/internal_rules.gni")
^-------------------------------------------------
See //media/midi/BUILD.gn:13:3: whence it was imported.
  import("//build/config/android/rules.gni")
  ^----------------------------------------
See //BUILD.gn:193:7: which caused the package to be included.
      "//media/midi:midi_unittests",
      ^----------------------------

BUG=604972
==========

[agrieve, 2016-04-20 02:21:45]

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

[agrieve, 2016-04-20 02:21:53]

On 2016/04/20 02:21:45, agrieve wrote:
> mailto:agrieve@chromium.org changed reviewers:
> + mailto:brettw@chromium.org

ptal

[agrieve, 2016-04-20 18:28:50]

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

[commit-bot: I haz the power, 2016-04-20 18:29:31]

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

[commit-bot: I haz the power, 2016-04-20 19:38:28]

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

[commit-bot: I haz the power, 2016-04-20 19:38:29]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-04-20 20:53:19]

This looks like a nice improvement. Some comments...

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

https://codereview.chromium.org/1905473003/diff/1/tools/gn/loader.cc#newcode267
tools/gn/loader.cc:267: if (err.has_error()) {
We should check that the origin range isn't empty before appending the sub
error. For some things like the toplevel .gn file or the root BUILD file, there
isn't an origin and it passes LocationRange() as this parameter.

We may want to add an is_null() check to LocationRange and Location to make this
code more sensible.

https://codereview.chromium.org/1905473003/diff/1/tools/gn/loader.cc#newcode268
tools/gn/loader.cc:268: err.AppendSubErr(Err(origin, "which caused the package
to be included."));
Can you replace "package" with "file"? I've not called anything in GN a package
(the google3 terminology), and the semantics are subtlety different.

[Dirk Pranke, 2016-04-20 22:23:23]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-04-20 22:23:24]

I agree that this will be a good improvement; I've wanted this more than a few
times myself.

[agrieve, 2016-04-21 00:59:36]

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

https://codereview.chromium.org/1905473003/diff/1/tools/gn/loader.cc#newcode267
tools/gn/loader.cc:267: if (err.has_error()) {
On 2016/04/20 20:53:19, brettw wrote:
> We should check that the origin range isn't empty before appending the sub
> error. For some things like the toplevel .gn file or the root BUILD file,
there
> isn't an origin and it passes LocationRange() as this parameter.
> 
> We may want to add an is_null() check to LocationRange and Location to make
this
> code more sensible.

Done.

https://codereview.chromium.org/1905473003/diff/1/tools/gn/loader.cc#newcode268
tools/gn/loader.cc:268: err.AppendSubErr(Err(origin, "which caused the package
to be included."));
On 2016/04/20 20:53:19, brettw wrote:
> Can you replace "package" with "file"? I've not called anything in GN a
package
> (the google3 terminology), and the semantics are subtlety different.

Done.

[brettw, 2016-04-21 20:33:36]

lgtm

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

https://codereview.chromium.org/1905473003/diff/20001/tools/gn/location.h#new...
tools/gn/location.h:22: bool is_null() const;
As per style guide, hacker_case function names should be inlined. I think it's
sufficient to check for a null file_.

https://codereview.chromium.org/1905473003/diff/20001/tools/gn/location.h#new...
tools/gn/location.h:49: bool is_null() const;
Ditto.

[agrieve, 2016-04-22 00:16:31]

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

https://codereview.chromium.org/1905473003/diff/20001/tools/gn/location.h#new...
tools/gn/location.h:22: bool is_null() const;
On 2016/04/21 20:33:36, brettw wrote:
> As per style guide, hacker_case function names should be inlined. I think it's
> sufficient to check for a null file_.

Done.

https://codereview.chromium.org/1905473003/diff/20001/tools/gn/location.h#new...
tools/gn/location.h:49: bool is_null() const;
On 2016/04/21 20:33:36, brettw wrote:
> Ditto.

Done.

[agrieve, 2016-04-22 00:16:39]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-04-22 00:17:07]

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

[commit-bot: I haz the power, 2016-04-22 00:58:17]

Description was changed from

==========
GN: Print the import trail when parse errors occur.

This is especially useful when asserts fail when the file should not be
included in the first place.

Example error message:
ERROR at //build/config/android/internal_rules.gni:11:1: Assertion failed.
assert(false)
^-----
See //build/config/android/rules.gni:7:1: whence it was imported.
import("//build/config/android/internal_rules.gni")
^-------------------------------------------------
See //media/midi/BUILD.gn:13:3: whence it was imported.
  import("//build/config/android/rules.gni")
  ^----------------------------------------
See //BUILD.gn:193:7: which caused the package to be included.
      "//media/midi:midi_unittests",
      ^----------------------------

BUG=604972
==========

to

==========
GN: Print the import trail when parse errors occur.

This is especially useful when asserts fail when the file should not be
included in the first place.

Example error message:
ERROR at //build/config/android/internal_rules.gni:11:1: Assertion failed.
assert(false)
^-----
See //build/config/android/rules.gni:7:1: whence it was imported.
import("//build/config/android/internal_rules.gni")
^-------------------------------------------------
See //media/midi/BUILD.gn:13:3: whence it was imported.
  import("//build/config/android/rules.gni")
  ^----------------------------------------
See //BUILD.gn:193:7: which caused the package to be included.
      "//media/midi:midi_unittests",
      ^----------------------------

BUG=604972
==========

[commit-bot: I haz the power, 2016-04-22 00:58:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:42:52]

Description was changed from

==========
GN: Print the import trail when parse errors occur.

This is especially useful when asserts fail when the file should not be
included in the first place.

Example error message:
ERROR at //build/config/android/internal_rules.gni:11:1: Assertion failed.
assert(false)
^-----
See //build/config/android/rules.gni:7:1: whence it was imported.
import("//build/config/android/internal_rules.gni")
^-------------------------------------------------
See //media/midi/BUILD.gn:13:3: whence it was imported.
  import("//build/config/android/rules.gni")
  ^----------------------------------------
See //BUILD.gn:193:7: which caused the package to be included.
      "//media/midi:midi_unittests",
      ^----------------------------

BUG=604972
==========

to

==========
GN: Print the import trail when parse errors occur.

This is especially useful when asserts fail when the file should not be
included in the first place.

Example error message:
ERROR at //build/config/android/internal_rules.gni:11:1: Assertion failed.
assert(false)
^-----
See //build/config/android/rules.gni:7:1: whence it was imported.
import("//build/config/android/internal_rules.gni")
^-------------------------------------------------
See //media/midi/BUILD.gn:13:3: whence it was imported.
  import("//build/config/android/rules.gni")
  ^----------------------------------------
See //BUILD.gn:193:7: which caused the package to be included.
      "//media/midi:midi_unittests",
      ^----------------------------

BUG=604972

Committed: https://crrev.com/dbecf02976975b93db37c3b5e3aa26d6fc92c517
Cr-Commit-Position: refs/heads/master@{#388986}
==========

[commit-bot: I haz the power, 2016-04-22 19:42:53]

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