From 12fe9df8d26485d1ba119b3a6925fc8ac75a4824 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 9 Oct 2024 13:25:45 -0400 Subject: [PATCH] Speed up linux and darwin CI (#35971) * Restrict clang-tidy * Move changed-files action forward * Update logic a bit to match tj-actions documentation * Restyled by autopep8 * Update logic to skip more tests for linux to make linux CI finish sooner on pull requests * Revert "Update logic to skip more tests for linux to make linux CI finish sooner on pull requests" This reverts commit a9bc97b2389a263d612de8667356a7262ac25b18. * Generate only for sanitizers * Make sure we checkout a bit of history and persist credentials * Fix up logic for tidy, should now be able to actually run based on github file diff * Restyle * Update .github/workflows/build.yaml Co-authored-by: Boris Zbarsky * Fix env * Code review update --------- Co-authored-by: Andrei Litvin Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky --- .github/workflows/build.yaml | 39 +++++++++++ scripts/run-clang-tidy-on-compile-commands.py | 69 ++++++++++++------- 2 files changed, 83 insertions(+), 25 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 4495a499c53487..e2763ec41069f3 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -157,6 +157,9 @@ jobs: run: echo "$CONCURRENCY_CONTEXT" - name: Checkout uses: actions/checkout@v4 + with: + fetch-depth: 2 + persist-credentials: true - name: Try to ensure the directories for core dumping exist and we can write them. run: | @@ -186,6 +189,10 @@ jobs: - name: Clean output run: rm -rf ./out - name: Run Tests with sanitizers + # Sanitizer tests are not likely to find extra issues so running the same tests + # as above repeatedly on every pull request seems extra time. Instead keep this run + # for master only + if: github.event.pull_request.number == null env: LSAN_OPTIONS: detect_leaks=1 run: | @@ -201,17 +208,34 @@ jobs: BUILD_TYPE=sanitizers scripts/build/gn_gen.sh --args="$GN_ARGS chip_data_model_check_die_on_failure=true" --export-compile-commands BUILD_TYPE=sanitizers scripts/tests/gn_tests.sh done + - name: Generate tests with sanitizers (for tidy) + if: github.event.pull_request.number != null + run: | + rm -rf ./out/sanitizers + BUILD_TYPE=sanitizers scripts/build/gn_gen.sh --args="is_clang=true is_asan=true chip_data_model_check_die_on_failure=true" --export-compile-commands - name: Ensure codegen is done for sanitize run: | ./scripts/run_in_build_env.sh "./scripts/run_codegen_targets.sh out/sanitizers" + - name: Find changed files + id: changed-files + uses: tj-actions/changed-files@v45 - name: Clang-tidy validation # NOTE: clang-tidy crashes on CodegenDataModel_Write due to Nullable/std::optional check. # See https://github.com/llvm/llvm-project/issues/97426 + env: + ALL_CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }} run: | + touch out/changed_files.txt + for file in ${ALL_CHANGED_FILES}; do + echo "$file changed and will be considered for tidy" + echo "$file" >>out/changed_files.txt + done + ./scripts/run_in_build_env.sh \ "./scripts/run-clang-tidy-on-compile-commands.py \ --compile-database out/sanitizers/compile_commands.json \ --file-exclude-regex '/(repo|zzz_generated|lwip/standalone)/|-ReadImpl|-InvokeSubscribeImpl|CodegenDataModel_Write|QuieterReporting' \ + --file-list-file out/changed_files.txt \ check \ " - name: Clean output @@ -382,6 +406,9 @@ jobs: steps: - name: Checkout uses: actions/checkout@v4 + with: + fetch-depth: 2 + persist-credentials: true - name: Checkout submodules & Bootstrap uses: ./.github/actions/checkout-submodules-and-bootstrap with: @@ -418,14 +445,26 @@ jobs: - name: Ensure codegen is done for default run: | ./scripts/run_in_build_env.sh "./scripts/run_codegen_targets.sh out/default" + - name: Find changed files + id: changed-files + uses: tj-actions/changed-files@v45 - name: Clang-tidy validation # NOTE: clang-tidy crashes on CodegenDataModel_Write due to Nullable/std::optional check. # See https://github.com/llvm/llvm-project/issues/97426 + env: + ALL_CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }} run: | + touch out/changed_files.txt + for file in ${ALL_CHANGED_FILES}; do + echo "$file changed and will be considered for tidy" + echo "$file" >>out/changed_files.txt + done + ./scripts/run_in_build_env.sh \ "./scripts/run-clang-tidy-on-compile-commands.py \ --compile-database out/default/compile_commands.json \ --file-exclude-regex '/(repo|zzz_generated|lwip/standalone)/|CodegenDataModel_Write|QuieterReporting' \ + --file-list-file out/changed_files.txt \ check \ " - name: Uploading diagnostic logs diff --git a/scripts/run-clang-tidy-on-compile-commands.py b/scripts/run-clang-tidy-on-compile-commands.py index 407caaa6f490a5..ee3bdc8f33fb65 100755 --- a/scripts/run-clang-tidy-on-compile-commands.py +++ b/scripts/run-clang-tidy-on-compile-commands.py @@ -40,6 +40,7 @@ import tempfile import threading import traceback +from pathlib import Path import click import coloredlogs @@ -92,20 +93,19 @@ def __init__(self, json_entry, gcc_sysroot=None): # # However that seems to potentially disable a lot, so for now just filter out the # offending argument - command_items = [arg for arg in command_items if arg not in {'-c', '-S'}] + command_items = [arg for arg in command_items if arg not in {"-c", "-S"}] # Allow gcc/g++ invocations to also be tidied - arguments should be # compatible and on darwin gcc/g++ is actually a symlink to clang - if compiler in ['clang++', 'clang', 'gcc', 'g++']: + if compiler in ["clang++", "clang", "gcc", "g++"]: self.valid = True self.clang_arguments = command_items[1:] else: - logging.warning( - "Cannot tidy %s - not a clang compile command", self.file) + logging.warning("Cannot tidy %s - not a clang compile command", self.file) return - if compiler in ['gcc', 'g++'] and gcc_sysroot: - self.clang_arguments.insert(0, '--sysroot='+gcc_sysroot) + if compiler in ["gcc", "g++"] and gcc_sysroot: + self.clang_arguments.insert(0, "--sysroot=" + gcc_sysroot) @property def full_path(self): @@ -122,8 +122,12 @@ def SetChecks(self, checks: str): def Check(self): logging.debug("Running tidy on %s from %s", self.file, self.directory) try: - cmd = ["clang-tidy", self.file] + \ - self.tidy_arguments + ["--"] + self.clang_arguments + cmd = ( + ["clang-tidy", self.file] + + self.tidy_arguments + + ["--"] + + self.clang_arguments + ) logging.debug("Executing: %r" % cmd) proc = subprocess.Popen( @@ -156,7 +160,7 @@ def Check(self): "Use -system-headers to display errors from system headers as well.", ] - for line in err.decode('utf-8').split('\n'): + for line in err.decode("utf-8").split("\n"): line = line.strip() if any(map(lambda s: s in line, skip_strings)): @@ -165,7 +169,7 @@ def Check(self): if not line: continue # no empty lines - logging.warning('TIDY %s: %s', self.file, line) + logging.warning("TIDY %s: %s", self.file, line) if proc.returncode != 0: if proc.returncode < 0: @@ -203,18 +207,20 @@ def Failure(self, path: str): def find_darwin_gcc_sysroot(): - for line in subprocess.check_output('xcodebuild -sdk -version'.split()).decode('utf8').split('\n'): - if not line.startswith('Path: '): + for line in subprocess.check_output( + "xcodebuild -sdk -version".split(), text=True + ).splitlines(): + if not line.startswith("Path: "): continue - path = line[line.find(': ')+2:] - if '/MacOSX.platform/' not in path: + path = line[line.find(": ") + 2:] + if "/MacOSX.platform/" not in path: continue logging.info("Found %s" % path) return path # A hard-coded value that works on default installations logging.warning("Using default platform sdk path. This may be incorrect.") - return '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk' + return "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk" class ClangTidyRunner: @@ -228,13 +234,12 @@ def __init__(self): self.gcc_sysroot = None self.file_names_to_check = set() - if sys.platform == 'darwin': + if sys.platform == "darwin": # Darwin gcc invocation will auto select a system root, however clang requires an explicit path since # we are using the built-in pigweed clang-tidy. - logging.info( - 'Searching for a MacOS system root for gcc invocations...') + logging.info("Searching for a MacOS system root for gcc invocations...") self.gcc_sysroot = find_darwin_gcc_sysroot() - logging.info(' Chose: %s' % self.gcc_sysroot) + logging.info(" Chose: %s" % self.gcc_sysroot) def AddDatabase(self, compile_commands_json): database = json.load(open(compile_commands_json)) @@ -245,7 +250,7 @@ def AddDatabase(self, compile_commands_json): continue if item.file in self.file_names_to_check: - logging.info('Ignoring additional request for checking %s', item.file) + logging.info("Ignoring additional request for checking %s", item.file) continue self.file_names_to_check.add(item.file) @@ -270,12 +275,12 @@ def Cleanup(self): # Allow all diagnostics for distinct paths to be applied # at once but never again for future paths for d in diagnostics: - if d['DiagnosticMessage']['FilePath'] not in already_seen: + if d["DiagnosticMessage"]["FilePath"] not in already_seen: all_diagnostics.append(d) # in the future assume these files were already processed for d in diagnostics: - already_seen.add(d['DiagnosticMessage']['FilePath']) + already_seen.add(d["DiagnosticMessage"]["FilePath"]) if all_diagnostics: with open(self.fixes_file, "w") as out: @@ -304,8 +309,7 @@ def ExportFixesTo(self, f): for idx, e in enumerate(self.entries): e.ExportFixesTo( os.path.join( - self.fixes_temporary_file_dir.name, "fixes%d.yaml" % ( - idx + 1,) + self.fixes_temporary_file_dir.name, "fixes%d.yaml" % (idx + 1,) ) ) @@ -407,6 +411,12 @@ def Check(self): type=str, help="Checks to run (passed in to clang-tidy). If not set the .clang-tidy file is used.", ) +@click.option( + "--file-list-file", + default=None, + type=click.Path(exists=True), + help="When provided, only tidy files that match files mentioned in this file.", +) @click.pass_context def main( context, @@ -417,6 +427,7 @@ def main( no_log_timestamps, export_fixes, checks, + file_list_file, ): log_fmt = "%(asctime)s %(levelname)-7s %(message)s" if no_log_timestamps: @@ -452,6 +463,14 @@ def cleanup(): r = re.compile(file_exclude_regex) runner.FilterEntries(lambda e: not r.search(e.file)) + if file_list_file: + acceptable = set() + with open(file_list_file, "rt") as f: + for file_name in f.readlines(): + acceptable.add(Path(file_name.strip()).resolve().as_posix()) + + runner.FilterEntries(lambda e: e.full_path in acceptable) + if export_fixes: runner.ExportFixesTo(export_fixes) @@ -492,4 +511,4 @@ def cmd_fix(context): if __name__ == "__main__": - main(auto_envvar_prefix='CHIP') + main(auto_envvar_prefix="CHIP")