diff --git a/.clang-tidy b/.clang-tidy index 1385849bb..d903d7d04 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -17,6 +17,7 @@ Checks: > -cppcoreguidelines-pro-bounds-pointer-arithmetic, -cppcoreguidelines-macro-usage, -cppcoreguidelines-non-private-member-variables-in-classes, + -llvm-header-guard, # Use scripts/fix_header_guards.py instead misc-*, -misc-non-private-member-variables-in-classes, -misc-no-recursion, diff --git a/.github/actions/handle-fix-commit/action.yaml b/.github/actions/handle-fix-commit/action.yaml index 5e03209e7..c3f39b3f9 100644 --- a/.github/actions/handle-fix-commit/action.yaml +++ b/.github/actions/handle-fix-commit/action.yaml @@ -55,14 +55,14 @@ runs: fi - name: No changes to apply - if: steps.check_changes.outputs.changes == 'false' + if: steps.check_changes.outputs.changes == 'false' && github.event.pull_request.number uses: thollander/actions-comment-pull-request@24bffb9b452ba05a4f3f77933840a6a841d1b32b # v3.0.1 with: message: 'No automatic ${{ inputs.tool }} fixes were necessary.' - name: Get PR maintainer_can_modify property id: pr-properties - if: steps.check_changes.outputs.changes == 'true' + if: steps.check_changes.outputs.changes == 'true' && github.event.pull_request.number uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 with: script: | @@ -123,7 +123,7 @@ runs: exit 1 - name: Notify of commit - if: steps.commit_and_push.conclusion == 'success' && steps.commit_and_push.outputs.pushed == 'true' + if: steps.commit_and_push.conclusion == 'success' && steps.commit_and_push.outputs.pushed == 'true' && github.event.pull_request.number uses: thollander/actions-comment-pull-request@24bffb9b452ba05a4f3f77933840a6a841d1b32b # v3.0.1 with: message: | @@ -141,13 +141,13 @@ runs: - name: Upload patch if: steps.create_patch.outputs.patch_name - uses: actions/upload-artifact@330a01c490aca151604b8cf639adc76d48f6c5d4 # v5.0.0 + uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6.0.0 with: name: fix-patch path: ${{ inputs.working-directory }}/${{ steps.create_patch.outputs.patch_name }} - name: Comment with patch instructions - if: steps.create_patch.outputs.patch_name + if: steps.create_patch.outputs.patch_name && github.event.pull_request.number uses: thollander/actions-comment-pull-request@24bffb9b452ba05a4f3f77933840a6a841d1b32b # v3.0.1 with: message: | diff --git a/.github/workflows/clang-tidy-check.yaml b/.github/workflows/clang-tidy-check.yaml index 20ffece94..22661c678 100644 --- a/.github/workflows/clang-tidy-check.yaml +++ b/.github/workflows/clang-tidy-check.yaml @@ -3,6 +3,7 @@ name: Clang-Tidy Check permissions: contents: read + pull-requests: read on: @@ -121,7 +122,7 @@ jobs: build-type: Debug extra-options: "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_CXX_SCAN_FOR_MODULES=OFF -DCMAKE_CXX_CLANG_TIDY='clang-tidy;--export-fixes=clang-tidy-fixes.yaml'" - - name: Run clang-tidy using CMake target + - name: Run clang-tidy using CMake id: tidy shell: bash run: | @@ -130,7 +131,7 @@ jobs: echo "➡️ Running clang-tidy checks..." cmake_status=0 - cmake --build . -j $(nproc) > clang-tidy.log 2>&1 || cmake_status=$? + cmake --build . -j "$(nproc)" > clang-tidy.log 2>&1 || cmake_status=$? # Distinguish tooling failures from issue detection by checking log content if [ "$cmake_status" -ne 0 ]; then diff --git a/.github/workflows/clang-tidy-fix.yaml b/.github/workflows/clang-tidy-fix.yaml index 316b87b7b..89f219cf2 100644 --- a/.github/workflows/clang-tidy-fix.yaml +++ b/.github/workflows/clang-tidy-fix.yaml @@ -76,36 +76,82 @@ jobs: repository: ${{ needs.pre-check.outputs.repo }} token: ${{ secrets.WORKFLOW_PAT }} + - name: Try to download existing fixes from check workflow + id: download_fixes_check + if: needs.pre-check.outputs.tidy_checks == '' + uses: dawidd6/action-download-artifact@fe9d59ce33ce92db8a6ac90b2c8be6b6d90417c8 # v15 + with: + workflow: clang-tidy-check.yaml + name: clang-tidy-report + path: fixes + branch: ${{ needs.pre-check.outputs.ref }} + continue-on-error: true + + - name: Try to download existing fixes from previous fix workflow + id: download_fixes_fix + if: steps.download_fixes_check.outcome != 'success' && needs.pre-check.outputs.tidy_checks == '' + uses: dawidd6/action-download-artifact@fe9d59ce33ce92db8a6ac90b2c8be6b6d90417c8 # v15 + with: + workflow: clang-tidy-fix.yaml + name: clang-tidy-report + path: fixes + branch: ${{ needs.pre-check.outputs.ref }} + continue-on-error: true + + - name: Apply fixes from artifact if available + id: apply_from_artifact + if: needs.pre-check.outputs.tidy_checks == '' && (steps.download_fixes_check.outcome == 'success' || steps.download_fixes_fix.outcome == 'success') + run: | + if [ -f fixes/clang-tidy-fixes.yaml ]; then + echo "Applying fixes from existing artifact..." + cd phlex-src + clang-apply-replacements ../fixes || true + echo "applied=true" >> "$GITHUB_OUTPUT" + else + echo "applied=false" >> "$GITHUB_OUTPUT" + fi + - name: Setup build environment + if: steps.apply_from_artifact.outputs.applied != 'true' uses: Framework-R-D/phlex/.github/actions/setup-build-env@main - - name: Configure CMake for clang-tidy + - name: Prepare CMake configuration options + if: steps.apply_from_artifact.outputs.applied != 'true' + id: prep_tidy_opts env: TIDY_CHECKS: ${{ needs.pre-check.outputs.tidy_checks }} run: | . /entrypoint.sh cd "$GITHUB_WORKSPACE" - CLANG_TIDY_OPTS="clang-tidy;--fix;--export-fixes=clang-tidy-fixes.yaml" + CLANG_TIDY_OPTS="clang-tidy;--export-fixes=clang-tidy-fixes.yaml" if [ -n "$TIDY_CHECKS" ]; then CHECKS_NORMALIZED=$(echo "$TIDY_CHECKS" | tr ' ' ',') CLANG_TIDY_OPTS="${CLANG_TIDY_OPTS};--checks=-*,${CHECKS_NORMALIZED}" fi + echo "clang_tidy_opts=${CLANG_TIDY_OPTS}" >> "$GITHUB_OUTPUT" - cmake -B phlex-build -S phlex-src \ - -DCMAKE_BUILD_TYPE=Debug \ - -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \ - -DCMAKE_CXX_SCAN_FOR_MODULES=OFF \ - -DCMAKE_CXX_CLANG_TIDY="${CLANG_TIDY_OPTS}" + - name: Configure CMake (Debug) + if: steps.apply_from_artifact.outputs.applied != 'true' + uses: Framework-R-D/phlex/.github/actions/configure-cmake@main + with: + build-type: Debug + extra-options: "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_CXX_SCAN_FOR_MODULES=OFF -DCMAKE_CXX_CLANG_TIDY='${{ steps.prep_tidy_opts.outputs.clang_tidy_opts }}'" - - name: Apply clang-tidy fixes using CMake build + - name: Generate clang-tidy fixes using CMake build + if: steps.apply_from_artifact.outputs.applied != 'true' run: | . /entrypoint.sh cd "$GITHUB_WORKSPACE/phlex-build" + cmake --build . -j "$(nproc)" || true - echo "Applying clang-tidy fixes..." - cmake --build . -j $(nproc) || true - echo "Clang-tidy fixes applied" + - name: Apply clang-tidy fixes + if: steps.apply_from_artifact.outputs.applied != 'true' + run: | + cd "$GITHUB_WORKSPACE/phlex-build" + if [ -f clang-tidy-fixes.yaml ]; then + clang-apply-replacements . || true + fi - name: Upload clang-tidy report if: always() @@ -114,6 +160,7 @@ jobs: name: clang-tidy-report path: phlex-build/clang-tidy-fixes.yaml retention-days: 7 + if-no-files-found: ignore - name: Handle fix commit uses: Framework-R-D/phlex/.github/actions/handle-fix-commit@main diff --git a/.github/workflows/header-guards-check.yaml b/.github/workflows/header-guards-check.yaml new file mode 100644 index 000000000..995c17555 --- /dev/null +++ b/.github/workflows/header-guards-check.yaml @@ -0,0 +1,163 @@ +name: Header Guards Check +run-name: "${{ github.actor }} checking header guards" + +permissions: + contents: read + pull-requests: read + +on: + pull_request: + workflow_dispatch: + inputs: + ref: + description: "The branch, ref, or SHA to checkout. Defaults to the repository's default branch." + required: false + type: string + workflow_call: + inputs: + checkout-path: + description: "Path to check out code to" + required: false + type: string + skip-relevance-check: + description: "Bypass relevance check" + required: false + type: boolean + default: false + pr-base-sha: + description: "Base SHA of the PR for relevance check" + required: false + type: string + pr-head-sha: + description: "Head SHA of the PR for relevance check" + required: false + type: string + ref: + description: "The branch, ref, or SHA to checkout" + required: false + type: string + repo: + description: "The repository to checkout from" + required: false + type: string + +env: + local_checkout_path: ${{ (github.event_name == 'workflow_call' && inputs.checkout-path) || format('{0}-src', github.event.repository.name) }} + +jobs: + pre-check: + runs-on: ubuntu-latest + outputs: + is_act: ${{ steps.detect_act.outputs.is_act }} + ref: ${{ (github.event_name == 'workflow_call' && inputs.ref) || (github.event_name == 'workflow_dispatch' && (github.event.inputs.ref || github.ref)) || github.sha }} + repo: ${{ (github.event_name == 'workflow_call' && inputs.repo) || github.repository }} + base_sha: ${{ (github.event_name == 'workflow_call' && inputs.pr-base-sha) || github.event.pull_request.base.sha || github.event.before }} + steps: + - name: Detect act environment + id: detect_act + uses: Framework-R-D/phlex/.github/actions/detect-act-env@main + + detect-changes: + needs: pre-check + if: > + needs.pre-check.result == 'success' && + github.event_name != 'workflow_dispatch' && + ( + github.event_name != 'workflow_call' || + (inputs.skip-relevance-check != 'true' && github.event.inputs == null && github.event.comment == null) + ) && + needs.pre-check.outputs.is_act != 'true' + runs-on: ubuntu-latest + permissions: + contents: read + packages: read + outputs: + has_changes: ${{ steps.filter.outputs.matched }} + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + fetch-depth: 0 + path: ${{ env.local_checkout_path }} + ref: ${{ needs.pre-check.outputs.ref }} + repository: ${{ needs.pre-check.outputs.repo }} + + - name: Detect relevant changes + id: filter + uses: Framework-R-D/phlex/.github/actions/detect-relevant-changes@main + with: + repo-path: ${{ env.local_checkout_path }} + base-ref: ${{ needs.pre-check.outputs.base_sha }} + head-ref: ${{ (github.event_name == 'workflow_call' && inputs.pr-head-sha) || needs.pre-check.outputs.ref }} + file-type: cpp + + - name: Report detection outcome + run: | + if [ "${{ steps.filter.outputs.matched }}" != "true" ]; then + echo "::notice::No header file changes detected; check will be skipped." + else + echo "::group::Header files" + printf '%s\n' "${{ steps.filter.outputs.matched_files }}" + echo "::endgroup::" + fi + + header-guards-check: + needs: [pre-check, detect-changes] + if: > + always() && + ( + needs.detect-changes.result == 'skipped' || + ( + needs.detect-changes.result == 'success' && + needs.detect-changes.outputs.has_changes == 'true' + ) + ) + runs-on: ubuntu-latest + permissions: + contents: read + + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ needs.pre-check.outputs.ref }} + path: ${{ env.local_checkout_path }} + repository: ${{ needs.pre-check.outputs.repo }} + + - name: Set up Python + uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + with: + python-version: '3.12' + + - name: Check header guards + id: check + working-directory: ${{ env.local_checkout_path }} + run: | + python3 scripts/fix_header_guards.py --check --root . phlex plugins form > check_output.txt 2>&1 || echo "has_issues=true" >> "$GITHUB_OUTPUT" + cat check_output.txt + + - name: Report results + if: always() && steps.check.outputs.has_issues == 'true' + run: | + echo "::error::Header guard check failed." + echo "::error::Comment '@${{ github.event.repository.name }}bot header-guards-fix' on the PR to auto-fix." + exit 1 + + header-guards-check-skipped: + needs: [pre-check, detect-changes] + if: > + needs.pre-check.result == 'success' && + github.event_name != 'workflow_dispatch' && + ( + github.event_name != 'workflow_call' || + (inputs.skip-relevance-check != 'true' && github.event.inputs == null && github.event.comment == null) + ) && + needs.pre-check.outputs.is_act != 'true' && + (needs.detect-changes.result == 'success' && needs.detect-changes.outputs.has_changes != 'true') + runs-on: ubuntu-latest + permissions: + contents: read + + steps: + - name: No relevant header changes detected + run: echo "::notice::No header file changes detected; header-guards-check skipped." diff --git a/.github/workflows/header-guards-fix.yaml b/.github/workflows/header-guards-fix.yaml new file mode 100644 index 000000000..25254ce36 --- /dev/null +++ b/.github/workflows/header-guards-fix.yaml @@ -0,0 +1,97 @@ +name: Header Guards Fix +run-name: "${{ github.actor }} fixing header guards" + +on: + issue_comment: + types: + - created + workflow_call: + inputs: + checkout-path: + description: "Path to check out code to" + required: false + type: string + ref: + description: "The branch, ref, or SHA to checkout" + required: true + type: string + repo: + description: "The repository to checkout from" + required: true + type: string + workflow_dispatch: + inputs: + ref: + description: "The branch, ref, or SHA to checkout. Defaults to the repository's default branch." + required: false + type: string + +permissions: + pull-requests: write + contents: write + +env: + local_checkout_path: ${{ (github.event_name == 'workflow_call' && inputs.checkout-path) || format('{0}-src', github.event.repository.name) }} + +jobs: + pre-check: + runs-on: ubuntu-latest + name: Parse command + if: > + github.event_name == 'workflow_dispatch' || + github.event_name == 'workflow_call' || + ( + github.event_name == 'issue_comment' && + github.event.issue.pull_request && + contains(fromJSON('["OWNER", "COLLABORATOR", "MEMBER"]'), github.event.comment.author_association) && + ( + startsWith(github.event.comment.body, format('@{0}bot format', github.event.repository.name)) || + startsWith(github.event.comment.body, format('@{0}bot header-guards-fix', github.event.repository.name)) + ) + ) + # Authorization: Only OWNER, COLLABORATOR, or MEMBER can trigger via comments. + # This covers repo owners, invited collaborators, and all org members. + # See .github/AUTHORIZATION_ANALYSIS.md for security rationale. + outputs: + ref: ${{ (github.event_name == 'workflow_call' && inputs.ref) || (github.event_name == 'workflow_dispatch' && (github.event.inputs.ref || github.ref)) || steps.get_pr.outputs.ref }} + repo: ${{ (github.event_name == 'workflow_call' && inputs.repo) || (github.event_name == 'workflow_dispatch' && github.repository) || steps.get_pr.outputs.repo }} + + steps: + - name: Get PR Info + if: github.event_name == 'issue_comment' + id: get_pr + uses: Framework-R-D/phlex/.github/actions/get-pr-info@main + + apply_fixes: + runs-on: ubuntu-latest + name: Apply fixes + needs: pre-check + if: needs.pre-check.result == 'success' + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + path: ${{ env.local_checkout_path }} + ref: ${{ needs.pre-check.outputs.ref }} + repository: ${{ needs.pre-check.outputs.repo }} + token: ${{ secrets.WORKFLOW_PAT }} + + - name: Set up Python + uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + with: + python-version: '3.12' + + - name: Fix header guards + working-directory: ${{ env.local_checkout_path }} + run: | + echo "Fixing header guards..." + python3 scripts/fix_header_guards.py --root . phlex plugins form || true + + - name: Handle fix commit + uses: Framework-R-D/phlex/.github/actions/handle-fix-commit@main + with: + tool: header-guards + working-directory: ${{ env.local_checkout_path }} + token: ${{ secrets.WORKFLOW_PAT }} + pr-info-ref: ${{ needs.pre-check.outputs.ref }} + pr-info-repo: ${{ needs.pre-check.outputs.repo }} diff --git a/CMakeLists.txt b/CMakeLists.txt index a5547316f..5e420734b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -57,7 +57,6 @@ set(CTEST_TEST_TIMEOUT 90 CACHE STRING "Per-test timeout (s) for CTest") # Make tools available FetchContent_MakeAvailable(Catch2 GSL mimicpp) -include(Modules/private/CreateClangTidyTargets.cmake) include(Modules/private/CreateCoverageTargets.cmake) option(ENABLE_TSAN "Enable Thread Sanitizer" OFF) @@ -203,7 +202,10 @@ if(BUILD_TESTING) endif() endif() -# Create clang-tidy targets -create_clang_tidy_targets() +# Report clang-tidy availability +if(CLANG_TIDY_EXECUTABLE) + message(STATUS "Clang-tidy available: ${CLANG_TIDY_EXECUTABLE}") + message(STATUS "Use -DCMAKE_CXX_CLANG_TIDY=clang-tidy to enable automatic checks during build") +endif() cet_cmake_config() diff --git a/Modules/private/CreateClangTidyTargets.cmake b/Modules/private/CreateClangTidyTargets.cmake deleted file mode 100644 index 139247b61..000000000 --- a/Modules/private/CreateClangTidyTargets.cmake +++ /dev/null @@ -1,22 +0,0 @@ -# CreateClangTidyTargets.cmake -# -# Adds clang-tidy-fix target for applying fixes. -# Note: clang-tidy-check is no longer needed - use CMAKE_CXX_CLANG_TIDY instead. - -include_guard() - -find_program(CLANG_TIDY_EXECUTABLE NAMES clang-tidy-20 clang-tidy-19 clang-tidy) - -function(create_clang_tidy_targets) - cmake_language(DEFER DIRECTORY "${PROJECT_SOURCE_DIR}" CALL _create_clang_tidy_targets_impl) -endfunction() - -function(_create_clang_tidy_targets_impl) - if(NOT CLANG_TIDY_EXECUTABLE) - message(STATUS "clang-tidy not found, skipping clang-tidy targets") - return() - endif() - - message(STATUS "Clang-tidy available: ${CLANG_TIDY_EXECUTABLE}") - message(STATUS "Use -DCMAKE_CXX_CLANG_TIDY=clang-tidy to enable automatic checks during build") -endfunction() diff --git a/scripts/fix_header_guards.py b/scripts/fix_header_guards.py new file mode 100755 index 000000000..ad8b5066f --- /dev/null +++ b/scripts/fix_header_guards.py @@ -0,0 +1,143 @@ +#!/usr/bin/env python3 +"""Check and fix header guards to enforce naming convention.""" + +import argparse +import re +import sys +from pathlib import Path + + +def compute_expected_guard(file_path: Path, root: Path) -> str: + """Compute expected guard macro: X_Y_HEADER_EXT.""" + rel = file_path.relative_to(root) + # X is the first subdirectory, Y is remaining path components + parts = [rel.parts[0].upper().replace("-", "_")] + if len(rel.parts) > 2: + parts.extend(p.upper().replace("-", "_") for p in rel.parts[1:-1]) + parts.extend([rel.stem.upper().replace("-", "_"), rel.suffix[1:].upper()]) + return "_".join(parts) + + +def check_header_guard(file_path: Path, root: Path) -> tuple[bool, str | None]: + """Check if header guard is correct. Returns (is_valid, expected_guard).""" + content = file_path.read_text() + lines = content.splitlines(keepends=True) + + if len(lines) < 3: + return True, None + + expected = compute_expected_guard(file_path, root) + + ifndef_idx = define_idx = None + ifndef_macro = define_macro = endif_macro = None + + for i in range(min(10, len(lines))): + if m := re.match(r"#ifndef\s+(\w+)\s*$", lines[i]): + ifndef_idx, ifndef_macro = i, m.group(1) + elif m := re.match(r"#define\s+(\w+)\s*$", lines[i]): + define_idx, define_macro = i, m.group(1) + break + + for i in range(len(lines) - 1, -1, -1): + line = lines[i].strip() + if line.startswith("#endif"): + if m := re.match(r"#endif\s*//\s*(\w+)\s*$", lines[i]): + endif_macro = m.group(1) + break + + if ifndef_idx is None or define_idx is None: + return True, None + + if ifndef_macro == define_macro == endif_macro == expected: + return True, None + + return False, expected + + +def fix_header_guard(file_path: Path, root: Path) -> bool: + """Fix header guard. Returns True if modified.""" + content = file_path.read_text() + lines = content.splitlines(keepends=True) + + if len(lines) < 3: + return False + + expected = compute_expected_guard(file_path, root) + + ifndef_idx = define_idx = endif_idx = None + + for i in range(min(10, len(lines))): + if re.match(r"#ifndef\s+\w+\s*$", lines[i]): + ifndef_idx = i + elif re.match(r"#define\s+\w+\s*$", lines[i]): + define_idx = i + break + + for i in range(len(lines) - 1, -1, -1): + line = lines[i].strip() + if line.startswith("#endif"): + endif_idx = i + break + + if ifndef_idx is None or define_idx is None: + return False + + modified = False + if lines[ifndef_idx] != f"#ifndef {expected}\n": + lines[ifndef_idx] = f"#ifndef {expected}\n" + modified = True + if lines[define_idx] != f"#define {expected}\n": + lines[define_idx] = f"#define {expected}\n" + modified = True + if endif_idx is not None and lines[endif_idx] != f"#endif // {expected}\n": + lines[endif_idx] = f"#endif // {expected}\n" + modified = True + + if modified: + file_path.write_text("".join(lines)) + return modified + + +def main() -> None: + """Check or fix header guards in C++ header files.""" + parser = argparse.ArgumentParser(description="Check/fix header guards") + parser.add_argument("paths", nargs="+", help="Files or directories") + parser.add_argument("--check", action="store_true", help="Check only") + parser.add_argument("--root", type=Path, default=Path.cwd(), help="Root path") + args = parser.parse_args() + + root = args.root.resolve() + bad_files = [] + + for arg in args.paths: + path = Path(arg).resolve() + files = [path] if path.is_file() else [*path.rglob("*.hpp"), *path.rglob("*.h")] + for f in files: + if f.suffix not in {".hpp", ".h"}: + continue + if args.check: + valid, expected = check_header_guard(f, root) + if not valid: + bad_files.append((f, expected)) + else: + if fix_header_guard(f, root): + bad_files.append((f, None)) + + if args.check: + if bad_files: + print(f"Found {len(bad_files)} files with incorrect guards:") + for f, expected in bad_files: + print(f" {f.relative_to(root)}: expected {expected}") + sys.exit(1) + print("All header guards are correct.") + else: + if bad_files: + print(f"Fixed {len(bad_files)} files:") + for f, _ in bad_files: + print(f" {f.relative_to(root)}") + else: + print("No header guards needed fixing.") + + +if __name__ == "__main__": + main()