diff --git a/.clang-tidy b/.clang-tidy index e6319275c0..a2aea63255 100644 --- a/.clang-tidy +++ b/.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 ... diff --git a/.github/actions/install-build-tools/action.yml b/.github/actions/install-build-tools/action.yml index 44e2016c01..388db9064d 100644 --- a/.github/actions/install-build-tools/action.yml +++ b/.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" diff --git a/.github/workflows/pipeline-electron-build-and-tidy-and-test-and-nan.yml b/.github/workflows/pipeline-electron-build-and-tidy-and-test-and-nan.yml index 3aa006b1b4..f2d2377691 100644 --- a/.github/workflows/pipeline-electron-build-and-tidy-and-test-and-nan.yml +++ b/.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: diff --git a/.github/workflows/pipeline-electron-build-and-tidy-and-test.yml b/.github/workflows/pipeline-electron-build-and-tidy-and-test.yml index 1eecbda1fb..8e92aaf503 100644 --- a/.github/workflows/pipeline-electron-build-and-tidy-and-test.yml +++ b/.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: diff --git a/.github/workflows/pipeline-segment-electron-clang-tidy.yml b/.github/workflows/pipeline-segment-electron-clang-tidy.yml index 4e34a1451b..05e41b1411 100644 --- a/.github/workflows/pipeline-segment-electron-clang-tidy.yml +++ b/.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::" diff --git a/build/run-clang-tidy-action.sh b/build/run-clang-tidy-action.sh new file mode 100755 index 0000000000..8abccf555d --- /dev/null +++ b/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 -c -o +# +# 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 (an empty stamp) plus the step's header dependencies: +# a depfile .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 `-- `: 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" diff --git a/script/gen-clang-tidy-ninja.py b/script/gen-clang-tidy-ninja.py new file mode 100755 index 0000000000..0ef695fe81 --- /dev/null +++ b/script/gen-clang-tidy-ninja.py @@ -0,0 +1,143 @@ +#!/usr/bin/env python3 +"""Generate /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() diff --git a/script/run-clang-tidy.ts b/script/run-clang-tidy.ts index 4a9942f0ea..8151fcdf2f 100644 --- a/script/run-clang-tidy.ts +++ b/script/run-clang-tidy.ts @@ -62,7 +62,8 @@ async function runClangTidy( fix: boolean = false ): Promise { 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');