gn check could check system includes too

[Gergely Nagy]

Hi!

We are trying to use gn for an external project that uses "#include <rel_path_from_root_source/file_name.h>" style includes and currently gn check only looks at includes that use quotes.

I patched gn to check system includes as well, and gn check still works and says OK for the whole chromium repo.

I counted all the include lines in the chromium repo from gn check, and it checks dependencies for 73381 quote includes and 54 system includes.

Here is the list of the 54 includes that are included as system includes and therefore not currently checked in gn.

 //components/feedback/anonymizer_tool.cc -> //base/strings/string_number_conversions.h

 //components/feedback/anonymizer_tool.cc -> //base/strings/string_util.h

 //components/feedback/anonymizer_tool.cc -> //base/strings/stringprintf.h

 //components/feedback/anonymizer_tool.h -> //base/macros.h

 //components/password_manager/core/browser/password_ui_utils.cc -> //components/password_manager/core/browser/password_ui_utils.h

 //components/update_client/utils.h -> //base/memory/scoped_ptr.h

 //crypto/p224_spake.cc -> //crypto/p224_spake.h

 //crypto/p224_spake.cc -> //base/logging.h

 //crypto/p224_spake.cc -> //crypto/p224.h

 //crypto/p224_spake.cc -> //crypto/random.h

 //crypto/p224_spake.cc -> //crypto/secure_util.h

 //crypto/p224_spake.h -> //crypto/p224.h

 //crypto/p224_spake.h -> //crypto/sha2.h

 //crypto/p224_spake.h -> //crypto/p224.h

 //crypto/p224_spake.h -> //crypto/sha2.h

 //crypto/p224_spake.cc -> //crypto/p224_spake.h

 //crypto/p224_spake.cc -> //base/logging.h

 //crypto/p224_spake.cc -> //crypto/p224.h

 //crypto/p224_spake.cc -> //crypto/random.h

 //crypto/p224_spake.cc -> //crypto/secure_util.h

 //third_party/WebKit/Source/core/dom/URLSearchParams.h -> //base/gtest_prod_util.h

 //third_party/WebKit/Source/core/loader/LinkHeaderTest.cpp -> //base/macros.h

 //third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp -> //base/macros.h

 //third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp -> //base/macros.h

 //third_party/WebKit/Source/core/svg/SVGAnimateElement.h -> //base/gtest_prod_util.h

 //third_party/WebKit/Source/modules/filesystem/DOMFileSystemBase.cpp -> //url/url_util.h

 //third_party/WebKit/Source/modules/mediasession/MediaSessionError.h -> //v8/include/v8.h

 //third_party/WebKit/Source/platform/LinkHash.cpp -> //url/url_util.h

 //third_party/WebKit/Source/platform/heap/RunAllTests.cpp -> //base/bind.h

 //third_party/WebKit/Source/platform/heap/RunAllTests.cpp -> //base/test/launcher/unit_test_launcher.h

 //third_party/WebKit/Source/platform/heap/RunAllTests.cpp -> //base/test/test_suite.h

 //third_party/WebKit/Source/platform/heap/RunAllTests.cpp -> //base/time/time.h

 //third_party/WebKit/Source/platform/heap/RunAllTests.cpp -> //content/test/blink_test_environment.h

 //third_party/WebKit/Source/platform/testing/RunAllTests.cpp -> //base/bind.h

 //third_party/WebKit/Source/platform/testing/RunAllTests.cpp -> //base/bind_helpers.h

 //third_party/WebKit/Source/platform/testing/RunAllTests.cpp -> //base/test/launcher/unit_test_launcher.h

 //third_party/WebKit/Source/platform/testing/RunAllTests.cpp -> //base/test/test_suite.h

 //third_party/WebKit/Source/platform/testing/RunAllTests.cpp -> //cc/blink/web_compositor_support_impl.h

 //third_party/WebKit/Source/platform/weborigin/KURL.h -> //url/third_party/mozilla/url_parse.h

 //third_party/WebKit/Source/platform/weborigin/OriginAccessEntry.cpp -> //url/third_party/mozilla/url_parse.h

 //third_party/WebKit/Source/platform/weborigin/KURL.h -> //url/url_canon.h

 //third_party/WebKit/Source/platform/weborigin/KURL.cpp -> //url/url_util.h

 //third_party/WebKit/Source/platform/weborigin/OriginAccessEntry.cpp -> //url/url_canon.h

 //third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp -> //v8/include/v8.h

 //third_party/WebKit/Source/web/WebTestingSupport.cpp -> //v8/include/v8.h

 //third_party/WebKit/Source/web/tests/ListenerLeakTest.cpp -> //v8/include/v8-profiler.h

 //third_party/WebKit/Source/web/tests/RunAllTests.cpp -> //content/test/blink_test_environment.h

 //third_party/WebKit/Source/web/tests/WebUnitTests.cpp -> //base/bind.h

 //third_party/WebKit/Source/web/tests/WebUnitTests.cpp -> //base/message_loop/message_loop.h

 //third_party/WebKit/Source/web/tests/WebUnitTests.cpp -> //base/run_loop.h

 //third_party/WebKit/Source/web/tests/WebUnitTests.cpp -> //base/test/launcher/unit_test_launcher.h

 //third_party/WebKit/Source/web/tests/WebUnitTests.cpp -> //base/test/test_suite.h

 //third_party/WebKit/Source/web/tests/ListenerLeakTest.cpp -> //v8/include/v8.h

 //third_party/WebKit/Source/wtf/testing/RunAllTests.cpp -> //base/test/test_suite.h

I propose the following patch to be integrated as I think it's more likely that it will find bugs in dependencies than that it will cause any trouble, and it would be great for external users of gn.

diff --git a/tools/gn/c_include_iterator.cc b/tools/gn/c_include_iterator.cc
index cc3cb35..28654a7 100644
--- a/tools/gn/c_include_iterator.cc
+++ b/tools/gn/c_include_iterator.cc
@@ -134,8 +134,7 @@ bool CIncludeIterator::GetNextIncludeString(base::StringPiece* out,
     base::StringPiece include_contents;
     int begin_char;
     IncludeType type = ExtractInclude(line, &include_contents, &begin_char);
-    if (type == INCLUDE_USER && !HasNoCheckAnnotation(line)) {
-      // Only count user includes for now.
+    if (type != INCLUDE_NONE && !HasNoCheckAnnotation(line)) {
       *out = include_contents;
       *location = LocationRange(
           Location(input_file_,

Thanks,

NGG

[Thiago Farina]

On Sun, Jan 10, 2016 at 2:24 PM, Gergely Nagy <n...@tresorit.com> wrote:

Hi!

We are trying to use gn for an external project that uses "#include <rel_path_from_root_source/file_name.h>" style includes and currently gn check only looks at includes that use quotes.

I patched gn to check system includes as well, and gn check still works and says OK for the whole chromium repo.

I counted all the include lines in the chromium repo from gn check, and it checks dependencies for 73381 quote includes and 54 system includes.

Here is the list of the 54 includes that are included as system includes and therefore not currently checked in gn.

 //components/feedback/anonymizer_tool.cc -> //base/strings/string_number_conversions.h

 //components/feedback/anonymizer_tool.cc -> //base/strings/string_util.h

 //components/feedback/anonymizer_tool.cc -> //base/strings/stringprintf.h

 //components/feedback/anonymizer_tool.h -> //base/macros.h

 //components/password_manager/core/browser/password_ui_utils.cc -> //components/password_manager/core/browser/password_ui_utils.h

 //components/update_client/utils.h -> //base/memory/scoped_ptr.h

Thanks! I'm doing a pass to fix those!

--

Thiago Farina

[Thiago Farina]

On Sun, Jan 10, 2016 at 2:24 PM, Gergely Nagy <n...@tresorit.com> wrote:

Hi!

 //third_party/WebKit/Source/core/dom/URLSearchParams.h -> //base/gtest_prod_util.h

I think the WebKit/Blink ones were using <>, because back then third_party/WebKit was a separate project in a separate repo, developed independently of Chromium. Now that it is merged into Chromium tree, we might be able to just use "" for these includes.

I will ask some Blink guys about this.

Thanks,

--

Thiago Farina

[Dirk Pranke]

Sorry for the delay in getting back to you about this ... this looks like a reasonable

change to make to me; did you end up posting this as a CL, or would you like

a committer to do so for you?

-- Dirk

[Gergely Nagy]

I just created a CL at https://codereview.chromium.org/1631873003

I think I followed the guidelines for external contributors.

NGG

---

From: dpr...@google.com <dpr...@google.com> on behalf of Dirk Pranke <dpr...@chromium.org>
Sent: Tuesday, January 26, 2016 3:09 AM
To: Gergely Nagy
Cc: gn-...@chromium.org
Subject: Re: gn check could check system includes too