chore: add GN linting (electron#14678)

* chore: add GN linter * chore: fix GN lint errors * try some crazy bash to get a gn exe * base64 on linux is different * cloning build_tools doesn't download GN * download_from_google_storage needs depot_tools in the path * fixup! chore: add GN linter
daverodgman · Oct 3, 2018 · 14fc6f3 · 14fc6f3
1 parent 5efb0fd
commit 14fc6f3
Show file tree

Hide file tree

Showing 9 changed files with 88 additions and 45 deletions.
diff --git a/.vsts/lint.yml b/.vsts/lint.yml
@@ -3,13 +3,29 @@ pool:
 
 steps:
 - bash: |
+    # "depot_tools" has to be checkout into "//third_party/depot_tools" so pylint.py can a "pylintrc" file.
     git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git "${AGENT_BUILDDIRECTORY}/third_party/depot_tools"
-    echo "##vso[task.setvariable variable=PATH]$PATH:${AGENT_BUILDDIRECTORY}/depot_tools"
+    echo "##vso[task.setvariable variable=PATH]$PATH:${AGENT_BUILDDIRECTORY}/third_party/depot_tools"
   displayName: Setup Depot Tools
 
 - bash: |
-    export PATH="$PATH:${AGENT_BUILDDIRECTORY}/third_party/depot_tools"
-    echo "##vso[task.setvariable variable=PATH]$PATH"
+    chromium_revision="$(grep -A1 chromium_version DEPS | tr -d '\n' | cut -d\' -f4)"
+    buildtools_revision="$(curl -sL "https://chromium.googlesource.com/chromium/src/+/${chromium_revision}/DEPS?format=TEXT" | base64 -d | grep buildtools_revision -A1 | tr -d '\n' | cut -d\' -f4)"
+
+    git clone https://chromium.googlesource.com/chromium/buildtools "${AGENT_TEMPDIRECTORY}/buildtools"
+    (cd "${AGENT_TEMPDIRECTORY}/buildtools" && git checkout "$buildtools_revision")
+    echo "##vso[task.setvariable variable=CHROMIUM_BUILDTOOLS_PATH]$AGENT_TEMPDIRECTORY/buildtools"
+
+    download_from_google_storage --bucket chromium-gn -s "${AGENT_TEMPDIRECTORY}/buildtools/linux64/gn.sha1"
+  displayName: Download gn binary
+
+- bash: |
+    # gn.py tries to find a gclient root folder starting from the current dir.
+    # When it fails and returns "None" path, the whole script fails. Let's "fix" it.
+    touch .gclient
+    # Another option would be to checkout "buildtools" inside the Electron checkout,
+    # but then we would lint its contents (at least gn format), and it doesn't pass it.
+
     npm install
     npm run lint
   displayName: Run Lint
diff --git a/brightray/BUILD.gn b/brightray/BUILD.gn
@@ -47,15 +47,21 @@ static_library("brightray") {
     "browser/inspectable_web_contents.cc",
     "browser/inspectable_web_contents.h",
     "browser/inspectable_web_contents_delegate.h",
-    "browser/inspectable_web_contents_view_delegate.cc",
-    "browser/inspectable_web_contents_view_delegate.h",
     "browser/inspectable_web_contents_impl.cc",
     "browser/inspectable_web_contents_impl.h",
     "browser/inspectable_web_contents_view.h",
+    "browser/inspectable_web_contents_view_delegate.cc",
+    "browser/inspectable_web_contents_view_delegate.h",
     "browser/inspectable_web_contents_view_mac.h",
     "browser/inspectable_web_contents_view_mac.mm",
     "browser/io_thread.cc",
     "browser/io_thread.h",
+    "browser/linux/libnotify_loader.cc",
+    "browser/linux/libnotify_loader.h",
+    "browser/linux/libnotify_notification.cc",
+    "browser/linux/libnotify_notification.h",
+    "browser/linux/notification_presenter_linux.cc",
+    "browser/linux/notification_presenter_linux.h",
     "browser/mac/bry_inspectable_web_contents_view.h",
     "browser/mac/bry_inspectable_web_contents_view.mm",
     "browser/mac/cocoa_notification.h",
@@ -76,19 +82,21 @@ static_library("brightray") {
     "browser/net/require_ct_delegate.h",
     "browser/net_log.cc",
     "browser/net_log.h",
+    "browser/notification.cc",
+    "browser/notification.h",
     "browser/notification_delegate.h",
     "browser/notification_presenter.cc",
     "browser/notification_presenter.h",
-    "browser/notification.cc",
-    "browser/notification.h",
     "browser/platform_notification_service.cc",
     "browser/platform_notification_service.h",
-    "browser/linux/libnotify_loader.h",
-    "browser/linux/libnotify_loader.cc",
-    "browser/linux/libnotify_notification.h",
-    "browser/linux/libnotify_notification.cc",
-    "browser/linux/notification_presenter_linux.h",
-    "browser/linux/notification_presenter_linux.cc",
+    "browser/url_request_context_getter.cc",
+    "browser/url_request_context_getter.h",
+    "browser/views/inspectable_web_contents_view_views.cc",
+    "browser/views/inspectable_web_contents_view_views.h",
+    "browser/views/views_delegate.cc",
+    "browser/views/views_delegate.h",
+    "browser/web_ui_controller_factory.cc",
+    "browser/web_ui_controller_factory.h",
     "browser/win/notification_presenter_win.cc",
     "browser/win/notification_presenter_win.h",
     "browser/win/notification_presenter_win7.cc",
@@ -98,26 +106,18 @@ static_library("brightray") {
     "browser/win/win32_desktop_notifications/common.h",
     "browser/win/win32_desktop_notifications/desktop_notification_controller.cc",
     "browser/win/win32_desktop_notifications/desktop_notification_controller.h",
-    "browser/win/win32_desktop_notifications/toast_uia.cc",
-    "browser/win/win32_desktop_notifications/toast_uia.h",
     "browser/win/win32_desktop_notifications/toast.cc",
     "browser/win/win32_desktop_notifications/toast.h",
+    "browser/win/win32_desktop_notifications/toast_uia.cc",
+    "browser/win/win32_desktop_notifications/toast_uia.h",
     "browser/win/win32_notification.cc",
     "browser/win/win32_notification.h",
     "browser/win/windows_toast_notification.cc",
     "browser/win/windows_toast_notification.h",
-    "browser/url_request_context_getter.cc",
-    "browser/url_request_context_getter.h",
-    "browser/views/inspectable_web_contents_view_views.h",
-    "browser/views/inspectable_web_contents_view_views.cc",
-    "browser/views/views_delegate.cc",
-    "browser/views/views_delegate.h",
-    "browser/web_ui_controller_factory.cc",
-    "browser/web_ui_controller_factory.h",
     "browser/zoom_level_delegate.cc",
     "browser/zoom_level_delegate.h",
-    "common/application_info.h",
     "common/application_info.cc",
+    "common/application_info.h",
     "common/application_info_mac.mm",
     "common/application_info_win.cc",
     "common/content_client.cc",
@@ -127,10 +127,10 @@ static_library("brightray") {
     "common/main_delegate.cc",
     "common/main_delegate.h",
     "common/main_delegate_mac.mm",
+    "common/platform_util.h",
+    "common/platform_util_linux.cc",
     "common/switches.cc",
     "common/switches.h",
-    "common/platform_util_linux.cc",
-    "common/platform_util.h",
   ]
   set_sources_assignment_filter(sources_assignment_filter)
 

diff --git a/build/config/BUILD.gn b/build/config/BUILD.gn
@@ -17,6 +17,7 @@ config("build_time_executable") {
     }
   }
 }
+
 # For MAS build, we force defining "MAS_BUILD".
 config("mas_build") {
   if (is_mas_build) {

diff --git a/build/npm.gni b/build/npm.gni
@@ -1,16 +1,22 @@
 template("npm_action") {
   assert(defined(invoker.script),
          "Need script name to run (must be defined in package.json)")
-  assert(defined(invoker.args),
-         "Need script argumets")
+  assert(defined(invoker.args), "Need script argumets")
 
   action(target_name) {
-    forward_variables_from(invoker, ["deps", "public_deps", "sources", "inputs", "outputs"])
+    forward_variables_from(invoker,
+                           [
+                             "deps",
+                             "public_deps",
+                             "sources",
+                             "inputs",
+                             "outputs",
+                           ])
     script = "//electron/build/npm-run.py"
     args = [
-      "--silent",
-      invoker.script,
-      "--"
-    ] + invoker.args
+             "--silent",
+             invoker.script,
+             "--",
+           ] + invoker.args
   }
 }
diff --git a/electron_paks.gni b/electron_paks.gni
@@ -17,9 +17,9 @@ template("electron_repack_percent") {
 
     # All sources should also have deps for completeness.
     sources = [
-      "$root_gen_dir/third_party/blink/public/resources/blink_scaled_resources_${percent}_percent.pak",
       "$root_gen_dir/components/components_resources_${percent}_percent.pak",
       "$root_gen_dir/content/app/resources/content_resources_${percent}_percent.pak",
+      "$root_gen_dir/third_party/blink/public/resources/blink_scaled_resources_${percent}_percent.pak",
       "$root_gen_dir/ui/resources/ui_resources_${percent}_percent.pak",
     ]
 
@@ -53,12 +53,12 @@ template("electron_extra_paks") {
                            ])
     output = "${invoker.output_dir}/resources.pak"
     sources = [
-      "$root_gen_dir/third_party/blink/public/resources/blink_resources.pak",
       "$root_gen_dir/components/components_resources.pak",
       "$root_gen_dir/content/browser/tracing/tracing_resources.pak",
       "$root_gen_dir/content/content_resources.pak",
       "$root_gen_dir/mojo/public/js/mojo_bindings_resources.pak",
       "$root_gen_dir/net/net_resources.pak",
+      "$root_gen_dir/third_party/blink/public/resources/blink_resources.pak",
     ]
     deps = [
       "//components/resources",

diff --git a/filenames.gni b/filenames.gni
@@ -644,9 +644,7 @@ filenames = {
     "chromium_src/library_loaders/libspeechd.h",
   ]
 
-  lib_sources_linux = [
-    "chromium_src/chrome/browser/icon_loader_auralinux.cc",
-  ]
+  lib_sources_linux = [ "chromium_src/chrome/browser/icon_loader_auralinux.cc" ]
   lib_sources_nss = [
     "chromium_src/chrome/browser/certificate_manager_model.cc",
     "chromium_src/chrome/browser/certificate_manager_model.h",
@@ -672,7 +670,5 @@ filenames = {
     "atom/app/atom_library_main.mm",
   ]
 
-  login_helper_sources = [
-    "atom/app/atom_login_helper.mm",
-  ]
+  login_helper_sources = [ "atom/app/atom_login_helper.mm" ]
 }
diff --git a/native_mate/BUILD.gn b/native_mate/BUILD.gn
@@ -6,9 +6,9 @@ config("native_mate_config") {
 
 source_set("native_mate") {
   deps = [
-    "//third_party/electron_node:node_lib",
     "//base",
     "//net",
+    "//third_party/electron_node:node_lib",
     "//v8:v8_headers",
   ]
   public_configs = [ ":native_mate_config" ]

diff --git a/package.json b/package.json
@@ -49,6 +49,7 @@
     "lint:clang-format": "python script/run-clang-format.py -r -c atom/ chromium_src/ brightray/ || (echo \"\\nCode not formatted correctly.\" && exit 1)",
     "lint:cpp": "./script/lint.js --cc",
     "lint:py": "./script/lint.js --py",
+    "lint:gn": "./script/lint.js --gn",
     "lint:docs": "remark docs -qf && npm run lint:js-in-markdown && npm run create-typescript-definitions && npm run lint:docs-relative-links",
     "lint:docs-relative-links": "python ./script/check-relative-doc-links.py",
     "lint:js-in-markdown": "standard-markdown docs",

diff --git a/script/lint.js b/script/lint.js
@@ -86,12 +86,35 @@ const LINTERS = [ {
     if (opts.fix) args.unshift('--fix')
     spawnAndCheckExitCode(cmd, args, { cwd: SOURCE_ROOT })
   }
+}, {
+  key: 'gn',
+  roots: ['.'],
+  test: filename => filename.endsWith('.gn') || filename.endsWith('.gni'),
+  run: (opts, filenames) => {
+    const allOk = filenames.map(filename => {
+      const args = ['format', filename]
+      if (!opts.fix) args.push('--dry-run')
+      const result = childProcess.spawnSync('gn', args, { stdio: 'inherit' })
+      if (result.status === 0) {
+        return true
+      } else if (result.status === 2) {
+        console.log(`GN format errors in "${filename}". Run 'gn format "${filename}"' or rerun with --fix to fix them.`)
+        return false
+      } else {
+        console.log(`Error running 'gn format --dry-run "${filename}"': exit code ${result.status}`)
+        return false
+      }
+    }).every(x => x)
+    if (!allOk) {
+      process.exit(1)
+    }
+  }
 }]
 
 function parseCommandLine () {
   let help
   const opts = minimist(process.argv.slice(2), {
-    boolean: [ 'c++', 'javascript', 'python', 'help', 'changed', 'fix', 'verbose' ],
+    boolean: [ 'c++', 'javascript', 'python', 'gn', 'help', 'changed', 'fix', 'verbose' ],
     alias: { 'c++': ['cc', 'cpp', 'cxx'], javascript: ['js', 'es'], python: 'py', changed: 'c', help: 'h', verbose: 'v' },
     unknown: arg => { help = true }
   })
@@ -167,8 +190,8 @@ async function main () {
   const opts = parseCommandLine()
 
   // no mode specified? run 'em all
-  if (!opts['c++'] && !opts.javascript && !opts.python) {
-    opts['c++'] = opts.javascript = opts.python = true
+  if (!opts['c++'] && !opts.javascript && !opts.python && !opts.gn) {
+    opts['c++'] = opts.javascript = opts.python = opts.gn = true
   }
 
   const linters = LINTERS.filter(x => opts[x.key])
-Original file line number
+Diff line change
@@ Expand Up / @@ -17,6 +17,7 @@ config("build_time_executable") { @@
         }
       }
     }
     # For MAS build, we force defining "MAS_BUILD".
     config("mas_build") {
       if (is_mas_build) {
@@ Expand Down @@