build: run linux clang-tidy as remotely cached siso build steps (#53447)

* build: run linux clang-tidy as remotely cached siso build steps

Prototype for the linux-x64 leg. Instead of run-clang-tidy.ts walking
shell/ on the runner's own cores, generate one ninja step per translation
unit and let siso treat it like a compile: include-scan the inputs, look
the action up in the RBE cache, and execute misses on the worker pool.

- script/gen-clang-tidy-ninja.py writes out/<dir>/clang_tidy.ninja from
  compile_commands.json: `run-clang-tidy-action.sh <clang++> <flags> -c
  <src> -o <stamp>` per shell/ TU, with a gcc depfile so ninja/siso track
  header deps.
- build/run-clang-tidy-action.sh runs clang-tidy with
  --warnings-as-errors=* (so only clean results are cached), then
  `clang -M` for the depfile, then touches the stamp.
- build/siso/main.star is build-tools' siso entry file plus an
  `electron/clang-tidy` step rule (remote, clang-tidy/.clang-tidy files as
  inputs); the tidy job points ELECTRON_BUILD_TOOLS_MAIN_STAR at it.
- The linux leg of the clang-tidy segment now does `e init --remote-build
  siso`, gn gen with compile commands for //electron:*, generates the
  ninja file and runs `e build -f clang_tidy.ninja electron_clang_tidy`.
  It gets ELECTRON_RBE_JWT for that. mac and win legs are unchanged.

Measured against out/Testing on rbe.notgoma.com: 343 TUs, cold run all
remote in 115 s wall (150 CPU-min of work, bounded by the slowest TU),
fully cached rerun 2.3 s, and a planted finding fails the step with the
diagnostic and is not cached.

* build: tidy up the clang-tidy step generator and wrapper

* build: run macOS and Windows clang-tidy through siso too

- gen-clang-tidy-ninja.py understands clang-cl commands (drops /c, /Fo,
  /Fd, /showIncludes) and emits `deps = msvc` for them, which is also what
  tells siso to parse the command cl-style when scanning for #includes.
- run-clang-tidy-action.sh: for clang-cl, run clang-tidy in cl driver mode
  and report headers via /showIncludes; when the host toolchain isn't
  runnable (macOS binaries on the Linux workers) use the Linux one that
  clang_remote_wrapper stages under Release+Asserts_linux.
- build/siso/main.star: off Linux, run the tidy step through
  clang_remote_wrapper with the Linux clang and clang-tidy as inputs, like
  the clang rules.
- The clang-tidy segment uses the siso path on every platform; on macOS it
  first generates the reclient configs (which fetches the Linux clang) and
  downloads the Linux clang-tidy package next to it.

* build: hand clang-tidy a one-entry compile db so clang-cl keeps a relative source path

clang-tidy appends an absolute source path to `-- <flags>` commands, and on the RBE workers that path starts with /worker, which the cl driver parses as a /w option ('no input files'). A compilation database entry keeps the command as written.

* build: upload siso metrics from the clang-tidy job

* build: install the macOS siso binary in the clang-tidy job

* build: one clang-tidy step per object, not per source file

A source built by several targets (electron_app and the helper app on macOS) has several compile commands with different flags; picking one by source path was order-dependent and cost two cache misses per run. Key steps on the object path instead.

* build: sync build/siso/main.star with the current build-tools pin

* build: use build-tools' clang-tidy siso rule

electron/build-tools#896 carries the electron/clang-tidy rule, so bump the build-tools pin to it and drop the in-repo copy of build-tools' main.star and the ELECTRON_BUILD_TOOLS_MAIN_STAR override. The pin bump also picks up electron/build-tools#895 (retry the initial depot_tools clone).

* build: set clang-tidy's header filter in .clang-tidy

Both clang-tidy entry points passed their own --header-filter. Set HeaderFilterRegex in .clang-tidy instead so they can't diverge. This clang-tidy reports header diagnostics when no filter is set, so the flags and the config change together.

* build: run gn gen from gen-clang-tidy-ninja.py

The steps are only right if compile_commands.json matches the out dir, so the generator now regenerates it instead of trusting an old one. Also say that the generator and the wrapper are CI plumbing, and what a local run needs.

* build: bump build-tools for nested .clang-tidy configs

Picks up electron/build-tools#897, which sends any .clang-tidy under electron/shell to the RBE worker and into the cache key.

.clang-tidy

@@ -8,5 +8,8 @@ Checks: 'clang-analyzer-core.*,
          clang-analyzer-unix.*,
          -clang-analyzer-core.CallAndMessage,
          -clang-analyzer-security.PointerSub'
+# Only report diagnostics in the file being checked, not in the headers it
+# includes. Chromium's headers would otherwise flood every run.
+HeaderFilterRegex: ''
 InheritParentConfig: true
 ...

.github/actions/install-build-tools/action.yml

@@ -16,7 +16,7 @@ runs:
         git config --global core.preloadindex true
         git config --global core.longpaths true
       fi
-      export BUILD_TOOLS_SHA=6bd58bb51aedf9029ed495e79b32dcbf63c181da
+      export BUILD_TOOLS_SHA=5a3a3bf9ad0bea0bd7cf24121b9b636d6cb5c9ea
       # Both fetch over the network (the first `e` command clones depot_tools),
       # so retry them on transient failures.
       RETRY="$PWD/src/electron/script/actions/retry.sh"

.github/workflows/pipeline-electron-build-and-tidy-and-test-and-nan.yml

@@ -135,6 +135,8 @@ jobs:
       clang-tidy-container: ${{ inputs.clang-tidy-container }}
       target-platform: ${{ inputs.target-platform }}
       target-arch: ${{ inputs.target-arch }}
+    secrets:
+      ELECTRON_RBE_JWT: ${{ secrets.ELECTRON_RBE_JWT }}
   test:
     uses: ./.github/workflows/pipeline-segment-electron-test.yml
     permissions:

.github/workflows/pipeline-electron-build-and-tidy-and-test.yml

@@ -142,6 +142,8 @@ jobs:
       clang-tidy-container: ${{ inputs.clang-tidy-container }}
       target-platform: ${{ inputs.target-platform }}
       target-arch: ${{ inputs.target-arch }}
+    secrets:
+      ELECTRON_RBE_JWT: ${{ secrets.ELECTRON_RBE_JWT }}
   test:
     uses: ./.github/workflows/pipeline-segment-electron-test.yml
     permissions:

.github/workflows/pipeline-segment-electron-clang-tidy.yml

@@ -20,6 +20,9 @@ on:
         description: 'JSON container information for aks runs-on'
         required: false
         default: '{"image":null}'
+    secrets:
+      ELECTRON_RBE_JWT:
+        required: false
 
 permissions: {}
 
@@ -41,6 +44,7 @@ jobs:
       contents: read
     container: ${{ fromJSON(inputs.clang-tidy-container) }} # zizmor: ignore[unpinned-images] the caller passes ghcr.io/electron/build tagged by the content hash computed in build.yml; there is no static digest to pin
     env:
+      ELECTRON_RBE_JWT: ${{ secrets.ELECTRON_RBE_JWT }}
       BUILD_TYPE: ${{ inputs.target-platform == 'macos' && 'darwin' || inputs.target-platform }}
       TARGET_ARCH: ${{ inputs.target-arch }}
       TARGET_PLATFORM: ${{ inputs.target-platform }}
@@ -142,41 +146,69 @@ jobs:
     - name: Add Clang problem matcher
       shell: bash
       run: echo "::add-matcher::src/electron/.github/problem-matchers/clang.json"
-    - name: Run Clang-Tidy
-      env:
-        # macOS runners have fewer CPU cores, so use fewer jobs for better results
-        CLANG_TIDY_JOBS: ${{ case(inputs.target-platform == 'macos', 5, 8) }}
+    # The RBE workers are Linux; on macOS stage the Linux clang and clang-tidy
+    # they will run (clang_remote_wrapper + Release+Asserts_linux, the same
+    # arrangement fix-sync sets up for the build jobs' compiles).
+    # The src cache is produced on Linux, so host tools fetched from CIPD are
+    # Linux binaries; swap in the macOS ones this job runs (as fix-sync does
+    # for the build job).
+    - name: Install siso (macOS)
+      if: ${{ inputs.target-platform == 'macos' }}
+      uses: ./src/electron/.github/actions/cipd-install
+      with:
+        dependency: siso
+        deps-file: src/DEPS
+        installation-dir: src/third_party/siso/cipd
+        target-platform: ${{ inputs.target-platform }}
+        package: build/siso/${platform}
+    - name: Install reclient (macOS)
+      if: ${{ inputs.target-platform == 'macos' }}
+      uses: ./src/electron/.github/actions/cipd-install
+      with:
+        dependency: reclient
+        deps-file: src/DEPS
+        installation-dir: src/buildtools/reclient
+        target-platform: ${{ inputs.target-platform }}
+        package: infra/rbe/client/${platform}
+    - name: Stage Linux clang-tidy for RBE (macOS)
+      if: ${{ inputs.target-platform == 'macos' }}
       run: |
-        e init -f --root=$(pwd) --out=${ELECTRON_OUT_DIR} testing --target-cpu ${TARGET_ARCH} --remote-build none
-
-        # For macOS use_remoteexec=false will cause GN errors, so even though we're doing no remote build, set it
-        export GN_EXTRA_ARGS="use_remoteexec=true target_cpu=\"${TARGET_ARCH}\""
+        python3 src/buildtools/reclient_cfgs/configure_reclient_cfgs.py --rbe_instance "projects/rbe-chrome-untrusted/instances/default_instance" --reproxy_cfg_template reproxy.cfg.template --rewrapper_cfg_project "" --skip_remoteexec_cfg_fetch
+        python3 src/tools/clang/scripts/update.py --package clang-tidy --host-os linux --output-dir src/third_party/llvm-build/Release+Asserts_linux
+    # clang-tidy runs as siso build steps (script/gen-clang-tidy-ninja.py): one
+    # remotely executed, remotely cached action per translation unit.
+    - name: Run Clang-Tidy
+      run: |
+        e init -f --root=$(pwd) --out=${ELECTRON_OUT_DIR} testing --target-cpu ${TARGET_ARCH} --remote-build siso
+        export GN_EXTRA_ARGS="target_cpu=\"${TARGET_ARCH}\""
         if [ "$TARGET_PLATFORM" = "win" ]; then
           export GN_EXTRA_ARGS="$GN_EXTRA_ARGS use_v8_context_snapshot=true target_os=\"win\""
         fi
-
         e build --gen=only
 
-        # Copy macOS framework headers so clang-tidy can find them via -F.
-        # This must happen after e build since e init -f may
-        # recreate the output directory.
+        # Squirrel and friends are included via -F framework paths that only
+        # exist in the out dir after a build; recreate their Headers dirs.
         if [ "$TARGET_PLATFORM" = "macos" ]; then
           OUT=src/out/${ELECTRON_OUT_DIR}
           SQRL=src/third_party/squirrel.mac
-
           mkdir -p ${OUT}/{ReactiveObjC,Squirrel,Mantle}.framework/Headers
-
-          cp ${SQRL}/vendor/ReactiveObjC/ReactiveObjC/*.h ${OUT}/ReactiveObjC.framework/Headers/
-          cp ${SQRL}/vendor/ReactiveObjC/ReactiveObjC/extobjc/*.h ${OUT}/ReactiveObjC.framework/Headers/
-
+          cp ${SQRL}/vendor/ReactiveObjC/ReactiveObjC/*.h ${SQRL}/vendor/ReactiveObjC/ReactiveObjC/extobjc/*.h ${OUT}/ReactiveObjC.framework/Headers/
           cp ${SQRL}/Squirrel/*.h ${OUT}/Squirrel.framework/Headers/
-
-          cp ${SQRL}/vendor/Mantle/Mantle/include/*.h ${OUT}/Mantle.framework/Headers/
-          cp ${SQRL}/vendor/Mantle/Mantle/extobjc/include/*.h ${OUT}/Mantle.framework/Headers/
+          cp ${SQRL}/vendor/Mantle/Mantle/include/*.h ${SQRL}/vendor/Mantle/Mantle/extobjc/include/*.h ${OUT}/Mantle.framework/Headers/
         fi
 
+        e d python3 src/electron/script/gen-clang-tidy-ninja.py src/out/${ELECTRON_OUT_DIR}
         cd src/electron
-        node script/yarn.js lint:clang-tidy --jobs ${CLANG_TIDY_JOBS} --out-dir ../out/${ELECTRON_OUT_DIR}
+        e build --gen=off --target electron_clang_tidy -f clang_tidy.ninja -batch -k 0
+    - name: Upload siso metrics
+      if: always() && !cancelled()
+      uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a #v7.0.1
+      with:
+        name: clang_tidy_siso_${{ env.ARTIFACT_KEY }}
+        path: |
+          src/out/Default/siso_metrics.json
+          src/out/Default/siso_output
+        if-no-files-found: ignore
     - name: Remove Clang problem matcher
       shell: bash
       run: echo "::remove-matcher owner=clang::"

build/run-clang-tidy-action.sh

@@ -0,0 +1,65 @@
+#!/bin/bash
+#
+# Runs clang-tidy on one translation unit as a build step; the steps are
+# generated by script/gen-clang-tidy-ninja.py. This is CI plumbing: only the
+# generated clang_tidy.ninja should run it. Invoked as
+#
+#   run-clang-tidy-action.sh <clang++|clang-cl> <compile flags...> -c <source> -o <output>
+#
+# i.e. an ordinary compile command with this script in front, which is what
+# lets siso include-scan, cache and remotely execute the step like a compile.
+#
+# Fails if clang-tidy reports anything, so only clean runs get cached. On
+# success writes <output> (an empty stamp) plus the step's header dependencies:
+# a depfile <output>.d for clang++, /showIncludes output for clang-cl.
+
+set -euo pipefail
+
+clang=$1
+shift
+
+# A macOS host's toolchain can't run on the (Linux) RBE workers. There,
+# clang_remote_wrapper has run before us and staged a Linux clang and
+# clang-tidy under Release+Asserts_linux; see the electron/clang-tidy rule in
+# build-tools' tools/main.star.
+if ! "$clang" --version >/dev/null 2>&1; then
+  clang=${clang/Release+Asserts/Release+Asserts_linux}
+fi
+clang_tidy=$(dirname "$clang")/clang-tidy
+
+flags=()
+while [[ $# -gt 0 ]]; do
+  case $1 in
+    -c) source=$2; shift ;;
+    -o) output=$2; shift ;;
+    *) flags+=("$1") ;;
+  esac
+  shift
+done
+
+json_list() {
+  local item items=()
+  for item in "$@"; do
+    item=${item//\\/\\\\}
+    items+=("\"${item//\"/\\\"}\"")
+  done
+  local IFS=,
+  echo "[${items[*]}]"
+}
+
+# Give clang-tidy the command through a one-entry compilation database rather
+# than `-- <flags>`: it then uses the command as written (compiler name, and
+# the source path kept relative) instead of appending an absolute source path,
+# which clang-cl would parse as an option when it starts with e.g. /U or /w.
+mkdir -p "$output.compdb"
+trap 'rm -rf "$output.compdb"' EXIT
+echo "[{\"directory\": \"$PWD\", \"file\": \"$source\", \"arguments\": $(json_list "$clang" "${flags[@]}" "$source")}]" \
+    > "$output.compdb/compile_commands.json"
+"$clang_tidy" -p "$output.compdb" --quiet --warnings-as-errors='*' "$source"
+
+if [[ $(basename "$clang") == clang-cl* ]]; then
+  "$clang" "${flags[@]}" /showIncludes:user /E "$source" >/dev/null
+else
+  "$clang" "${flags[@]}" -w -M -MF "$output.d" -MT "$output" "$source"
+fi
+touch "$output"

script/gen-clang-tidy-ninja.py

@@ -0,0 +1,143 @@
+#!/usr/bin/env python3
+"""Generate <out_dir>/clang_tidy.ninja: one clang-tidy build step per Electron
+translation unit.
+
+Each step's command line is an ordinary compile command with
+build/run-clang-tidy-action.sh in front of it. That shape is what lets siso
+scan the step's #includes, look it up in the RBE action cache and run it
+remotely (see the electron/clang-tidy rule in build-tools' tools/main.star).
+
+This is CI plumbing, run by the clang-tidy job. To run clang-tidy locally use
+`yarn lint:clang-tidy`. If you do run this yourself, the out dir must already
+be built (the steps need its generated headers), and it has to run through
+`e d` so that gn is on PATH:
+
+  e d python3 script/gen-clang-tidy-ninja.py ../out/X
+  e build -f clang_tidy.ninja --target electron_clang_tidy
+"""
+
+import collections
+import json
+import os
+import shlex
+import shutil
+import subprocess
+import sys
+
+ELECTRON_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+SRC_DIR = os.path.dirname(ELECTRON_DIR)
+SHELL_DIR = os.path.join(ELECTRON_DIR, 'shell')
+
+# Keep in sync with script/run-clang-tidy.ts.
+SKIPPED_SOURCES = {
+    # Host tool built in v8_snapshot_toolchain; its generated buildflag headers
+    # don't exist in the default toolchain's gen dir.
+    'shell/common/electron_natives_codecache_main.cc',
+}
+if os.environ.get('CI'):
+  # TODO(dsanders11): clang-tidy compilation errors in CI that don't reproduce
+  # locally.
+  SKIPPED_SOURCES.add('shell/renderer/electron_smooth_round_rect.cc')
+
+Step = collections.namedtuple('Step', ['source', 'output', 'compiler', 'flags'])
+
+
+def parse_compile_command(command, source):
+  """Splits a clang++ or clang-cl compile command into (compiler, object, flags).
+  flags omits the source file and the output/depfile/pdb arguments, since the
+  wrapper supplies its own."""
+  compiler, *args = shlex.split(command)
+  flags = []
+  obj = None
+  args = iter(args)
+  for arg in args:
+    if arg == '-o':
+      obj = next(args)
+    elif arg.startswith('/Fo'):
+      obj = arg[len('/Fo'):]
+    elif arg == '-MF':
+      next(args)
+    elif arg in ('-c', '/c', '-MMD', source) or arg.startswith(('/Fd', '/showIncludes')):
+      pass
+    else:
+      flags.append(arg)
+  return compiler, obj, flags
+
+
+def tidy_steps(out_dir):
+  with open(os.path.join(out_dir, 'compile_commands.json'), encoding='utf-8') as f:
+    compile_commands = json.load(f)
+
+  # A file built by several targets (with different flags) gets a step per
+  # target, told apart by object path like the compiles themselves.
+  steps = []
+  for entry in compile_commands:
+    path = os.path.normpath(os.path.join(entry['directory'], entry['file']))
+    if not path.startswith(SHELL_DIR + os.sep) or not path.endswith(('.cc', '.mm')):
+      continue
+    if os.path.relpath(path, ELECTRON_DIR) in SKIPPED_SOURCES:
+      continue
+    compiler, obj, flags = parse_compile_command(entry['command'], entry['file'])
+    steps.append(Step(
+        source=os.path.relpath(path, out_dir),
+        output=f'{obj}.tidy',
+        compiler=compiler,
+        flags=tuple(flags),
+    ))
+  return sorted(set(steps), key=lambda step: step.output)
+
+
+def ninja_escape(text):
+  return text.replace('$', '$$')
+
+
+def write_ninja(out_dir, steps):
+  wrapper = os.path.join(os.path.relpath(ELECTRON_DIR, out_dir), 'build', 'run-clang-tidy-action.sh')
+  # How ninja/siso learn each step's header dependencies. This also tells siso
+  # which flag style to expect when it scans the command for #includes.
+  if os.path.basename(steps[0].compiler).startswith('clang-cl'):
+    deps = 'deps = msvc'
+  else:
+    deps = 'depfile = $out.d\n  deps = gcc'
+
+  with open(os.path.join(out_dir, 'clang_tidy.ninja'), 'w', encoding='utf-8') as ninja:
+    ninja.write(f'''# Generated by electron/script/gen-clang-tidy-ninja.py
+
+rule clang_tidy
+  command = {wrapper} $compiler $flags -c $in -o $out
+  description = CLANG-TIDY $in
+  {deps}
+
+''')
+    for step in steps:
+      flags = ' '.join(shlex.quote(flag) for flag in step.flags)
+      ninja.write(f'build {step.output}: clang_tidy {step.source}\n')
+      ninja.write(f'  compiler = {ninja_escape(step.compiler)}\n')
+      ninja.write(f'  flags = {ninja_escape(flags)}\n')
+
+    outputs = ' $\n    '.join(step.output for step in steps)
+    ninja.write(f'\nbuild electron_clang_tidy: phony {outputs}\n')
+    ninja.write('default electron_clang_tidy\n')
+
+
+def main():
+  if len(sys.argv) != 2:
+    sys.exit(__doc__)
+  out_dir = os.path.abspath(sys.argv[1])
+
+  # Regenerate compile_commands.json here rather than trusting an old one, so
+  # the steps always carry the out dir's current flags.
+  gn = shutil.which('gn')
+  if not gn:
+    sys.exit('gn is not on PATH; run this through `e d`')
+  subprocess.check_call([gn, 'gen', out_dir, '--add-export-compile-commands=//electron:*'], cwd=SRC_DIR)
+
+  steps = tidy_steps(out_dir)
+  if not steps:
+    sys.exit('No electron/shell translation units in compile_commands.json')
+  write_ninja(out_dir, steps)
+  print(f'Wrote {len(steps)} clang-tidy steps to {os.path.join(sys.argv[1], "clang_tidy.ninja")}')
+
+
+if __name__ == '__main__':
+  main()

script/run-clang-tidy.ts

@@ -62,7 +62,8 @@ async function runClangTidy(
   fix: boolean = false
 ): Promise<boolean> {
   const cmd = path.resolve(LLVM_BIN, 'clang-tidy');
-  const args = [`-p=${outDir}`, "-header-filter=''"];
+  // The header filter is HeaderFilterRegex in .clang-tidy.
+  const args = [`-p=${outDir}`];
 
   if (!process.env.CI) args.push('--use-color');
   if (fix) args.push('--fix');