From 6e4ad90688af32c0a486ef81f4cc3a107647b22d Mon Sep 17 00:00:00 2001 From: Luis Madrigal <599908+Madrigal@users.noreply.github.com> Date: Thu, 5 Nov 2026 10:29:56 -0500 Subject: [PATCH 1/5] Run clang-tidy only on changed lines in CI Move the clang-tidy-check logic into tools/scripts/run_clang_tidy.py so it can also run locally. Only lines added or modified by the PR are reported, and only files with a real compile command are analyzed: BUILD_ONLY adds the services whose tests or libraries changed, and CMake is configured with tests on and unity builds off. --- .github/workflows/clang-tidy.yml | 33 +--- CONTRIBUTING.md | 14 ++ tools/scripts/run_clang_tidy.py | 272 +++++++++++++++++++++++++++++++ 3 files changed, 293 insertions(+), 26 deletions(-) create mode 100644 tools/scripts/run_clang_tidy.py diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index d5f4309da4a9..1cf886cef022 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.github/workflows/clang-tidy.yml @@ -29,31 +29,12 @@ jobs: - name: Install dependencies run: | sudo apt-get update - sudo apt-get install -y clang-tidy cmake libcurl4-openssl-dev libssl-dev + sudo apt-get install -y clang-tidy cmake libcurl4-openssl-dev libssl-dev python3 - - name: Configure CMake - run: | - cmake -B build \ - -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \ - -DBUILD_ONLY="s3" \ - -DENABLE_TESTING=OFF - - - name: Get changed files - id: changed + # Reports clang-tidy diagnostics only on lines changed by the PR. To run it locally: + # python3 tools/scripts/run_clang_tidy.py + - name: Run clang-tidy on changed lines + if: github.event_name == 'pull_request' env: - BASE_BRANCH: "${{ github.event.pull_request.base.ref }}" - HEAD_BRANCH: "${{ github.event.pull_request.head.ref }}" - run: | - git fetch origin "$BASE_BRANCH" - git fetch origin "$HEAD_BRANCH" - git diff --name-only --diff-filter=d origin/$BASE_BRANCH...origin/$HEAD_BRANCH \ - | grep -E '\.(cpp|cc|cxx|c|h|hh|hpp)$' \ - | grep -v '^generated/' > changed_files.txt || true - echo "count=$(wc -l < changed_files.txt | tr -d ' ')" >> "$GITHUB_OUTPUT" - - - name: Run clang-tidy - if: steps.changed.outputs.count != '0' - run: | - echo "Running clang-tidy on changed files:" - cat changed_files.txt - xargs -a changed_files.txt clang-tidy -p build --warnings-as-errors='performance-*' + BASE_SHA: "${{ github.event.pull_request.base.sha }}" + run: python3 tools/scripts/run_clang_tidy.py --base "$BASE_SHA" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index aaefe208ab01..e4ff83e7817c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -70,6 +70,19 @@ Valid fragment types: - `dependency` - dependency update (e.g. CRT bump) - `breaking-change` - ABI/API breaking change +## Checking Your Changes Locally + +Pull requests run `clang-tidy` on the lines they add or modify, and fail on any finding that `.clang-tidy` treats as an error (`WarningsAsErrors`). You can run the same check before pushing. You need `git`, `cmake`, `python3`, and `clang-tidy`: + +```bash +pip install clang-tidy # or a system clang-tidy: apt install clang-tidy, brew install llvm +git submodule update --init --recursive +git fetch origin +python3 tools/scripts/run_clang_tidy.py +``` + +Run it from the repository root. It checks the commits on your branch since `origin/main`, so commit your changes first. The first run configures CMake into `./build`, which takes a few minutes; later runs are faster. It exits with a non-zero status and lists each error, with file and line, if clang-tidy finds problems on your changed lines. + ## Your First Code Change Before submitting your pull request, refer to the pull request readiness checklist below: @@ -79,6 +92,7 @@ checklist below: * [ ] Git commit message is detailed and includes context behind the change * [ ] If the change is related to an existing Bug Report or Feature Request, the issue number is referenced * [ ] Includes a changelog fragment (run `tools/scripts/new-change`) +* [ ] `clang-tidy` reports no errors on your changes (see [Checking Your Changes Locally](#checking-your-changes-locally)) __Note__: Some changes have additional requirements. Refer to the section below to see if your change will require additional work to be accepted. diff --git a/tools/scripts/run_clang_tidy.py b/tools/scripts/run_clang_tidy.py new file mode 100644 index 000000000000..636089ad9046 --- /dev/null +++ b/tools/scripts/run_clang_tidy.py @@ -0,0 +1,272 @@ +#!/usr/bin/env python3 + +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0. + +""" +Runs clang-tidy on the lines changed between --base and --head, as the clang-tidy-check +workflow does. + +Running locally (from the repo root, with submodules checked out). Needs git, cmake and clang-tidy: + + pip install clang-tidy # or a system clang-tidy (apt install clang-tidy, brew install llvm) + git fetch origin + python3 tools/scripts/run_clang_tidy.py # checks commits since origin/main; see --help + +On macOS the script passes the SDK path from `xcrun --show-sdk-path` to clang-tidy, so system +headers are found without extra flags. + +Steps: + 1. Pick BUILD_ONLY: s3, plus the services whose tests or high-level libraries changed + (and their test dependencies), using the lists in cmake/sdksCommon.cmake. + 2. Configure CMake to produce compile_commands.json, with tests on and unity builds off + so that every source file has its own compile command. + 3. Keep only changed files that have a compile command. Headers and tests of services + that were not configured would fail with missing-include errors, so they are skipped. + 4. Run clang-tidy on those files with -line-filter set to the added or modified lines, so + only diagnostics on those lines are reported. The line filter is built here from the diff + rather than by LLVM's clang-tidy-diff.py, so the clang-tidy binary is the only dependency: + whether and where clang-tidy-diff.py is installed varies between apt, Homebrew and pip + packages (some pip versions don't ship it at all). +""" +import argparse +import contextlib +import json +import os +import re +import secrets +import shutil +import subprocess +import sys + +DEFAULT_SERVICES = ["s3"] +SOURCE_EXTENSIONS = (".cpp", ".cc", ".cxx", ".c") +CHECKED_EXTENSIONS = SOURCE_EXTENSIONS + (".h", ".hh", ".hpp") +# Hunk header of a -U0 diff, e.g. "@@ -10,0 +11,3 @@": 3 lines added starting at line 11. +HUNK_HEADER = re.compile(r"^@@ -\S+ \+(\d+)(?:,(\d+))? @@", re.MULTILINE) +# e.g. "/path/File.cpp:1575:101: error: the parameter 'x' is copied ... [performance-unnecessary-value-param]" +DIAGNOSTIC_ERROR = re.compile(r"^(.+?):(\d+):(\d+): error: (.+)$") + + +def parse_arguments() -> dict: + """Parse arguments and/or print help + + :return: dict with parsed arguments + """ + parser = argparse.ArgumentParser(description="Run clang-tidy on changed lines for AWS-SDK-CPP") + parser.add_argument("--base", default="origin/main", + help="Base ref or commit to diff against (default: origin/main)") + parser.add_argument("--head", default="HEAD", help="Ref or commit with the changes (default: HEAD)") + parser.add_argument("--build_dir", default="build", help="CMake build directory (default: build)") + parser.add_argument("--clang_tidy", default="clang-tidy", help="clang-tidy binary (default: clang-tidy)") + return vars(parser.parse_args()) + + +def main(): + # Keep our output in order with clang-tidy's when stdout is a pipe (e.g. in CI). + sys.stdout.reconfigure(line_buffering=True) + args = parse_arguments() + repo_root = git("rev-parse", "--show-toplevel", cwd=os.getcwd()).strip() + build_dir = os.path.abspath(args["build_dir"]) + diff_range = f"{args['base']}...{args['head']}" + + clang_tidy = shutil.which(args["clang_tidy"]) + if not clang_tidy: + error(f"{args['clang_tidy']} not found; install it (e.g. pip install clang-tidy) or pass --clang_tidy") + return 1 + + changed = git("diff", "--name-only", "--diff-filter=d", diff_range, cwd=repo_root).splitlines() + services = select_services(changed, repo_root) + print(f"BUILD_ONLY={';'.join(services)}") + with workflow_commands_paused(): + configure(repo_root, build_dir, services) + + compiled = compiled_files(repo_root, build_dir) + # Core is always built; if none of its sources map back into the repo, every changed file + # would be skipped and the check would pass without analyzing anything. + if not any(path.startswith("src/aws-cpp-sdk-core/source/") for path in compiled): + error(f"compile_commands.json in {build_dir} has no core sources under {repo_root}") + return 1 + + candidates = [path for path in changed + if path.endswith(CHECKED_EXTENSIONS) and not path.startswith("generated/")] + to_check = [path for path in candidates if path in compiled] + skipped = [path for path in candidates if path not in compiled] + if skipped: + print_paths("Skipped (no compile command):", skipped) + if not to_check: + print("No changed files to check.") + return 0 + line_filter = changed_line_ranges(repo_root, diff_range, to_check) + if not line_filter: + print("No added or modified lines to check.") + return 0 + print_paths("Running clang-tidy on changed lines of:", [entry["name"] for entry in line_filter]) + + tidy_command = [clang_tidy, "-p", build_dir, "--quiet", f"--line-filter={json.dumps(line_filter)}", + *macos_sysroot_args(), *(entry["name"] for entry in line_filter)] + with workflow_commands_paused(): + returncode, errors = run_clang_tidy(tidy_command, repo_root) + if returncode == 0: + return 0 + if not errors: + error(f"clang-tidy exited with {returncode}; see the output above") + return returncode + + print(f"\nclang-tidy reported {len(errors)} error(s) on changed lines:") + for path, line, column, message in errors: + print(f" - {path}:{line}:{column}: {message}") + if os.environ.get("GITHUB_ACTIONS"): + # Shown on the PR's "Files changed" tab, next to the offending line. + print(f"::error file={annotation_escape(path, True)},line={line},col={column}::" + f"{annotation_escape(message)}") + return returncode + + +def error(message: str): + """Print an error, as a GitHub annotation when running in Actions.""" + prefix = "::error::" if os.environ.get("GITHUB_ACTIONS") else "ERROR: " + print(f"{prefix}{message}", file=sys.stderr) + + +def git(*args: str, cwd: str) -> str: + return subprocess.run(["git", *args], cwd=cwd, check=True, capture_output=True, text=True).stdout + + +def read_cmake_flatmap(sdks_common: str, list_name: str) -> dict: + """Read a list such as SDK_TEST_PROJECT_LIST from cmake/sdksCommon.cmake. + + Entries look like `list(APPEND SDK_TEST_PROJECT_LIST "s3-crt:tests/a-tests,tests/b-tests")`. + + :return: dict of key to list of values, e.g. {"s3-crt": ["tests/a-tests", "tests/b-tests"]} + """ + result = {} + for key, values in re.findall(rf'{list_name} "([^":]+):([^"]*)"', sdks_common): + result.setdefault(key, []).extend(v for v in values.split(",") if v) + return result + + +def select_services(changed_files: list, repo_root: str) -> list: + """Return the BUILD_ONLY services needed to get compile commands for the changed files.""" + with open(os.path.join(repo_root, "cmake", "sdksCommon.cmake")) as sdks_common_file: + sdks_common = sdks_common_file.read() + test_projects = read_cmake_flatmap(sdks_common, "SDK_TEST_PROJECT_LIST") + test_dependencies = read_cmake_flatmap(sdks_common, "TEST_DEPENDENCY_LIST") + service_by_test_dir = {project: service for service, projects in test_projects.items() for project in projects} + + services = set(DEFAULT_SERVICES) + for top_dir in {"/".join(path.split("/")[:2]) for path in changed_files}: + service = service_by_test_dir.get(top_dir) + # src/ holds core and the high-level libraries, whose BUILD_ONLY name is the directory name + # after the prefix (src/aws-cpp-sdk-transfer -> transfer). Service clients under generated/ are ignored. + if service is None and top_dir.startswith("src/aws-cpp-sdk-"): + service = top_dir.removeprefix("src/aws-cpp-sdk-") + if service is None or service == "core": + continue + services.add(service) + # CMake drops a test project unless its test dependencies are also in BUILD_ONLY. + services.update(test_dependencies.get(service, [])) + services.discard("core") # always built + # Set order varies between Python runs; a stable order keeps BUILD_ONLY identical across reruns, + # so CMake doesn't see a changed cache value and the logged value is reproducible. + return sorted(services) + + +def configure(repo_root: str, build_dir: str, services: list): + subprocess.run(["cmake", "-S", repo_root, "-B", build_dir, + "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON", + f"-DBUILD_ONLY={';'.join(services)}", + "-DENABLE_UNITY_BUILD=OFF", + "-DENABLE_TESTING=ON"], check=True) + + +def compiled_files(repo_root: str, build_dir: str) -> set: + """Return the repo-relative paths of files that have a compile command.""" + with open(os.path.join(build_dir, "compile_commands.json")) as compile_commands_file: + compile_commands = json.load(compile_commands_file) + real_root = os.path.realpath(repo_root) + files = set() + for entry in compile_commands: + path = os.path.realpath(os.path.join(entry["directory"], entry["file"])) + if path.startswith(real_root + os.sep): + files.add(os.path.relpath(path, real_root)) + return files + + +def changed_line_ranges(repo_root: str, diff_range: str, paths: list) -> list: + """Return clang-tidy's -line-filter entries for the lines added or modified in each path. + + :return: list such as [{"name": "src/a.cpp", "lines": [[11, 13]]}] + """ + line_filter = [] + for path in paths: + diff = git("diff", "--no-ext-diff", "--no-color", "-U0", diff_range, "--", path, cwd=repo_root) + lines = [] + for start, count in HUNK_HEADER.findall(diff): + count = 1 if count == "" else int(count) + if count: # count 0 means the hunk only deletes lines + lines.append([int(start), int(start) + count - 1]) + if lines: + line_filter.append({"name": path, "lines": lines}) + return line_filter + + +def macos_sysroot_args() -> list: + """On macOS, point clang-tidy at the SDK so it finds the system headers.""" + if sys.platform != "darwin" or not shutil.which("xcrun"): + return [] + sdk = subprocess.run(["xcrun", "--show-sdk-path"], capture_output=True, text=True).stdout.strip() + return [f"--extra-arg=-isysroot{sdk}"] if sdk else [] + + +@contextlib.contextmanager +def workflow_commands_paused(): + """Stop GitHub Actions from interpreting PR-controlled output (CMake, clang-tidy) as workflow commands.""" + token = secrets.token_hex(16) if os.environ.get("GITHUB_ACTIONS") else None + if token: + print(f"::stop-commands::{token}") + try: + yield + finally: + if token: + print(f"::{token}::") + + +def print_paths(title: str, paths: list): + # The "- " prefix keeps a crafted file name such as "::add-mask::x.cpp" from starting a line. + print(title + "".join(f"\n - {path}" for path in paths)) + + +def annotation_escape(value: str, is_property: bool = False) -> str: + """Escape a message (or a property such as file=) for a GitHub workflow command.""" + value = value.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + return value.replace(":", "%3A").replace(",", "%2C") if is_property else value + + +def run_clang_tidy(command: list, repo_root: str) -> tuple: + """Run clang-tidy, echoing its output as it arrives. + + :return: (exit code, list of (path, line, column, message) for each distinct error) + """ + process = subprocess.Popen(command, cwd=repo_root, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True) + real_root = os.path.realpath(repo_root) + errors = [] + for line in process.stdout: + print(line, end="") + match = DIAGNOSTIC_ERROR.match(line.rstrip("\n")) + if not match: + continue + path, line_number, column, message = match.groups() + # Real diagnostics start with an absolute path into the repo; this skips look-alikes such as + # a source line containing "x.cpp:1:1: error: ..." echoed in a code snippet. + real_path = os.path.realpath(path) + if not os.path.isabs(path) or not real_path.startswith(real_root + os.sep): + continue + entry = (os.path.relpath(real_path, real_root), line_number, column, message) + if entry not in errors: # a header can be reported once per file that includes it + errors.append(entry) + return process.wait(), errors + + +if __name__ == "__main__": + sys.exit(main()) From 1b8549a20c574b76acda96afee9a75c6316e7e50 Mon Sep 17 00:00:00 2001 From: Luis Madrigal <599908+Madrigal@users.noreply.github.com> Date: Thu, 5 Nov 2026 11:19:44 -0500 Subject: [PATCH 2/5] DO NOT MERGE: intentional clang-tidy finding to demo the check --- src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp b/src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp index ac41987427f7..aea425f136e0 100644 --- a/src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp +++ b/src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp @@ -208,7 +208,7 @@ Aws::Vector calculateAuthPreferences() { return res; } -Aws::Vector calculateSigV4aSigningRegionSet(const Aws::String& profileName) { +Aws::Vector calculateSigV4aSigningRegionSet(Aws::String profileName) { Aws::Vector res; Aws::String regionSetStr = Aws::Environment::GetEnv(AWS_SIGV4A_SIGNING_REGION_SET_ENV_VAR); if (regionSetStr.empty()) { From eeba9afaab21c4a5293a42c8c6d3c103a89877cc Mon Sep 17 00:00:00 2001 From: Luis Madrigal <599908+Madrigal@users.noreply.github.com> Date: Thu, 5 Nov 2026 13:06:17 -0500 Subject: [PATCH 3/5] clang-tidy check: report only errors, use PR head for correct line numbers --- .github/workflows/clang-tidy.yml | 7 ++- tools/scripts/run_clang_tidy.py | 90 +++++++++++++++++++++++--------- 2 files changed, 69 insertions(+), 28 deletions(-) diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index 1cf886cef022..d1a1d8bdb517 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.github/workflows/clang-tidy.yml @@ -19,17 +19,20 @@ jobs: runs-on: ubuntu-latest steps: + # Check out the PR's own commit rather than the merge with main, so reported line numbers + # (and annotations) match the files in the PR. - name: Checkout code uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 with: + ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 0 submodules: recursive persist-credentials: false - name: Install dependencies run: | - sudo apt-get update - sudo apt-get install -y clang-tidy cmake libcurl4-openssl-dev libssl-dev python3 + sudo apt-get update -qq + sudo apt-get install -y -qq clang-tidy cmake libcurl4-openssl-dev libssl-dev python3 > /dev/null # Reports clang-tidy diagnostics only on lines changed by the PR. To run it locally: # python3 tools/scripts/run_clang_tidy.py diff --git a/tools/scripts/run_clang_tidy.py b/tools/scripts/run_clang_tidy.py index 636089ad9046..f1284a375674 100644 --- a/tools/scripts/run_clang_tidy.py +++ b/tools/scripts/run_clang_tidy.py @@ -45,7 +45,10 @@ # Hunk header of a -U0 diff, e.g. "@@ -10,0 +11,3 @@": 3 lines added starting at line 11. HUNK_HEADER = re.compile(r"^@@ -\S+ \+(\d+)(?:,(\d+))? @@", re.MULTILINE) # e.g. "/path/File.cpp:1575:101: error: the parameter 'x' is copied ... [performance-unnecessary-value-param]" -DIAGNOSTIC_ERROR = re.compile(r"^(.+?):(\d+):(\d+): error: (.+)$") +DIAGNOSTIC = re.compile(r"^(.+?):(\d+):(\d+): (warning|error|note): (.+)$") +# clang's summary lines, e.g. "51527 warnings generated." or "Suppressed 12 warnings (12 in non-user code)." +CLANG_COUNTERS = re.compile( + r"^(\d+ (warnings?|errors?)( and \d+ errors?)? generated\.|Suppressed \d+ warnings|Use -header-filter)") def parse_arguments() -> dict: @@ -74,12 +77,17 @@ def main(): if not clang_tidy: error(f"{args['clang_tidy']} not found; install it (e.g. pip install clang-tidy) or pass --clang_tidy") return 1 + print(clang_tidy_version(clang_tidy)) changed = git("diff", "--name-only", "--diff-filter=d", diff_range, cwd=repo_root).splitlines() services = select_services(changed, repo_root) - print(f"BUILD_ONLY={';'.join(services)}") - with workflow_commands_paused(): - configure(repo_root, build_dir, services) + print(f"Configuring CMake with BUILD_ONLY={';'.join(services)}") + configured, cmake_output = configure(repo_root, build_dir, services) + if not configured: + with workflow_commands_paused(): + print(cmake_output, end="") + error("CMake configure failed; see the output above") + return 1 compiled = compiled_files(repo_root, build_dir) # Core is always built; if none of its sources map back into the repo, every changed file @@ -106,8 +114,11 @@ def main(): tidy_command = [clang_tidy, "-p", build_dir, "--quiet", f"--line-filter={json.dumps(line_filter)}", *macos_sysroot_args(), *(entry["name"] for entry in line_filter)] with workflow_commands_paused(): - returncode, errors = run_clang_tidy(tidy_command, repo_root) + returncode, errors, hidden_warnings = run_clang_tidy(tidy_command, repo_root) + if hidden_warnings: + print(f"{hidden_warnings} warning(s) on changed lines not shown; only errors fail this check.") if returncode == 0: + print("No errors on changed lines.") return 0 if not errors: error(f"clang-tidy exited with {returncode}; see the output above") @@ -172,12 +183,23 @@ def select_services(changed_files: list, repo_root: str) -> list: return sorted(services) -def configure(repo_root: str, build_dir: str, services: list): - subprocess.run(["cmake", "-S", repo_root, "-B", build_dir, - "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON", - f"-DBUILD_ONLY={';'.join(services)}", - "-DENABLE_UNITY_BUILD=OFF", - "-DENABLE_TESTING=ON"], check=True) +def clang_tidy_version(clang_tidy: str) -> str: + output = subprocess.run([clang_tidy, "--version"], capture_output=True, text=True).stdout + version = next((line.strip() for line in output.splitlines() if "version" in line), "unknown version") + return f"Using {clang_tidy} ({version})" + + +def configure(repo_root: str, build_dir: str, services: list) -> tuple: + """Configure CMake. + + :return: (whether it succeeded, its combined output, which main() prints only on failure) + """ + result = subprocess.run(["cmake", "-S", repo_root, "-B", build_dir, + "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON", + f"-DBUILD_ONLY={';'.join(services)}", + "-DENABLE_UNITY_BUILD=OFF", + "-DENABLE_TESTING=ON"], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True) + return result.returncode == 0, result.stdout def compiled_files(repo_root: str, build_dir: str) -> set: @@ -229,6 +251,7 @@ def workflow_commands_paused(): yield finally: if token: + print("Resuming workflow commands (GitHub masks the token below):") print(f"::{token}::") @@ -244,28 +267,43 @@ def annotation_escape(value: str, is_property: bool = False) -> str: def run_clang_tidy(command: list, repo_root: str) -> tuple: - """Run clang-tidy, echoing its output as it arrives. + """Run clang-tidy, echoing errors (with their code snippets and notes) as they arrive. - :return: (exit code, list of (path, line, column, message) for each distinct error) + Warnings don't fail the check, so they and clang's "N warnings generated" counters are not printed. + + :return: (exit code, list of (path, line, column, message) for each distinct error, number of hidden warnings) """ process = subprocess.Popen(command, cwd=repo_root, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True) real_root = os.path.realpath(repo_root) errors = [] + hidden_warnings = 0 + showing = True # whether the current diagnostic (and its snippet and notes) is printed for line in process.stdout: - print(line, end="") - match = DIAGNOSTIC_ERROR.match(line.rstrip("\n")) - if not match: - continue - path, line_number, column, message = match.groups() - # Real diagnostics start with an absolute path into the repo; this skips look-alikes such as - # a source line containing "x.cpp:1:1: error: ..." echoed in a code snippet. - real_path = os.path.realpath(path) - if not os.path.isabs(path) or not real_path.startswith(real_root + os.sep): + text = line.rstrip("\n") + if CLANG_COUNTERS.match(text): continue - entry = (os.path.relpath(real_path, real_root), line_number, column, message) - if entry not in errors: # a header can be reported once per file that includes it - errors.append(entry) - return process.wait(), errors + match = DIAGNOSTIC.match(text) + # Real diagnostics start with an absolute path; snippet lines are indented, so a source line + # containing "x.cpp:1:1: error: ..." is never taken for one. + if match and os.path.isabs(match[1]): + path, line_number, column, level, message = match.groups() + if level == "warning": + showing = False + hidden_warnings += 1 + elif level == "error": + showing = True + real_path = os.path.realpath(path) + entry = (os.path.relpath(real_path, real_root), line_number, column, message) + # Only errors inside the repo become annotations; a header can be reported once per + # file that includes it. + if real_path.startswith(real_root + os.sep) and entry not in errors: + errors.append(entry) + # A note belongs to the diagnostic before it, so it keeps that one's visibility. + elif not text.startswith((" ", "\t")): + showing = True # other messages, such as "Error while processing ...", are always shown + if showing: + print(line, end="") + return process.wait(), errors, hidden_warnings if __name__ == "__main__": From f91dc8cbc51c9d20d1faaf400aa800e96301ccff Mon Sep 17 00:00:00 2001 From: Luis Madrigal <599908+Madrigal@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:04:44 -0500 Subject: [PATCH 4/5] Revert "DO NOT MERGE: intentional clang-tidy finding to demo the check" This reverts commit 1b8549a20c574b76acda96afee9a75c6316e7e50. --- src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp b/src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp index aea425f136e0..ac41987427f7 100644 --- a/src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp +++ b/src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp @@ -208,7 +208,7 @@ Aws::Vector calculateAuthPreferences() { return res; } -Aws::Vector calculateSigV4aSigningRegionSet(Aws::String profileName) { +Aws::Vector calculateSigV4aSigningRegionSet(const Aws::String& profileName) { Aws::Vector res; Aws::String regionSetStr = Aws::Environment::GetEnv(AWS_SIGV4A_SIGNING_REGION_SET_ENV_VAR); if (regionSetStr.empty()) { From 63affbc727504057e364a9921aa351443c9536c5 Mon Sep 17 00:00:00 2001 From: Luis Madrigal <599908+Madrigal@users.noreply.github.com> Date: Mon, 9 Nov 2026 10:20:53 -0500 Subject: [PATCH 5/5] Address review; add changelog --- .changelog/bugfix-ciclang-tidy-changed-lines.json | 6 ++++++ .github/workflows/clang-tidy.yml | 2 -- tools/scripts/run_clang_tidy.py | 3 +-- 3 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 .changelog/bugfix-ciclang-tidy-changed-lines.json diff --git a/.changelog/bugfix-ciclang-tidy-changed-lines.json b/.changelog/bugfix-ciclang-tidy-changed-lines.json new file mode 100644 index 000000000000..b31df66a9aab --- /dev/null +++ b/.changelog/bugfix-ciclang-tidy-changed-lines.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "tools", + "contributor": "Madrigal", + "description": "Run the clang-tidy CI check only on lines changed by the pull request, and only on files with a compile command, so it no longer fails on pre-existing issues or missing includes. The check can also be run locally with tools/scripts/run_clang_tidy.py." +} diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index d1a1d8bdb517..57d2be3a93fa 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.github/workflows/clang-tidy.yml @@ -19,8 +19,6 @@ jobs: runs-on: ubuntu-latest steps: - # Check out the PR's own commit rather than the merge with main, so reported line numbers - # (and annotations) match the files in the PR. - name: Checkout code uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 with: diff --git a/tools/scripts/run_clang_tidy.py b/tools/scripts/run_clang_tidy.py index f1284a375674..762fadad0d40 100644 --- a/tools/scripts/run_clang_tidy.py +++ b/tools/scripts/run_clang_tidy.py @@ -40,8 +40,7 @@ import sys DEFAULT_SERVICES = ["s3"] -SOURCE_EXTENSIONS = (".cpp", ".cc", ".cxx", ".c") -CHECKED_EXTENSIONS = SOURCE_EXTENSIONS + (".h", ".hh", ".hpp") +CHECKED_EXTENSIONS = (".cpp", ".h") # Hunk header of a -U0 diff, e.g. "@@ -10,0 +11,3 @@": 3 lines added starting at line 11. HUNK_HEADER = re.compile(r"^@@ -\S+ \+(\d+)(?:,(\d+))? @@", re.MULTILINE) # e.g. "/path/File.cpp:1575:101: error: the parameter 'x' is copied ... [performance-unnecessary-value-param]"