From 21244c437e3555a868344903293c141de093e820 Mon Sep 17 00:00:00 2001 From: Simon Duerr Date: Fri, 13 Mar 2026 06:53:37 +0000 Subject: [PATCH 1/3] Limit clang-tidy CI to changed files on branch workflows (#381) Run the full clang-tidy scan only on pushes to main. Use changed C/C++ files for pull requests, merge groups, and other branch pushes. Treat an empty findings YAML as zero findings so changed-file clang-tidy runs do not fail when no diagnostics are emitted. --- .ci/clang-tidy.py | 65 +++- .ci/determine-clang-tidy-scope.py | 176 +++++++++++ .ci/resolve-clang-tidy-translation-units.py | 292 ++++++++++++++++++ .ci/test/test_determine_clang_tidy_scope.py | 141 +++++++++ ...st_resolve_clang_tidy_translation_units.py | 220 +++++++++++++ .github/workflows/clang-tidy.yml | 62 +++- .github/workflows/python-unit-tests.yml | 26 ++ doc/dev/guidelines/practices.rst | 24 +- 8 files changed, 994 insertions(+), 12 deletions(-) create mode 100644 .ci/determine-clang-tidy-scope.py create mode 100644 .ci/resolve-clang-tidy-translation-units.py create mode 100644 .ci/test/test_determine_clang_tidy_scope.py create mode 100644 .ci/test/test_resolve_clang_tidy_translation_units.py create mode 100644 .github/workflows/python-unit-tests.yml diff --git a/.ci/clang-tidy.py b/.ci/clang-tidy.py index 62a20590ce9..fed4d8e0195 100644 --- a/.ci/clang-tidy.py +++ b/.ci/clang-tidy.py @@ -11,6 +11,7 @@ """ import argparse +import re import subprocess import sys import yaml @@ -65,6 +66,12 @@ def parse_arguments(): help="Comma-separated list of diagnostic names to ignore (e.g., 'clang-diagnostic-error,clang-diagnostic-warning')", ) + parser.add_argument( + "--file-list", + type=Path, + help="Path to a newline-separated list of source files to analyze", + ) + output_group = parser.add_mutually_exclusive_group() output_group.add_argument( "--quiet", @@ -91,9 +98,38 @@ def parse_arguments(): f"compile_commands.json not found in build directory: {args.build_directory}" ) + if args.file_list is not None and not args.file_list.exists(): + parser.error(f"File list does not exist: {args.file_list}") + return args +def create_file_pattern(file_list: Path) -> str: + """ + Create a regular expression that only matches the requested source files. + + Args: + file_list: Path to a file containing one source path per line + + Returns: + Regular expression fragment matching the selected files + """ + + file_patterns = [] + for line in file_list.read_text().splitlines(): + source = line.strip() + if not source: + continue + + resolved_source = (Path.cwd() / source).resolve(strict=False) + file_patterns.append(re.escape(str(resolved_source))) + + if not file_patterns: + return "" + + return "(?:" + "|".join(file_patterns) + ")" + + def count_findings(file_name: Path, ignored_checks: list = []) -> int: """ Reads the YAML file generated by clang-tidy and counts the number of findings per diagnostic name. @@ -107,7 +143,7 @@ def count_findings(file_name: Path, ignored_checks: list = []) -> int: """ with open(file_name, "r") as f: - data = yaml.safe_load(f) + data = yaml.safe_load(f) or {} # Count findings per diagnostic name counter = Counter() @@ -155,7 +191,11 @@ def count_findings(file_name: Path, ignored_checks: list = []) -> int: def run_clang_tidy( - build_dir: Path, output_file: Path, exclude_pattern: str, quiet: bool + build_dir: Path, + output_file: Path, + exclude_pattern: str, + quiet: bool, + include_pattern: str, ) -> int: """ Run clang-tidy on the build directory. @@ -165,13 +205,16 @@ def run_clang_tidy( output_file: Path to the output YAML file exclude_pattern: Regular expression pattern to exclude files quiet: Whether to suppress run-clang-tidy output + include_pattern: Regular expression fragment matching files to analyze Returns: Return code from run-clang-tidy """ - # Convert the exclusion pattern to a negative lookahead pattern - # This makes run-clang-tidy check files that DO NOT match the exclude pattern - negated_pattern = f"^(?!.*({exclude_pattern})).*" + target_pattern = include_pattern or ".*" + if exclude_pattern: + target_pattern = f"^(?!.*(?:{exclude_pattern})){target_pattern}$" + else: + target_pattern = f"^{target_pattern}$" cmd = [ "run-clang-tidy-17.py", @@ -179,7 +222,7 @@ def run_clang_tidy( str(build_dir), "-export-fixes", str(output_file), - negated_pattern, + target_pattern, ] if quiet: @@ -201,10 +244,19 @@ def run_clang_tidy( if __name__ == "__main__": args = parse_arguments() + include_pattern = "" + + if args.file_list is not None: + include_pattern = create_file_pattern(args.file_list) + if not include_pattern: + print("No source files selected for clang-tidy.") + sys.exit(0) print(f"Running clang-tidy on build directory: {args.build_directory}") print(f"Output file: {args.output_file}") print(f"Exclude pattern: {args.exclude}") + if args.file_list is not None: + print(f"File list: {args.file_list}") print("-" * 60) # Run clang-tidy @@ -213,6 +265,7 @@ def run_clang_tidy( args.output_file, args.exclude, not args.verbose, # quiet mode is default unless --verbose is specified + include_pattern, ) # Parse ignored checks diff --git a/.ci/determine-clang-tidy-scope.py b/.ci/determine-clang-tidy-scope.py new file mode 100644 index 00000000000..4bbb46baba5 --- /dev/null +++ b/.ci/determine-clang-tidy-scope.py @@ -0,0 +1,176 @@ +import argparse +import json +import subprocess +import sys +from pathlib import Path + + +ZERO_SHA = "0000000000000000000000000000000000000000" +RELEVANT_SUFFIXES = (".c", ".cc", ".cpp", ".h", ".hh", ".hpp", ".hxx") +SUPPORTED_EVENTS = {"pull_request", "merge_group", "push"} + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + parser = argparse.ArgumentParser(description="Determine clang-tidy CI scope") + parser.add_argument("--event-name", required=True, help="GitHub event name") + parser.add_argument("--ref-name", required=True, help="GitHub ref name") + parser.add_argument("--sha", required=True, help="GitHub commit SHA") + parser.add_argument( + "--event-path", type=Path, required=True, help="Path to GitHub event payload" + ) + parser.add_argument( + "--output-file", + type=Path, + required=True, + help="File that will receive changed C/C++ source paths", + ) + parser.add_argument( + "--github-output", + type=Path, + required=True, + help="Path to the GitHub Actions step output file", + ) + return parser.parse_args(argv) + + +def load_event(event_path: Path) -> dict: + try: + with event_path.open("r", encoding="utf-8") as event_file: + event = json.load(event_file) + except FileNotFoundError as exc: + raise RuntimeError( + f"GitHub event payload file not found: {event_path}." + ) from exc + except json.JSONDecodeError as exc: + raise RuntimeError( + f"Invalid JSON in GitHub event payload {event_path}: {exc.msg} at line {exc.lineno} column {exc.colno}." + ) from exc + + if not isinstance(event, dict): + raise RuntimeError(f"GitHub event payload {event_path} must contain a JSON object.") + + return event + + +def determine_refs(event_name: str, sha: str, event: dict) -> tuple[str, str]: + if event_name not in SUPPORTED_EVENTS: + raise RuntimeError( + f"Unsupported GitHub event '{event_name}'. Supported events: {', '.join(sorted(SUPPORTED_EVENTS))}." + ) + + base_sha = "" + head_sha = sha + + if event_name == "pull_request": + base_sha = event.get("pull_request", {}).get("base", {}).get("sha", "") + elif event_name == "merge_group": + base_sha = event.get("merge_group", {}).get("base_sha", "") + elif event_name == "push": + base_sha = event.get("before", "") + + if event_name in {"pull_request", "merge_group"} and not base_sha: + raise RuntimeError(f"Unable to determine base SHA for GitHub event '{event_name}'.") + + if not head_sha: + raise RuntimeError("Unable to determine head SHA for clang-tidy scope calculation.") + + return base_sha, head_sha + + +def run_git_command(args: list[str]) -> str: + try: + result = subprocess.run( + ["git", *args], + check=True, + capture_output=True, + text=True, + ) + except FileNotFoundError as exc: + raise RuntimeError("The 'git' executable is not available in the CI environment.") from exc + except subprocess.CalledProcessError as exc: + details = (exc.stderr or exc.stdout or "no additional output").strip() + raise RuntimeError( + f"Git command failed: git {' '.join(args)}: {details}" + ) from exc + + return result.stdout + + +def list_changed_sources(base_sha: str, head_sha: str) -> list[str]: + if base_sha and base_sha != ZERO_SHA: + try: + merge_base = run_git_command(["merge-base", head_sha, base_sha]).strip() + diff_output = run_git_command( + ["diff", "--name-only", "--diff-filter=ACMR", f"{merge_base}...{head_sha}"] + ) + except RuntimeError as exc: + if "git merge-base" not in str(exc): + raise + + # PR merge refs and merge-queue refs may not contain every original + # commit object locally. Falling back to a direct base..head diff + # still scopes clang-tidy to files changed by the checked-out ref. + diff_output = run_git_command( + ["diff", "--name-only", "--diff-filter=ACMR", f"{base_sha}..{head_sha}"] + ) + else: + diff_output = run_git_command( + ["diff-tree", "--no-commit-id", "--name-only", "--diff-filter=ACMR", "-r", head_sha] + ) + + return sorted( + { + line.strip() + for line in diff_output.splitlines() + if line.strip().endswith(RELEVANT_SUFFIXES) + } + ) + + +def write_changed_files(output_file: Path, changed_files: list[str]) -> None: + output_file.parent.mkdir(parents=True, exist_ok=True) + output_file.write_text( + "\n".join(changed_files) + ("\n" if changed_files else ""), + encoding="utf-8", + ) + + +def write_github_outputs(github_output: Path, mode: str, has_changes: bool) -> None: + github_output.parent.mkdir(parents=True, exist_ok=True) + with github_output.open("a", encoding="utf-8") as output_file: + output_file.write(f"mode={mode}\n") + output_file.write(f"has_changes={'true' if has_changes else 'false'}\n") + + +def main(argv: list[str] | None = None) -> int: + args = parse_args(argv) + args.output_file.parent.mkdir(parents=True, exist_ok=True) + args.output_file.write_text("", encoding="utf-8") + + try: + if args.event_name == "push" and args.ref_name == "main": + write_github_outputs(args.github_output, "full", True) + return 0 + + event = load_event(args.event_path) + base_sha, head_sha = determine_refs(args.event_name, args.sha, event) + changed_files = list_changed_sources(base_sha, head_sha) + except RuntimeError as exc: + print(exc, file=sys.stderr) + return 1 + + write_changed_files(args.output_file, changed_files) + write_github_outputs(args.github_output, "changed", bool(changed_files)) + + if changed_files: + print("Changed files:") + for changed_file in changed_files: + print(changed_file) + else: + print("No modified source/header files found for clang-tidy.") + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) \ No newline at end of file diff --git a/.ci/resolve-clang-tidy-translation-units.py b/.ci/resolve-clang-tidy-translation-units.py new file mode 100644 index 00000000000..e00e1fe1f33 --- /dev/null +++ b/.ci/resolve-clang-tidy-translation-units.py @@ -0,0 +1,292 @@ +""" +Resolve clang-tidy translation units from changed source/header files. + +This helper is used by the branch-mode clang-tidy CI flow after the build step. +It takes a diff-derived file list and expands header-only changes into affected +translation units by reusing dependency metadata from the build directory. + +Inputs: +- --build-directory: Build folder containing compile_commands.json and Ninja + dependency metadata (.ninja_deps). +- --changed-files: Newline-separated list of files from git diff. This list may + contain source files and headers. +- --output-file: Destination file for resolved translation units. + +Behavior: +- Changed source files (.c/.cc/.cpp) are kept directly. +- Changed headers (.h/.hh/.hpp/.hxx) are mapped to translation units by parsing + `ninja -t deps` output and joining dependency targets with compile_commands + output mappings. +- The result is written as repository-relative source file paths, one per line. + +This script does not run clang-tidy itself; it only prepares the source-file +list consumed by .ci/clang-tidy.py (`--file-list`). +""" + +import argparse +import json +import subprocess +import sys +from pathlib import Path + + +SOURCE_SUFFIXES = (".c", ".cc", ".cpp") +HEADER_SUFFIXES = (".h", ".hh", ".hpp", ".hxx") + + +def get_repo_root() -> Path: + """Return the repository root derived from this script's location.""" + return Path(__file__).resolve().parents[1] + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse CLI arguments for translation-unit resolution.""" + parser = argparse.ArgumentParser( + description="Resolve changed headers to clang-tidy translation units" + ) + parser.add_argument( + "--build-directory", + type=Path, + required=True, + help="Build directory containing compile_commands.json and Ninja metadata", + ) + parser.add_argument( + "--changed-files", + type=Path, + required=True, + help="Path to newline-separated changed files from git diff", + ) + parser.add_argument( + "--output-file", + type=Path, + required=True, + help="Output file with translation units to run clang-tidy on", + ) + return parser.parse_args(argv) + + +def is_source(path: Path) -> bool: + """Return True when the path has a supported source-file suffix.""" + return path.suffix in SOURCE_SUFFIXES + + +def is_header(path: Path) -> bool: + """Return True when the path has a supported header-file suffix.""" + return path.suffix in HEADER_SUFFIXES + + +def read_changed_files(path: Path, repo_root: Path) -> tuple[set[Path], set[Path]]: + """Read and classify changed files into source and header path sets.""" + sources: set[Path] = set() + headers: set[Path] = set() + + for line in path.read_text(encoding="utf-8").splitlines(): + value = line.strip() + if not value: + continue + + candidate = (repo_root / value).resolve(strict=False) + if is_source(candidate): + sources.add(candidate) + elif is_header(candidate): + headers.add(candidate) + + return sources, headers + + +def build_output_to_source_map(build_directory: Path, repo_root: Path) -> dict[str, Path]: + """Map possible Ninja object target names to their source files.""" + compile_commands = build_directory / "compile_commands.json" + try: + raw_compile_commands = compile_commands.read_text(encoding="utf-8") + except FileNotFoundError as exc: + raise RuntimeError( + f"Missing {compile_commands}. Ensure the CI build step generated compile_commands.json." + ) from exc + + try: + data = json.loads(raw_compile_commands) + except json.JSONDecodeError as exc: + raise RuntimeError( + f"Invalid JSON in {compile_commands}: {exc.msg} at line {exc.lineno} column {exc.colno}." + ) from exc + + if not isinstance(data, list): + raise RuntimeError(f"Expected {compile_commands} to contain a JSON list of compile commands.") + + mapping: dict[str, Path] = {} + for index, entry in enumerate(data): + if not isinstance(entry, dict): + raise RuntimeError( + f"Invalid compile command at index {index} in {compile_commands}: expected an object." + ) + + file_value = entry.get("file") + if not file_value: + raise RuntimeError( + f"Invalid compile command at index {index} in {compile_commands}: missing 'file'." + ) + + source = Path(file_value).resolve(strict=False) + output = entry.get("output") + + if output: + mapping[output] = source + + output_path = Path(output) + if not output_path.is_absolute(): + output_path = (repo_root / output_path).resolve(strict=False) + else: + output_path = output_path.resolve(strict=False) + mapping[str(output_path)] = source + + # Ninja deps typically reports object paths relative to build dir. + try: + relative_to_build = output_path.relative_to(build_directory) + mapping[str(relative_to_build)] = source + except ValueError: + pass + + object_guess = source.with_suffix(source.suffix + ".o") + try: + relative_guess = object_guess.relative_to(repo_root) + mapping[str(relative_guess)] = source + except ValueError: + pass + + return mapping + + +def parse_ninja_deps(build_directory: Path) -> dict[str, set[Path]]: + """Parse `ninja -t deps` output into target-to-dependency mappings.""" + cmd = ["ninja", "-C", str(build_directory), "-t", "deps"] + try: + result = subprocess.run(cmd, check=True, capture_output=True, text=True) + except FileNotFoundError as exc: + raise RuntimeError( + "The 'ninja' executable is not available in the CI environment." + ) from exc + except subprocess.CalledProcessError as exc: + details = (exc.stderr or exc.stdout or "no additional output").strip() + raise RuntimeError( + f"Failed to read Ninja dependency data from {build_directory}: {details}" + ) from exc + + deps_by_target: dict[str, set[Path]] = {} + current_target: str | None = None + + for raw_line in result.stdout.splitlines(): + line = raw_line.rstrip() + if not line: + continue + + if not raw_line.startswith(" ") and ":" in line: + current_target = line.split(":", 1)[0].strip() + deps_by_target[current_target] = set() + continue + + if current_target is None: + continue + + dep = line.strip() + if dep: + dep_path = Path(dep) + if not dep_path.is_absolute(): + dep_path = (build_directory / dep_path).resolve(strict=False) + else: + dep_path = dep_path.resolve(strict=False) + deps_by_target[current_target].add(dep_path) + + return deps_by_target + + +def resolve_translation_units( + changed_sources: set[Path], + changed_headers: set[Path], + output_to_source: dict[str, Path], + deps_by_target: dict[str, set[Path]], +) -> set[Path]: + """Resolve final source files by expanding changed headers to owning TUs.""" + resolved_sources = set(changed_sources) + if not changed_headers: + return resolved_sources + + for target, deps in deps_by_target.items(): + source = output_to_source.get(target) + if source is None: + continue + + if deps.intersection(changed_headers): + resolved_sources.add(source) + + return resolved_sources + + +def write_sources(output_file: Path, repo_root: Path, sources: set[Path]) -> None: + """Write resolved sources as sorted repository-relative paths.""" + lines = [] + for source in sorted(sources): + try: + lines.append(str(source.relative_to(repo_root))) + except ValueError: + continue + + output_file.parent.mkdir(parents=True, exist_ok=True) + output_file.write_text("\n".join(lines) + ("\n" if lines else ""), encoding="utf-8") + + +def main(argv: list[str] | None = None) -> int: + """Run the resolver workflow and write translation-unit output.""" + args = parse_args(argv) + repo_root = get_repo_root() + + changed_sources, changed_headers = read_changed_files(args.changed_files, repo_root) + + if not changed_sources and not changed_headers: + args.output_file.write_text("") + print("No changed source/header files selected for clang-tidy.") + return 0 + + try: + output_to_source = build_output_to_source_map(args.build_directory, repo_root) + deps_by_target = parse_ninja_deps(args.build_directory) if changed_headers else {} + except RuntimeError as exc: + print(exc, file=sys.stderr) + return 1 + + if changed_headers and not output_to_source: + print( + "No compile command entries were available to map changed headers to translation units.", + file=sys.stderr, + ) + return 1 + + if changed_headers and not deps_by_target: + print( + f"No Ninja dependency data was available in {args.build_directory} for changed header resolution.", + file=sys.stderr, + ) + return 1 + + resolved_sources = resolve_translation_units( + changed_sources, changed_headers, output_to_source, deps_by_target + ) + + write_sources(args.output_file, repo_root, resolved_sources) + if resolved_sources: + print("Resolved translation units:") + for source in sorted(resolved_sources): + print(source) + else: + if changed_headers: + print( + "Warning: changed headers did not resolve to any translation units.", + file=sys.stderr, + ) + print("No translation units resolved from changed source/header files.") + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.ci/test/test_determine_clang_tidy_scope.py b/.ci/test/test_determine_clang_tidy_scope.py new file mode 100644 index 00000000000..ef31c665fcf --- /dev/null +++ b/.ci/test/test_determine_clang_tidy_scope.py @@ -0,0 +1,141 @@ +import importlib.util +import subprocess +import tempfile +import unittest +from pathlib import Path +from unittest import mock + + +class TestDetermineClangTidyScope(unittest.TestCase): + @classmethod + def setUpClass(cls): + repo_root = Path(__file__).resolve().parents[2] + module_path = repo_root / ".ci/determine-clang-tidy-scope.py" + spec = importlib.util.spec_from_file_location( + "determine_clang_tidy_scope", module_path + ) + module = importlib.util.module_from_spec(spec) + assert spec.loader is not None + spec.loader.exec_module(module) + cls.module = module + + def test_load_event_fails_for_missing_file(self): + with self.assertRaisesRegex(RuntimeError, "GitHub event payload file not found"): + self.module.load_event(Path("/does/not/exist.json")) + + def test_determine_refs_rejects_unsupported_event(self): + with self.assertRaisesRegex(RuntimeError, "Unsupported GitHub event"): + self.module.determine_refs("workflow_dispatch", "head", {}) + + def test_determine_refs_requires_pull_request_base_sha(self): + with self.assertRaisesRegex(RuntimeError, "Unable to determine base SHA"): + self.module.determine_refs("pull_request", "head", {}) + + def test_run_git_command_wraps_missing_git(self): + with mock.patch.object(self.module.subprocess, "run", side_effect=FileNotFoundError): + with self.assertRaisesRegex(RuntimeError, "'git' executable is not available"): + self.module.run_git_command(["status"]) + + def test_list_changed_sources_uses_diff_filter_and_suffix_filter(self): + outputs = { + ("merge-base", "head", "base"): "merge-base-sha\n", + ("diff", "--name-only", "--diff-filter=ACMR", "merge-base-sha...head"): + "src/main.cpp\nsrc/removed.cpp\ndoc/readme.md\ninclude/api.hpp\n", + } + + def fake_run_git_command(args): + return outputs[tuple(args)] + + with mock.patch.object(self.module, "run_git_command", side_effect=fake_run_git_command): + changed = self.module.list_changed_sources("base", "head") + + self.assertEqual(changed, ["include/api.hpp", "src/main.cpp", "src/removed.cpp"]) + + def test_list_changed_sources_falls_back_when_merge_base_fails(self): + def fake_run_git_command(args): + if args == ["merge-base", "head", "base"]: + raise RuntimeError("Git command failed: git merge-base head base: missing commit") + if args == ["diff", "--name-only", "--diff-filter=ACMR", "base..head"]: + return "src/main.cpp\n" + raise AssertionError(f"Unexpected git args: {args}") + + with mock.patch.object(self.module, "run_git_command", side_effect=fake_run_git_command): + changed = self.module.list_changed_sources("base", "head") + + self.assertEqual(changed, ["src/main.cpp"]) + + def test_list_changed_sources_raises_for_non_merge_base_git_failure(self): + def fake_run_git_command(args): + if args == ["merge-base", "head", "base"]: + return "merge-base-sha\n" + raise RuntimeError("Git command failed: git diff --name-only --diff-filter=ACMR merge-base-sha...head: bad revision") + + with mock.patch.object(self.module, "run_git_command", side_effect=fake_run_git_command): + with self.assertRaisesRegex(RuntimeError, "bad revision"): + self.module.list_changed_sources("base", "head") + + def test_write_changed_files_creates_parent_directory(self): + with tempfile.TemporaryDirectory() as tmp: + output_file = Path(tmp) / "nested" / "scope" / "files.txt" + + self.module.write_changed_files(output_file, ["src/main.cpp"]) + + self.assertEqual(output_file.read_text(), "src/main.cpp\n") + + def test_main_main_branch_push_sets_full_mode(self): + with tempfile.TemporaryDirectory() as tmp: + output_file = Path(tmp) / "nested" / "files.txt" + github_output = Path(tmp) / "nested" / "github-output.txt" + event_path = Path(tmp) / "event.json" + event_path.write_text("{}") + + ret = self.module.main( + [ + "--event-name", + "push", + "--ref-name", + "main", + "--sha", + "head", + "--event-path", + str(event_path), + "--output-file", + str(output_file), + "--github-output", + str(github_output), + ] + ) + + self.assertEqual(ret, 0) + self.assertEqual(output_file.read_text(), "") + self.assertEqual(github_output.read_text(), "mode=full\nhas_changes=true\n") + + def test_main_returns_error_for_invalid_event_payload(self): + with tempfile.TemporaryDirectory() as tmp: + output_file = Path(tmp) / "files.txt" + github_output = Path(tmp) / "github-output.txt" + event_path = Path(tmp) / "event.json" + event_path.write_text("not json") + + ret = self.module.main( + [ + "--event-name", + "pull_request", + "--ref-name", + "feature", + "--sha", + "head", + "--event-path", + str(event_path), + "--output-file", + str(output_file), + "--github-output", + str(github_output), + ] + ) + + self.assertEqual(ret, 1) + + +if __name__ == "__main__": + unittest.main() diff --git a/.ci/test/test_resolve_clang_tidy_translation_units.py b/.ci/test/test_resolve_clang_tidy_translation_units.py new file mode 100644 index 00000000000..54071b02a8e --- /dev/null +++ b/.ci/test/test_resolve_clang_tidy_translation_units.py @@ -0,0 +1,220 @@ +import importlib.util +import subprocess +import tempfile +import unittest +from pathlib import Path +from unittest import mock + + +class TestResolveClangTidyTranslationUnits(unittest.TestCase): + @classmethod + def setUpClass(cls): + repo_root = Path(__file__).resolve().parents[2] + module_path = repo_root / ".ci/resolve-clang-tidy-translation-units.py" + spec = importlib.util.spec_from_file_location( + "resolve_clang_tidy_translation_units", module_path + ) + module = importlib.util.module_from_spec(spec) + assert spec.loader is not None + spec.loader.exec_module(module) + cls.module = module + + def test_read_changed_files_classifies_sources_and_headers(self): + with tempfile.TemporaryDirectory() as tmp: + repo_root = Path(tmp) + changed = repo_root / "changed.txt" + changed.write_text("src/main.cpp\ninclude/api.hpp\ndoc/readme.md\n\n") + + sources, headers = self.module.read_changed_files(changed, repo_root) + + self.assertEqual(sources, {(repo_root / "src/main.cpp").resolve(strict=False)}) + self.assertEqual(headers, {(repo_root / "include/api.hpp").resolve(strict=False)}) + + def test_build_output_to_source_map_tracks_output_variants(self): + with tempfile.TemporaryDirectory() as tmp: + repo_root = Path(tmp) + build_dir = repo_root / "build" + build_dir.mkdir(parents=True) + + source = (repo_root / "src/main.cpp").resolve(strict=False) + output = "build/obj/main.cpp.o" + compile_commands = build_dir / "compile_commands.json" + compile_commands.write_text( + "[" + '{"file": "' + str(source) + '", "output": "' + output + '"}' + "]" + ) + + mapping = self.module.build_output_to_source_map(build_dir, repo_root) + + self.assertEqual(mapping[output], source) + self.assertEqual( + mapping[str((repo_root / output).resolve(strict=False))], source + ) + self.assertEqual(mapping["obj/main.cpp.o"], source) + + def test_build_output_to_source_map_fails_without_compile_commands(self): + with tempfile.TemporaryDirectory() as tmp: + repo_root = Path(tmp) + build_dir = repo_root / "build" + build_dir.mkdir(parents=True) + + with self.assertRaisesRegex(RuntimeError, "Missing .*compile_commands.json"): + self.module.build_output_to_source_map(build_dir, repo_root) + + def test_resolve_translation_units_expands_header_dependencies(self): + changed_sources = {Path("/repo/src/direct.cpp")} + changed_header = Path("/repo/include/common.hpp") + + output_to_source = { + "obj/a.o": Path("/repo/src/a.cpp"), + "obj/b.o": Path("/repo/src/b.cpp"), + } + deps_by_target = { + "obj/a.o": {changed_header}, + "obj/b.o": {Path("/repo/include/unrelated.hpp")}, + } + + resolved = self.module.resolve_translation_units( + changed_sources, + {changed_header}, + output_to_source, + deps_by_target, + ) + + self.assertEqual( + resolved, + {Path("/repo/src/direct.cpp"), Path("/repo/src/a.cpp")}, + ) + + def test_parse_ninja_deps_parses_targets_and_dependencies(self): + with tempfile.TemporaryDirectory() as tmp: + build_dir = Path(tmp) / "build" + dep1 = str((Path(tmp) / "include/a.hpp").resolve(strict=False)) + dep2 = str((Path(tmp) / "src/a.cpp").resolve(strict=False)) + dep3 = str((Path(tmp) / "include/b.hpp").resolve(strict=False)) + + ninja_stdout = ( + "obj/a.o: #deps 2, deps mtime 123456\n" + f" {dep1}\n" + f" {dep2}\n" + "obj/b.o: #deps 1, deps mtime 123456\n" + f" {dep3}\n" + ) + + completed = subprocess.CompletedProcess( + args=[], + returncode=0, + stdout=ninja_stdout, + ) + + with mock.patch.object(self.module.subprocess, "run", return_value=completed) as run_mock: + deps = self.module.parse_ninja_deps(build_dir) + + run_mock.assert_called_once_with( + ["ninja", "-C", str(build_dir), "-t", "deps"], + check=True, + capture_output=True, + text=True, + ) + + self.assertEqual( + deps, + { + "obj/a.o": { + Path(dep1).resolve(strict=False), + Path(dep2).resolve(strict=False), + }, + "obj/b.o": {Path(dep3).resolve(strict=False)}, + }, + ) + + def test_parse_ninja_deps_resolves_relative_dependencies_from_build_directory(self): + with tempfile.TemporaryDirectory() as tmp: + build_dir = Path(tmp) / "build" + ninja_stdout = "obj/a.o: #deps 1, deps mtime 123456\n ../include/a.hpp\n" + completed = subprocess.CompletedProcess(args=[], returncode=0, stdout=ninja_stdout) + + with mock.patch.object(self.module.subprocess, "run", return_value=completed): + deps = self.module.parse_ninja_deps(build_dir) + + self.assertEqual( + deps, + { + "obj/a.o": { + (build_dir / "../include/a.hpp").resolve(strict=False), + } + }, + ) + + def test_write_sources_uses_repo_relative_sorted_paths(self): + with tempfile.TemporaryDirectory() as tmp: + repo_root = Path(tmp).resolve(strict=False) + output_file = repo_root / "out.txt" + sources = { + repo_root / "src/b.cpp", + repo_root / "src/a.cpp", + Path("/outside/repo/ignored.cpp"), + } + + self.module.write_sources(output_file, repo_root, sources) + + self.assertEqual(output_file.read_text(), "src/a.cpp\nsrc/b.cpp\n") + + def test_write_sources_creates_parent_directory(self): + with tempfile.TemporaryDirectory() as tmp: + repo_root = Path(tmp).resolve(strict=False) + output_file = repo_root / "nested" / "clang-tidy" / "out.txt" + + self.module.write_sources(output_file, repo_root, {repo_root / "src/a.cpp"}) + + self.assertEqual(output_file.read_text(), "src/a.cpp\n") + + def test_main_returns_early_when_no_relevant_changes(self): + with tempfile.TemporaryDirectory() as tmp: + repo_root = Path(tmp) + changed = repo_root / "changed.txt" + changed.write_text("README.md\n") + output_file = repo_root / "out.txt" + + ret = self.module.main( + [ + "--build-directory", + str(repo_root / "build"), + "--changed-files", + str(changed), + "--output-file", + str(output_file), + ] + ) + + self.assertEqual(ret, 0) + self.assertEqual(output_file.read_text(), "") + + def test_main_fails_for_header_changes_without_compile_commands(self): + with tempfile.TemporaryDirectory() as tmp: + repo_root = Path(self.module.get_repo_root()) + changed = Path(tmp) / "changed.txt" + output_file = Path(tmp) / "out.txt" + build_dir = Path(tmp) / "build" + build_dir.mkdir(parents=True) + + header_path = repo_root / "include" / "api.hpp" + changed.write_text(f"{header_path.relative_to(repo_root)}\n") + + ret = self.module.main( + [ + "--build-directory", + str(build_dir), + "--changed-files", + str(changed), + "--output-file", + str(output_file), + ] + ) + + self.assertEqual(ret, 1) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index 0ed20ae72c5..257d4157e73 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.github/workflows/clang-tidy.yml @@ -1,6 +1,9 @@ name: Clang-tidy check -on: [ merge_group, push, pull_request ] +on: + merge_group: + push: + pull_request: jobs: clang-tidy: @@ -22,10 +25,40 @@ jobs: with: fetch-depth: 0 + - name: Fetch clang-tidy diff refs + if: ${{ github.event_name == 'pull_request' || github.event_name == 'merge_group' }} + run: | + set -euo pipefail + + if [[ "${{ github.event_name }}" == "pull_request" ]]; then + base_sha=$(python3 -c 'import json, os; print(json.load(open(os.environ["GITHUB_EVENT_PATH"], "r", encoding="utf-8"))["pull_request"]["base"]["sha"])') + head_sha=$(python3 -c 'import json, os; print(json.load(open(os.environ["GITHUB_EVENT_PATH"], "r", encoding="utf-8"))["pull_request"]["head"]["sha"])') + + git fetch --no-tags origin \ + "${base_sha}" \ + "${head_sha}" + else + base_sha=$(python3 -c 'import json, os; print(json.load(open(os.environ["GITHUB_EVENT_PATH"], "r", encoding="utf-8")).get("merge_group", {}).get("base_sha", ""))') + + git fetch --no-tags origin \ + "${base_sha}" \ + "${{ github.sha }}" + fi + + - name: Determine clang-tidy scope + id: scope + run: >- + python3 .ci/determine-clang-tidy-scope.py --event-name "${{ github.event_name }}" + --ref-name "${{ github.ref_name }}" --sha "${{ github.sha }}" + --event-path "${GITHUB_EVENT_PATH}" --output-file .clang-tidy-diff-files.txt + --github-output "${GITHUB_OUTPUT}" + - name: Set up Docker Buildx + if: ${{ steps.scope.outputs.mode == 'full' || steps.scope.outputs.has_changes == 'true' }} uses: docker/setup-buildx-action@v3 - name: Build development Image + if: ${{ steps.scope.outputs.mode == 'full' || steps.scope.outputs.has_changes == 'true' }} id: build-docker-development uses: docker/build-push-action@v6 with: @@ -37,15 +70,36 @@ jobs: cache-to: type=gha,mode=max,ignore-error=true - name: Run the ${{ matrix.platform }} ${{ matrix.config }} build inside the docker container + if: ${{ steps.scope.outputs.mode == 'full' || steps.scope.outputs.has_changes == 'true' }} run: >- DOCKER_UID=$(id -u) DOCKER_GID=$(id -g) DOCKER_HISTORY=/dev/null docker compose run --rm development python3 .ci/build.py --preset ${{ matrix.preset }} --platform "linux" --cxxid "clang" --cxxstd "14" --config "${{ matrix.config }}" - - name: Run the clang-tidy.py inside the docker container - id: clang-tidy + - name: Resolve changed translation units for clang-tidy + if: ${{ steps.scope.outputs.mode == 'changed' && steps.scope.outputs.has_changes == 'true' }} run: >- DOCKER_UID=$(id -u) DOCKER_GID=$(id -g) DOCKER_HISTORY=/dev/null docker compose run --rm development - python3 .ci/clang-tidy.py --build_directory=build/tests/${{ matrix.platform }}/${{ matrix.config }} --output_file=ct-findings.yaml --exclude="3rdparty" --ignore-checks="clang-diagnostic-error" + python3 .ci/resolve-clang-tidy-translation-units.py --build-directory=build/tests/${{ matrix.platform }}/${{ matrix.config }} --changed-files=.clang-tidy-diff-files.txt --output-file=.clang-tidy-changed-files.txt + + - name: Run the clang-tidy.py inside the docker container + if: ${{ steps.scope.outputs.mode == 'full' || steps.scope.outputs.has_changes == 'true' }} + id: clang_tidy + env: + CLANG_TIDY_MODE: ${{ steps.scope.outputs.mode }} + run: | + set -euo pipefail + + if [[ "${CLANG_TIDY_MODE}" == "full" ]]; then + DOCKER_UID=$(id -u) DOCKER_GID=$(id -g) DOCKER_HISTORY=/dev/null docker compose run --rm development \ + python3 .ci/clang-tidy.py --build_directory=build/tests/${{ matrix.platform }}/${{ matrix.config }} --output_file=ct-findings.yaml --exclude="3rdparty" --ignore-checks="clang-diagnostic-error" + else + DOCKER_UID=$(id -u) DOCKER_GID=$(id -g) DOCKER_HISTORY=/dev/null docker compose run --rm development \ + python3 .ci/clang-tidy.py --build_directory=build/tests/${{ matrix.platform }}/${{ matrix.config }} --output_file=ct-findings.yaml --exclude="3rdparty" --ignore-checks="clang-diagnostic-error" --file-list=.clang-tidy-changed-files.txt + fi + + - name: Skip clang-tidy because no C/C++ files changed + if: ${{ steps.scope.outputs.mode == 'changed' && steps.scope.outputs.has_changes != 'true' }} + run: echo "Skipping clang-tidy because the diff does not contain modified C/C++ files." - name: Upload clang-tidy findings if: ${{ steps.clang_tidy.outcome == 'failure' }} diff --git a/.github/workflows/python-unit-tests.yml b/.github/workflows/python-unit-tests.yml new file mode 100644 index 00000000000..65fc1f6a9ab --- /dev/null +++ b/.github/workflows/python-unit-tests.yml @@ -0,0 +1,26 @@ +name: Python unit tests + +on: + push: + paths: + - '.ci/**/*.py' + - '.github/workflows/python-unit-tests.yml' + pull_request: + paths: + - '.ci/**/*.py' + - '.github/workflows/python-unit-tests.yml' + +jobs: + python-unit-tests: + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.x' + + - name: Run .ci Python unit tests + run: python3 -m unittest discover -s .ci/test -p 'test_*.py' \ No newline at end of file diff --git a/doc/dev/guidelines/practices.rst b/doc/dev/guidelines/practices.rst index 6d61949e7c8..62a980c8734 100644 --- a/doc/dev/guidelines/practices.rst +++ b/doc/dev/guidelines/practices.rst @@ -14,7 +14,27 @@ Additionally, most of these practices are enforced by clang-tidy which is run as The clang-tidy configuration can be found in the file ``.clang-tidy`` in the root directory of openbsw repository. The CI script for running clang-tidy is located in ``.ci/clang-tidy.py``, which runs an llvm helper tool called ``run-clang-tidy`` which checks all compilation units specified in a compile_commands.json file, which is generated by -current CMake configurations. +current CMake configurations. Pull requests, merge queue runs, and non-main branch pushes restrict clang-tidy to the +changed scope, while pushes to ``main`` keep the full repository scan. + +For the branch-scoped CI runs, the workflow first fetches the relevant base and head commits for +``pull_request`` and ``merge_group`` events before it determines the changed source files. This keeps +``git merge-base`` available in the normal case and reduces the number of fallbacks caused by synthetic +GitHub refs or incomplete local commit graphs. The helper script ``.ci/determine-clang-tidy-scope.py`` +still keeps a fallback to a direct base-to-head diff for CI environments where no merge base can be +resolved from the fetched commits. + +This means the branch-mode clang-tidy flow is: + +#. checkout with full history +#. fetch the relevant comparison commits for PR or merge queue runs +#. compute changed source/header files from git diff (`.c`, `.cc`, `.cpp`, `.h`, `.hh`, `.hpp`, `.hxx`) +#. resolve changed headers to affected translation units using build dependency metadata (`ninja -t deps`) +#. skip clang-tidy when no translation units are selected +#. otherwise run clang-tidy only for the selected translation units + +Pushes to ``main`` skip this reduced-scope path and continue to run the full repository scan. + In case you want to run the llvm helper tool ``run-clang-tidy`` locally, you can use the dev container in the root ``docker-compose.yaml``, build the project with a unit test configuration, in order to generate the compile_commands.json file, which will appear in ``build/tests/posix/Debug/compile_commands.json``, if you use the @@ -28,7 +48,7 @@ Alternatively you can run the same script that is used in the CI pipeline: .. code-block:: bash - python3 .ci/clang-tidy.py + python3 .ci/clang-tidy.py --build_directory --output_file In case you want to run clang-tidy on a single file, you can also use the following command: From b167e31bd500a778e95c15ef9236e4e7bce14208 Mon Sep 17 00:00:00 2001 From: Simon Duerr Date: Fri, 13 Mar 2026 16:53:31 +0000 Subject: [PATCH 2/3] Temporarily limit PR CI to clang-tidy for scenario validation --- .ci/clang-tidy-ci-scenarios.md | 45 +++++++++++++++++++++++ .github/workflows/build.yml | 2 +- .github/workflows/code-coverage.yml | 2 +- .github/workflows/doxygen-build.yml | 2 +- .github/workflows/format.yml | 2 +- .github/workflows/puncover_tool.yml | 2 +- .github/workflows/python-unit-tests.yml | 4 -- .github/workflows/rim.yml | 2 +- .github/workflows/run-pytest-on-posix.yml | 2 +- .github/workflows/sphinx-doc-build.yml | 2 +- 10 files changed, 53 insertions(+), 12 deletions(-) create mode 100644 .ci/clang-tidy-ci-scenarios.md diff --git a/.ci/clang-tidy-ci-scenarios.md b/.ci/clang-tidy-ci-scenarios.md new file mode 100644 index 00000000000..6b6f196c164 --- /dev/null +++ b/.ci/clang-tidy-ci-scenarios.md @@ -0,0 +1,45 @@ +# Clang-tidy CI scenario checklist + +Use this checklist to validate the changed-scope clang-tidy flow in real CI while non-clang-tidy PR workflows are temporarily disabled. + +## Campaign setup + +1. Keep `.github/workflows/clang-tidy.yml` PR trigger enabled. +2. Keep `pull_request` removed from other top-level workflows during this campaign. +3. For each scenario branch, open a PR against `main` and record outcomes below. + +## Scenarios + +| ID | Branch name | Change type | Expected mode | Expected has_changes | Expected outcome | +| --- | --- | --- | --- | --- | --- | +| S01 | `ci-scope-s01-header-only` | Change only a common header (`.h/.hpp`) | `changed` | `true` | Header resolves to one or more TUs; clang-tidy runs | +| S02 | `ci-scope-s02-header-plus-source` | Change one header and one source | `changed` | `true` | Union of direct source + header-impacted TUs | +| S03 | `ci-scope-s03-rename-source` | `git mv` a `.cpp` file | `changed` | `true` | Renamed source appears in file list; clang-tidy runs | +| S04 | `ci-scope-s04-rename-header` | `git mv` a header and update includes | `changed` | `true` | Renamed header resolves to impacted TUs | +| S05 | `ci-scope-s05-delete-source` | Delete a `.cpp` file | `changed` | usually `false` | Deleted file excluded by diff filter; no missing-file failure | +| S06 | `ci-scope-s06-non-cpp-only` | Change only docs/config | `changed` | `false` | Scope step reports no relevant files; clang-tidy skipped | +| S07 | `ci-scope-s07-merge-history` | Scenario branch includes a merge commit | `changed` | `true` or `false` | Merge-base path or fallback works without crash | + +## Evidence to capture per scenario + +- Scope step output values: `mode`, `has_changes` +- Logged changed files list +- Logged resolved translation units list +- Final clang-tidy step status (run/skip, pass/fail) +- Any warnings from scope/resolve scripts + +## Result log + +| ID | PR URL | Status | Notes | +| --- | --- | --- | --- | +| S01 | | | | +| S02 | | | | +| S03 | | | | +| S04 | | | | +| S05 | | | | +| S06 | | | | +| S07 | | | | + +## Rollback + +After completing validation, restore original `pull_request` triggers in all top-level workflows before merging to `main`. diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index f6419c3546c..80a7de2d8f1 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1,6 +1,6 @@ name: Build S32k and posix platform -on: [workflow_call, push, pull_request] +on: [workflow_call, push] jobs: run-command: diff --git a/.github/workflows/code-coverage.yml b/.github/workflows/code-coverage.yml index 0b749adaf5a..bc9e6f3f9e7 100644 --- a/.github/workflows/code-coverage.yml +++ b/.github/workflows/code-coverage.yml @@ -1,6 +1,6 @@ name: Code Coverage -on: [workflow_call, pull_request] +on: [workflow_call] jobs: build: diff --git a/.github/workflows/doxygen-build.yml b/.github/workflows/doxygen-build.yml index 2e5962536df..402279a5422 100644 --- a/.github/workflows/doxygen-build.yml +++ b/.github/workflows/doxygen-build.yml @@ -1,6 +1,6 @@ name: Build and Deploy Doxygen Documentation -on: [workflow_call, pull_request] +on: [workflow_call] jobs: build: diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml index 8b60c0df708..4797d720298 100644 --- a/.github/workflows/format.yml +++ b/.github/workflows/format.yml @@ -1,6 +1,6 @@ name: Code format check -on: [ merge_group, push, pull_request ] +on: [ merge_group, push ] jobs: treefmt: diff --git a/.github/workflows/puncover_tool.yml b/.github/workflows/puncover_tool.yml index 81dc0cfcb78..a0545f4e37c 100644 --- a/.github/workflows/puncover_tool.yml +++ b/.github/workflows/puncover_tool.yml @@ -1,6 +1,6 @@ name: Generate Puncover Report -on: [workflow_call, push, pull_request] +on: [workflow_call, push] jobs: elf_tool: diff --git a/.github/workflows/python-unit-tests.yml b/.github/workflows/python-unit-tests.yml index 65fc1f6a9ab..6c1e96f48e5 100644 --- a/.github/workflows/python-unit-tests.yml +++ b/.github/workflows/python-unit-tests.yml @@ -5,10 +5,6 @@ on: paths: - '.ci/**/*.py' - '.github/workflows/python-unit-tests.yml' - pull_request: - paths: - - '.ci/**/*.py' - - '.github/workflows/python-unit-tests.yml' jobs: python-unit-tests: diff --git a/.github/workflows/rim.yml b/.github/workflows/rim.yml index 3d5e244ab9d..e8b6e0754a0 100644 --- a/.github/workflows/rim.yml +++ b/.github/workflows/rim.yml @@ -1,6 +1,6 @@ name: Third party libraries check -on: [ merge_group, push, pull_request ] +on: [ merge_group, push ] jobs: rim: diff --git a/.github/workflows/run-pytest-on-posix.yml b/.github/workflows/run-pytest-on-posix.yml index a77bbcba456..6585bcc37df 100644 --- a/.github/workflows/run-pytest-on-posix.yml +++ b/.github/workflows/run-pytest-on-posix.yml @@ -1,6 +1,6 @@ name: Run pytest for posix platform -on: [push, pull_request] +on: [push] jobs: run-pytest-on-posix: diff --git a/.github/workflows/sphinx-doc-build.yml b/.github/workflows/sphinx-doc-build.yml index fe66e531ae5..b9fd87ebce2 100644 --- a/.github/workflows/sphinx-doc-build.yml +++ b/.github/workflows/sphinx-doc-build.yml @@ -1,6 +1,6 @@ name: Build Sphinx Documentation -on: [workflow_call, pull_request] +on: [workflow_call] permissions: contents: write From c16afb9dd4ac79aa22948c51d0493350b3d134cf Mon Sep 17 00:00:00 2001 From: Simon Duerr Date: Mon, 16 Mar 2026 08:35:24 +0000 Subject: [PATCH 3/3] ci-scope S06: non-C++ only change --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index bd5a3cff57d..878ffbe6633 100644 --- a/README.md +++ b/README.md @@ -103,3 +103,5 @@ For more details see [CONTRIBUTING](CONTRIBUTING.md). Distributed under the [Apache 2.0 License](LICENSE). Also see [NOTICE](NOTICE.md). + +[CI scenario S06] Non-C/C++-only change for clang-tidy scope validation.