diff --git a/.clang-tidy b/.clang-tidy index 41c13ba845..aaf40c7448 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -8,7 +8,7 @@ Checks: > -misc-non-private-member-variables-in-classes, -misc-no-recursion, -misc-use-anonymous-namespace, - readability-identifier-naming' + readability-identifier-naming, -misc-const-correctness, bugprone-argument-comment, bugprone-assert-side-effect, diff --git a/.github/workflows/buildAndTest.yml b/.github/workflows/buildAndTest.yml index 5524e6dae7..bd0a99daf7 100644 --- a/.github/workflows/buildAndTest.yml +++ b/.github/workflows/buildAndTest.yml @@ -8,13 +8,6 @@ on: types: [assigned, opened, synchronize, reopened] workflow_dispatch: -defaults: - run: - # This is already the default, except when running inside another Docker - # image, which is the case here. So set it up globally to avoid - # repeating elsewhere. - shell: bash - env: # Run apt package manager in the CI in non-interactive mode. # Otherwise, on Ubuntu 20.04 the installation of tzdata asking question @@ -53,11 +46,8 @@ jobs: run: | pip install cmake numpy psutil pybind11 rich pkginfo lit PyYAML - - name: Install Ninja - run: sudo apt-get install -y ninja-build - - - name: Install lld and clang (for aiecc) - run: sudo apt-get install -y clang lld + - name: Install packages + run: sudo apt-get install -y ninja-build clang lld - name: Get MLIR id: mlir-wheels @@ -68,7 +58,6 @@ jobs: echo "MLIR_DIR=$PWD/mlir" | tee -a $GITHUB_OUTPUT - name: Ccache for C++ compilation - # https://github.com/hendrikmuhs/ccache-action/releases/tag/v1.2.9 uses: hendrikmuhs/ccache-action@ca3acd2731eef11f1572ccb126356c2f9298d35e with: # Since there are now several compilation jobs running in parallel, @@ -139,133 +128,221 @@ jobs: ninja check-tutorials ninja check-reference-designs - lint-repo: - name: Check code format - runs-on: ubuntu-20.04 + clang-tidy-pylint: + + name: Python and C/C++ Lint + + runs-on: ubuntu-22.04 + + permissions: + contents: write + pull-requests: write + steps: - # We'll be running clang-tidy later in this flow. + - uses: actions/checkout@v3 + with: + fetch-depth: 2 + submodules: "true" + - name: Install clang-tidy run: | sudo apt-get update - sudo apt-get install -y clang-tidy-12 - sudo update-alternatives --install /usr/bin/clang-tidy clang-tidy \ - /usr/bin/clang-tidy-12 100 + sudo apt-get install -y clang-tidy ninja-build clang - # Clone the repo and its submodules. Do shallow clone to save clone - # time. - - name: Get repo - uses: actions/checkout@v3 + - uses: actions/setup-python@v4 with: - fetch-depth: 2 - submodules: "true" + python-version: '3.9' - # -------- - # Lint the code. - # ------- - - # Choose the git commit to diff against for the purposes of linting. - # Since this workflow is triggered on both pushes and pull requests, we - # have to determine if the pull request target branch is set (which it - # will only be on the PR triggered flow). If it's not, then compare - # against the last commit. - - name: choose-commit - if: always() - env: - # Base ref is the target branch, in text form (not hash) - PR_BASE: ${{ github.base_ref }} + - name: Install Python packages + run: | + pip install cmake numpy psutil pybind11 rich pkginfo lit PyYAML requests + + - name: Get MLIR + id: mlir-wheels + run: | + pip -q download mlir -f https://github.com/Xilinx/mlir-aie/releases/expanded_assets/mlir-distro && unzip -q mlir-*.whl + echo "MLIR_DIR=$PWD/mlir" | tee -a $GITHUB_OUTPUT + + - name: Prepare compile_commands.json + run: | + mkdir build + pushd build + cmake .. \ + -GNinja \ + -DCMAKE_BUILD_TYPE=Release \ + -DCMAKE_C_COMPILER=clang \ + -DCMAKE_CXX_COMPILER=clang++ \ + -DCMAKE_PLATFORM_NO_VERSIONED_SONAME=ON \ + -DCMAKE_VISIBILITY_INLINES_HIDDEN=ON \ + -DCMAKE_C_VISIBILITY_PRESET=hidden \ + -DCMAKE_CXX_VISIBILITY_PRESET=hidden \ + -DAIE_COMPILER=NONE \ + -DAIE_LINKER=NONE \ + -DHOST_COMPILER=NONE \ + -DLLVM_ENABLE_ASSERTIONS=ON \ + -DLLVM_ENABLE_RTTI=ON \ + -DCMAKE_MODULE_PATH=`pwd`/../cmake/modulesXilinx \ + -DMLIR_DIR=${{ steps.mlir-wheels.outputs.MLIR_DIR }}/lib/cmake/mlir \ + -DLLVM_DIR=${{ steps.mlir-wheels.outputs.MLIR_DIR }}/lib/cmake/llvm \ + -DLLVM_EXTERNAL_LIT=$(which lit) \ + -DCMAKE_EXPORT_COMPILE_COMMANDS=ON + + ninja aie-headers mlir-headers + + popd + + - name: Analyze run: | - # Run clang-format - if [[ -z "$PR_BASE" ]]; then - DIFF_COMMIT_NAME="HEAD^" - else - DIFF_COMMIT_NAME="$PR_BASE" - fi - echo "DIFF_COMMIT_NAME=$DIFF_COMMIT_NAME" >> $GITHUB_ENV - - # Since we did a shallow fetch for this repo, we must fetch the commit - # upon which we be diff'ing. The last step set the ref name in the - # $DIFF_COMMIT_NAME environment variable. When running the fetch, resolve - # it to the commit hash and pass that hash along to subsequent steps. - - name: git fetch base commit - continue-on-error: true + git fetch origin main + git diff -U0 origin/main | clang-tidy-diff -p1 -path build -export-fixes fixes.yml + + - name: Post clang-tidy requests + env: + GITHUB_PR_NUMBER: ${{ github.event.pull_request.number }} run: | - if [[ ! "$DIFF_COMMIT_NAME" == *"HEAD"* ]]; then - git fetch --recurse-submodules=no origin $DIFF_COMMIT_NAME - DIFF_COMMIT_SHA=$( git rev-parse origin/$DIFF_COMMIT_NAME ) - else - DIFF_COMMIT_SHA=$( git rev-parse $DIFF_COMMIT_NAME ) - fi - echo "DIFF_COMMIT=$DIFF_COMMIT_SHA" >> $GITHUB_ENV - - # Run 'git clang-format', comparing against the target commit hash. If - # clang-format fixed anything, fail and output a patch. - - name: clang-format - if: always() + PULL_REQUEST_ID="$(jq "if (.issue.number != null) then .issue.number else .number end" < "$GITHUB_EVENT_PATH")" + echo $PULL_REQUEST_ID + + GITHUB_TOKEN=${{ secrets.GITHUB_TOKEN }} python utils/git/clang_tidy_pr.py \ + --clang-tidy-fixes fixes.yml \ + --pull-request-id "$PULL_REQUEST_ID" \ + --repository "$GITHUB_REPOSITORY" \ + --repository-root "$PWD" \ + --request-changes "false" \ + --suggestions-per-comment 10 + + # You can feed the pylintrc path to ciborow/action-pylint + # but it doesn't seem to work. + - name: Copy pylintrcs run: | - # Run clang-format - git clang-format-12 $DIFF_COMMIT - git diff --ignore-submodules > clang-format.patch - if [ -s clang-format.patch ]; then - echo "Clang-format found formatting problems in the following " \ - "files. See diff in the clang-format.patch artifact." - git diff --ignore-submodules --name-only - git checkout . - exit 1 - fi - echo "Clang-format found no formatting problems" - exit 0 - - # Run clang-tidy against only the changes. The 'clang-tidy-diff' script - # does this if supplied with the diff. - - name: clang-tidy - if: always() + cp .pylintrc python/.pylintrc + cp .pylintrc test/python/.pylintrc + cp .pylintrc tutorials/.pylintrc + cp .pylintrc reference_designs/.pylintrc + cp .pylintrc utils/.pylintrc + + # You can just do this wholesale at the root (not use workdir) + # but it'll take too long because it picks up + # the stuff in the build directory. + - uses: dciborow/action-pylint@0.1.0 + env: + PYTHONPATH: build/python + with: + github_token: ${{ secrets.GITHUB_TOKEN }} + reporter: github-pr-review + # GitHub Status Check won't become failure with warning. + level: warning + filter_mode: diff_context + workdir: python + glob_pattern: "**/*.py" + + - uses: dciborow/action-pylint@0.1.0 + env: + PYTHONPATH: build/python + with: + github_token: ${{ secrets.GITHUB_TOKEN }} + reporter: github-pr-review + level: warning + filter_mode: diff_context + workdir: test/python + glob_pattern: "**/*.py" + + - uses: dciborow/action-pylint@0.1.0 + env: + PYTHONPATH: build/python + with: + github_token: ${{ secrets.GITHUB_TOKEN }} + reporter: github-pr-review + level: warning + filter_mode: diff_context + workdir: tutorials + glob_pattern: "**/*.py" + + - uses: dciborow/action-pylint@0.1.0 + env: + PYTHONPATH: build/python + with: + github_token: ${{ secrets.GITHUB_TOKEN }} + reporter: github-pr-review + level: warning + filter_mode: diff_context + workdir: reference_designs + glob_pattern: "**/*.py" + + - uses: dciborow/action-pylint@0.1.0 + env: + PYTHONPATH: build/python + with: + github_token: ${{ secrets.GITHUB_TOKEN }} + reporter: github-pr-review + level: warning + filter_mode: diff_context + workdir: utils + glob_pattern: "**/*.py" + + formatting: + + name: Check code format + + runs-on: ubuntu-latest + + permissions: + contents: write + pull-requests: write + + steps: + - name: Get the project repository + uses: actions/checkout@v3 + with: + fetch-depth: 2 + submodules: "true" + + - name: Install clang-format + uses: aminya/setup-cpp@v1 + with: + clangformat: 17.0.1 + + - name: Setup Python env + uses: actions/setup-python@v4 + with: + python-version: '3.11' + + - name: Install black + run: pip install black[jupyter] + + - name: Run git-clang-format + id: git-clang-format run: | - git diff -U0 $DIFF_COMMIT | \ - clang-tidy-diff-12.py -path build_assert -p1 -fix - git diff --ignore-submodules > clang-tidy.patch - if [ -s clang-tidy.patch ]; then - echo "Clang-tidy problems in the following files. " \ - "See diff in the clang-tidy.patch artifact." - git diff --ignore-submodules --name-only - git checkout . - exit 1 - fi - echo "Clang-tidy found no problems" - exit 0 - - # Upload the format and tidy patches to an artifact (zip'd) associated - # with the workflow run. Only run this on a failure. - - name: Upload format and tidy patches - uses: actions/upload-artifact@v3 - continue-on-error: true - if: failure() + git fetch origin main + # git clang-format returns an error if changes made? + git clang-format origin/main || true + + - name: Check C/C++ format + uses: reviewdog/action-suggester@v1 with: - name: clang-format-tidy-patches - path: clang-*.patch - - # Unfortunately, artifact uploads are always zips so display the diff as - # well to provide feedback at a glance. - - name: clang format and tidy patches display - if: failure() - continue-on-error: true + tool_name: clang-format + level: error + fail_on_error: true + cleanup: true + + - name: Run black format + if: success() || failure() + id: black-format run: | - # Display patches - if [ ! -z clang-format.patch ]; then - echo "Clang-format patch" - echo "================" - cat clang-format.patch - echo "================" - fi - if [ ! -z clang-tidy.patch ]; then - echo "Clang-tidy patch" - echo "================" - cat clang-tidy.patch - echo "================" - fi + black --exclude python/compiler/aiecc/main.py . || true + black -l 10000 python/compiler/aiecc/main.py || true + + - name: Check Python format + if: success() || failure() + uses: reviewdog/action-suggester@v1 + with: + tool_name: black + level: error + fail_on_error: true code-coverage: - name: Code Coverage + name: C/C++ test code coverage runs-on: ubuntu-latest @@ -289,10 +366,7 @@ jobs: pip install cmake numpy psutil pybind11 rich lit - name: Install Ninja - run: sudo apt-get install -y ninja-build - - - name: Install llvm-cov - run: sudo apt-get install -y clang lld llvm + run: sudo apt-get install -y ninja-build clang lld llvm - name: Get changed files id: changed-files @@ -310,7 +384,6 @@ jobs: - name: Ccache for C++ compilation if: steps.changed-files.outputs.changed-files != '' - # https://github.com/hendrikmuhs/ccache-action/releases/tag/v1.2.9 uses: hendrikmuhs/ccache-action@ca3acd2731eef11f1572ccb126356c2f9298d35e with: key: ${{ runner.os }}-${{ matrix.ubuntu_version }}-${{ steps.get-llvm-commit-hash.outputs.hash }}-code-cov @@ -375,3 +448,4 @@ jobs: comment-author: 'github-actions[bot]' body-path: /home/runner/work/mlir-aie/mlir-aie/build_release/report/index.html edit-mode: replace + diff --git a/.pylintrc b/.pylintrc new file mode 100644 index 0000000000..c4b1380cf1 --- /dev/null +++ b/.pylintrc @@ -0,0 +1,59 @@ +[MAIN] + +py-version=3.9 +suggestion-mode=yes + +[BASIC] + +argument-naming-style=snake_case +attr-naming-style=snake_case +bad-names=foo, + bar, + baz, + toto, + tutu, + tata + +class-attribute-naming-style=snake_case +class-const-naming-style=UPPER_CASE +class-naming-style=PascalCase +const-naming-style=UPPER_CASE +docstring-min-length=-1 +function-naming-style=snake_case +include-naming-hint=yes +inlinevar-naming-style=snake_case +method-naming-style=snake_case +module-naming-style=snake_case +variable-naming-style=snake_case + +[CLASSES] + +valid-classmethod-first-arg=cls +valid-metaclass-classmethod-first-arg=mcs + +[IMPORTS] + +allow-wildcard-with-all=no + +[SIMILARITIES] + +# Minimum lines number of a similarity. +min-similarity-lines=4 + +[STRING] + +# This flag controls whether inconsistent-quotes generates a warning when the +# character used as a quote delimiter is used inconsistently within a module. +check-quote-consistency=yes + +# This flag controls whether the implicit-str-concat should generate a warning +# on implicit string concatenation in sequences defined over several lines. +check-str-concat-over-line-jumps=yes + +[MESSAGES CONTROL] + +# C0116: Missing function or method docstring (missing-function-docstring) +# C0114: Missing module docstring (missing-module-docstring) +# E0402: Attempted relative import beyond top-level package (relative-beyond-top-level) +# C0115: Missing class docstring (missing-class-docstring) +disable=C0116,C0114,E0402,C0115 \ No newline at end of file diff --git a/lib/Dialect/AIE/IR/AIEDialect.cpp b/lib/Dialect/AIE/IR/AIEDialect.cpp index 584df98333..e4b9eefcbf 100644 --- a/lib/Dialect/AIE/IR/AIEDialect.cpp +++ b/lib/Dialect/AIE/IR/AIEDialect.cpp @@ -618,8 +618,8 @@ std::vector ObjectFifoLinkOp::getInputObjectFifos() { if (parent->hasTrait()) { for (auto sym : getFifoIns()) { auto name = dyn_cast(sym); - if (auto st = SymbolTable::lookupSymbolIn(parent, name); - st && isa(st)) + if (auto *st = SymbolTable::lookupSymbolIn(parent, name); + isa_and_nonnull(st)) inputObjFifos.push_back(dyn_cast(st)); } } @@ -634,8 +634,8 @@ std::vector ObjectFifoLinkOp::getOutputObjectFifos() { if (parent->hasTrait()) { for (auto sym : getFifoOuts()) { auto name = dyn_cast(sym); - if (auto st = SymbolTable::lookupSymbolIn(parent, name); - st && isa(st)) + if (auto *st = SymbolTable::lookupSymbolIn(parent, name); + isa_and_nonnull(st)) outputObjFifos.push_back(dyn_cast(st)); } } diff --git a/python/util.py b/python/util.py index 423baa7118..0fd8382125 100644 --- a/python/util.py +++ b/python/util.py @@ -53,13 +53,13 @@ def infer_mlir_type( if isinstance(py_val, bool): return T.bool() elif isinstance(py_val, int): - if -(2 ** 31) <= py_val < 2 ** 31: + if -(2**31) <= py_val < 2**31: return T.i32() - elif 2 ** 31 <= py_val < 2 ** 32: + elif 2**31 <= py_val < 2**32: return T.ui32() - elif -(2 ** 63) <= py_val < 2 ** 63: + elif -(2**63) <= py_val < 2**63: return T.i64() - elif 2 ** 63 <= py_val < 2 ** 64: + elif 2**63 <= py_val < 2**64: return T.ui64() else: raise RuntimeError(f"Nonrepresentable integer {py_val}.") diff --git a/utils/git/clang_tidy_pr.py b/utils/git/clang_tidy_pr.py new file mode 100644 index 0000000000..2d31ba0105 --- /dev/null +++ b/utils/git/clang_tidy_pr.py @@ -0,0 +1,626 @@ +#!/usr/bin/env python3 + +"""Runner of the 'pull request comments from Clang-Tidy reports' action""" + +import argparse +import difflib +import json +import os +import posixpath +import re +import sys +import time + +import requests +import yaml + + +def get_diff_lines_per_file(pr_files): + """Generates and returns a set of affected line numbers for each file that has been modified + by the processed PR""" + + def change_to_line_range(change): + split_change = change.split(",") + start = int(split_change[0]) + + if len(split_change) > 1: + size = int(split_change[1]) + else: + size = 1 + + return range(start, start + size) + + result = {} + + for pr_file in pr_files: + # Not all PR file metadata entries may contain a patch section + # For example, entries related to removed binary files may not contain it + if "patch" not in pr_file: + continue + + file_name = pr_file["filename"] + + # The result is something like ['@@ -101,8 +102,11 @@', '@@ -123,9 +127,7 @@'] + git_line_tags = re.findall(r"^@@ -.*? +.*? @@", pr_file["patch"], re.MULTILINE) + + # We need to get it to a state like this: ['102,11', '127,7'] + changes = [ + tag.replace("@@", "").strip().split()[1].replace("+", "") + for tag in git_line_tags + ] + + result[file_name] = set() + for lines in [change_to_line_range(change) for change in changes]: + for line in lines: + result[file_name].add(line) + + return result + + +def get_pull_request_files( + github_api_url, github_token, github_api_timeout, repo, pull_request_id +): + """Generator of GitHub metadata about files modified by the processed PR""" + + # Request a maximum of 100 pages (3000 files) + for page in range(1, 100): + url = f"{github_api_url}/repos/{repo}/pulls/{pull_request_id:d}/files?page={page:d}" + print(url, file=sys.stderr) + result = requests.get( + f"{github_api_url}/repos/{repo}/pulls/{pull_request_id:d}/files?page={page:d}", + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": f"token {github_token}", + }, + timeout=github_api_timeout, + ) + + print(f"{result.status_code=}") + assert result.status_code == requests.codes.ok # pylint: disable=no-member + + chunk = json.loads(result.text) + + if len(chunk) == 0: + break + + for item in chunk: + yield item + + +def get_pull_request_comments( + github_api_url, github_token, github_api_timeout, repo, pull_request_id +): + """Generator of GitHub metadata about comments to the processed PR""" + + # Request a maximum of 100 pages (3000 files) + for page in range(1, 101): + result = requests.get( + f"{github_api_url}/repos/{repo}/pulls/{pull_request_id:d}/comments?page={page:d}", + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": f"token {github_token}", + }, + timeout=github_api_timeout, + ) + + assert result.status_code == requests.codes.ok # pylint: disable=no-member + + chunk = json.loads(result.text) + + if len(chunk) == 0: + break + + for item in chunk: + yield item + + +def generate_review_comments( + clang_tidy_fixes, repository_root, diff_lines_per_file +): # pylint: disable=too-many-locals,too-many-branches,too-many-statements + """Generator of the Clang-Tidy review comments""" + + def get_line_by_offset(repository_root, file_path, offset): + # Clang-Tidy doesn't support multibyte encodings and measures offsets in bytes + with open(repository_root + file_path, encoding="latin_1") as file: + source_file = file.read() + + return source_file[:offset].count("\n") + 1 + + def calculate_replacements_diff(repository_root, file_path, replacements): + # Apply the replacements in reverse order so that subsequent offsets are not shifted + replacements.sort(key=lambda item: (-item["Offset"])) + + # Clang-Tidy doesn't support multibyte encodings and measures offsets in bytes + with open(repository_root + file_path, encoding="latin_1") as file: + source_file = file.read() + + changed_file = source_file + + for replacement in replacements: + changed_file = ( + changed_file[: replacement["Offset"]] + + replacement["ReplacementText"] + + changed_file[replacement["Offset"] + replacement["Length"] :] + ) + + # Create and return the diff between the original version of the file and the version + # with the applied replacements + return difflib.Differ().compare( + source_file.splitlines(keepends=True), + changed_file.splitlines(keepends=True), + ) + + def markdown(s): + md_chars = "\\`*_{}[]<>()#+-.!|" + + def escape_chars(s): + for ch in md_chars: + s = s.replace(ch, "\\" + ch) + + return s + + def unescape_chars(s): + for ch in md_chars: + s = s.replace("\\" + ch, ch) + + return s + + # Escape markdown characters + s = escape_chars(s) + # Decorate quoted symbols as code + s = re.sub( + "'([^']*)'", lambda match: "`` " + unescape_chars(match.group(1)) + " ``", s + ) + + return s + + def generate_single_comment( + file_path, start_line_num, end_line_num, name, message, replacement_text=None + ): # pylint: disable=too-many-arguments + result = { + "path": file_path, + "line": end_line_num, + "side": "RIGHT", + "body": ":warning: **" + + markdown(name) + + "** :warning:\n" + + markdown(message), + } + + if start_line_num != end_line_num: + result["start_line"] = start_line_num + result["start_side"] = "RIGHT" + + if replacement_text is not None: + # Make sure the code suggestion ends with a newline character + if not replacement_text or replacement_text[-1] != "\n": + replacement_text += "\n" + + result["body"] += f"\n```suggestion\n{replacement_text}```" + + return result + + for diag in clang_tidy_fixes[ # pylint: disable=too-many-nested-blocks + "Diagnostics" + ]: + # If we have a Clang-Tidy 8 format, then upconvert it to the Clang-Tidy 9+ + if "DiagnosticMessage" not in diag: + diag["DiagnosticMessage"] = { + "FileOffset": diag["FileOffset"], + "FilePath": diag["FilePath"], + "Message": diag["Message"], + "Replacements": diag["Replacements"], + } + + diag_message = diag["DiagnosticMessage"] + if not diag_message["FilePath"]: + continue + + # Normalize paths + diag_message["FilePath"] = posixpath.normpath( + diag_message["FilePath"].replace(repository_root, "") + ) + for replacement in diag_message["Replacements"]: + replacement["FilePath"] = posixpath.normpath( + replacement["FilePath"].replace(repository_root, "") + ) + + diag_name = diag["DiagnosticName"] + diag_message_msg = diag_message["Message"] + + if not diag_message["Replacements"]: + file_path = diag_message["FilePath"] + offset = diag_message["FileOffset"] + line_num = get_line_by_offset(repository_root, file_path, offset) + + print(f"Processing '{diag_name}' at line {line_num:d} of {file_path}...") + + if ( + file_path in diff_lines_per_file + and line_num in diff_lines_per_file[file_path] + ): + yield generate_single_comment( + file_path, line_num, line_num, diag_name, diag_message_msg + ) + else: + print("This warning does not apply to the lines changed in this PR") + else: + diag_message_replacements = diag_message["Replacements"] + + for file_path in {item["FilePath"] for item in diag_message_replacements}: + line_num = 1 + start_line_num = None + end_line_num = None + replacement_text = None + + for line in calculate_replacements_diff( + repository_root, + file_path, + [ + item + for item in diag_message_replacements + if item["FilePath"] == file_path + ], + ): + # The comment line in the diff, ignore it + if line.startswith("? "): + continue + + # A string belonging only to the original version is the beginning or + # continuation of the section of the file that should be replaced + if line.startswith("- "): + if start_line_num is None: + assert end_line_num is None + + start_line_num = line_num + end_line_num = line_num + else: + assert end_line_num is not None + + end_line_num = line_num + + if replacement_text is None: + replacement_text = "" + + line_num += 1 + # A string belonging only to the modified version is part of the + # replacement text + elif line.startswith("+ "): + if replacement_text is None: + replacement_text = line[2:] + else: + replacement_text += line[2:] + # A string belonging to both original and modified versions is the + # end of the section to replace + elif line.startswith(" "): + if replacement_text is not None: + # If there is a replacement text, but there is no information about + # the section to replace, then this is not a replacement, but a pure + # addition of text. Add the current line to the end of the replacement + # text and "replace" it with the replacement text. + if start_line_num is None: + assert end_line_num is None + + start_line_num = line_num + end_line_num = line_num + replacement_text += line[2:] + else: + assert end_line_num is not None + + print( + # pylint: disable=line-too-long + f"Processing '{diag_name}' at lines {start_line_num:d}-{end_line_num:d} of {file_path}..." + ) + + if ( + file_path in diff_lines_per_file + and start_line_num in diff_lines_per_file[file_path] + ): + yield generate_single_comment( + file_path, + start_line_num, + end_line_num, + diag_name, + diag_message_msg, + replacement_text, + ) + else: + print( + "This warning does not apply to the lines changed in this PR" + ) + + start_line_num = None + end_line_num = None + replacement_text = None + + line_num += 1 + # Unknown prefix, this should not happen + else: + assert False, "Please report this to the repository maintainer" + + # The end of the file is reached, but there is a section to replace + if replacement_text is not None: + # Pure addition of text to the end of the file is not currently supported. If + # you have an example of a Clang-Tidy replacement of this kind, please contact + # the repository maintainer. + assert ( + start_line_num is not None and end_line_num is not None + ), "Please report this to the repository maintainer" + + print( + # pylint: disable=line-too-long + f"Processing '{diag_name}' at lines {start_line_num:d}-{end_line_num:d} of {file_path}..." + ) + + if ( + file_path in diff_lines_per_file + and start_line_num in diff_lines_per_file[file_path] + ): + yield generate_single_comment( + file_path, + start_line_num, + end_line_num, + diag_name, + diag_message_msg, + replacement_text, + ) + else: + print( + "This warning does not apply to the lines changed in this PR" + ) + + +def post_review_comments( + github_api_url, + github_token, + github_api_timeout, + repo, + pull_request_id, + warning_comment_prefix, + review_event, + review_comments, + suggestions_per_comment, +): # pylint: disable=too-many-arguments + """Sending the Clang-Tidy review comments to GitHub""" + + def split_into_chunks(lst, n): + # Copied from: https://stackoverflow.com/a/312464 + """Yield successive n-sized chunks from lst.""" + for i in range(0, len(lst), n): + yield lst[i : i + n] + + # Split the comments in chunks to avoid overloading the server + # and getting 502 server errors as a response for large reviews + review_comments = list(split_into_chunks(review_comments, suggestions_per_comment)) + + total_reviews = len(review_comments) + current_review = 1 + + for comments_chunk in review_comments: + warning_comment = ( + warning_comment_prefix + f" ({current_review:d}/{total_reviews:d})" + ) + current_review += 1 + + result = requests.post( + f"{github_api_url}/repos/{repo}/pulls/{pull_request_id:d}/reviews", + json={ + "body": warning_comment, + "event": review_event, + "comments": comments_chunk, + }, + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": f"token {github_token}", + }, + timeout=github_api_timeout, + ) + + # Ignore bad gateway errors (false negatives?) + assert result.status_code in ( + requests.codes.ok, # pylint: disable=no-member + requests.codes.bad_gateway, # pylint: disable=no-member + ) + + # Avoid triggering abuse detection + time.sleep(10) + + +def dismiss_change_requests( + github_api_url, + github_token, + github_api_timeout, + repo, + pull_request_id, + warning_comment_prefix, +): # pylint: disable=too-many-arguments + """Dismissing stale Clang-Tidy requests for changes""" + + print("Checking if there are any stale requests for changes to dismiss...") + + result = requests.get( + f"{github_api_url}/repos/{repo}/pulls/{pull_request_id:d}/reviews", + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": f"token {github_token}", + }, + timeout=github_api_timeout, + ) + + assert result.status_code == requests.codes.ok # pylint: disable=no-member + + reviews = json.loads(result.text) + + # Dismiss only our own reviews + reviews_to_dismiss = [ + review["id"] + for review in reviews + if review["state"] == "CHANGES_REQUESTED" + and warning_comment_prefix in review["body"] + and review["user"]["login"] == "github-actions[bot]" + ] + + for review_id in reviews_to_dismiss: + print(f"Dismissing review {review_id:d}") + + result = requests.put( + # pylint: disable=line-too-long + f"{github_api_url}/repos/{repo}/pulls/{pull_request_id:d}/reviews/{review_id:d}/dismissals", + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": f"token {github_token}", + }, + json={ + "message": "No Clang-Tidy warnings found so I assume my comments were addressed", + "event": "DISMISS", + }, + timeout=github_api_timeout, + ) + + assert result.status_code == requests.codes.ok # pylint: disable=no-member + + # Avoid triggering abuse detection + time.sleep(10) + + +def main(): + """Entry point""" + + parser = argparse.ArgumentParser( + description="Runner of the 'pull request comments from Clang-Tidy reports' action" + ) + parser.add_argument( + "--clang-tidy-fixes", + type=str, + required=True, + help="Path to the Clang-Tidy fixes YAML", + ) + parser.add_argument( + "--pull-request-id", + type=int, + required=True, + help="Pull request ID", + ) + parser.add_argument( + "--repository", + type=str, + required=True, + help="Name of the repository containing the code", + ) + parser.add_argument( + "--repository-root", + type=str, + required=True, + help="Path to the root of the repository containing the code", + ) + parser.add_argument( + "--request-changes", + type=str, + required=True, + help="If 'true', then request changes if there are warnings, otherwise leave a comment", + ) + parser.add_argument( + "--suggestions-per-comment", + type=int, + required=True, + help="Number of suggestions per comment", + ) + + args = parser.parse_args() + + # The GitHub API token is sensitive information, pass it through the environment + github_token = os.environ.get("GITHUB_TOKEN") + + github_api_url = os.environ.get("GITHUB_API_URL") + github_api_timeout = 10 + + warning_comment_prefix = ( + "# :warning: `Clang-Tidy` found issue(s) with the introduced code" + ) + + diff_lines_per_file = get_diff_lines_per_file( + get_pull_request_files( + github_api_url, + github_token, + github_api_timeout, + args.repository, + args.pull_request_id, + ) + ) + + with open(args.clang_tidy_fixes, encoding="utf_8") as file: + clang_tidy_fixes = yaml.safe_load(file) + + if ( + clang_tidy_fixes is None + or "Diagnostics" not in clang_tidy_fixes.keys() + or len(clang_tidy_fixes["Diagnostics"]) == 0 + ): + print("No warnings found by Clang-Tidy") + dismiss_change_requests( + github_api_url, + github_token, + github_api_timeout, + args.repository, + args.pull_request_id, + warning_comment_prefix, + ) + return 0 + + review_comments = list( + generate_review_comments( + clang_tidy_fixes, args.repository_root + "/", diff_lines_per_file + ) + ) + + existing_pull_request_comments = list( + get_pull_request_comments( + github_api_url, + github_token, + github_api_timeout, + args.repository, + args.pull_request_id, + ) + ) + + # Exclude already posted comments + for comment in existing_pull_request_comments: + review_comments = list( + filter( + lambda review_comment: not ( + review_comment["path"] + == comment["path"] # pylint: disable=cell-var-from-loop + and review_comment["line"] + == comment["line"] # pylint: disable=cell-var-from-loop + and review_comment["side"] + == comment["side"] # pylint: disable=cell-var-from-loop + and review_comment["body"] + == comment["body"] # pylint: disable=cell-var-from-loop + ), + review_comments, + ) + ) + + if not review_comments: + print("No new warnings found by Clang-Tidy") + return 0 + + print(f"Clang-Tidy found {len(review_comments):d} new warning(s)") + + post_review_comments( + github_api_url, + github_token, + github_api_timeout, + args.repository, + args.pull_request_id, + warning_comment_prefix, + "REQUEST_CHANGES" if args.request_changes == "true" else "COMMENT", + review_comments, + args.suggestions_per_comment, + ) + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/utils/requirements.txt b/utils/requirements.txt index 6ce53a69db..c6160d72d7 100644 --- a/utils/requirements.txt +++ b/utils/requirements.txt @@ -3,3 +3,4 @@ rich pybind11 numpy cmake +pylint \ No newline at end of file