gn check could check system includes too

30 views
Skip to first unread message

Gergely Nagy

unread,
Jan 10, 2016, 11:24:31 AM1/10/16
to gn-...@chromium.org

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

unread,
Jan 10, 2016, 2:23:01 PM1/10/16
to Gergely Nagy, gn-...@chromium.org
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

unread,
Jan 13, 2016, 8:40:32 AM1/13/16
to Gergely Nagy, gn-...@chromium.org
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

unread,
Jan 25, 2016, 9:10:10 PM1/25/16
to Gergely Nagy, gn-...@chromium.org
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

unread,
Jan 26, 2016, 7:34:57 AM1/26/16
to Dirk Pranke, gn-...@chromium.org

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
 
Reply all
Reply to author
Forward
0 new messages