From bd5d0816a39e289d35c20c22275f066e456c7cf4 Mon Sep 17 00:00:00 2001 From: Peter Hill Date: Wed, 2 Feb 2022 09:34:13 +0000 Subject: [PATCH 1/4] Add `base_dir` argument to more accurately fix absolute paths --- README.md | 3 +++ action.yml | 5 +++++ review.py | 64 +++++++++++++++++++++++++++++------------------------- 3 files changed, 42 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 8b24748..7da7494 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,9 @@ at once, so `clang-tidy-review` will only attempt to post the first should be relative to `GITHUB_WORKSPACE` (the default place where your repository is cloned) - default: `'.'` +- `base_dir`: Absolute path to initial working directory. Almost always + `GITHUB_WORKSPACE`. + - default: `GITHUB_WORKSPACE` - `clang_tidy_version`: Version of clang-tidy to use; one of 6.0, 7, 8, 9, 10, 11 - default: '11' - `clang_tidy_checks`: List of checks diff --git a/action.yml b/action.yml index 8afde12..1396ebc 100644 --- a/action.yml +++ b/action.yml @@ -13,6 +13,10 @@ inputs: description: 'Directory containing the compile_commands.json file' default: '.' required: false + base_dir: + description: 'Absolute path to initial working directory. Useful if generating `compile_commands.json` outside of the Action' + default: ${{ github.workspace }} + require: false clang_tidy_version: description: 'Version of clang-tidy to use; one of 6.0, 7, 8, 9, 10, 11, 12' default: '12' @@ -61,6 +65,7 @@ runs: - --repo=${{ inputs.repo }} - --pr=${{ inputs.pr }} - --build_dir=${{ inputs.build_dir }} + - --base_dir=${{ inputs.base_dir }} - --clang_tidy_checks=${{ inputs.clang_tidy_checks }} - --config_file=${{ inputs.config_file }} - --include='${{ inputs.include }}' diff --git a/review.py b/review.py index 81978a7..f32633f 100755 --- a/review.py +++ b/review.py @@ -13,6 +13,7 @@ import json import os from operator import itemgetter +import pathlib import pprint import re import requests @@ -323,6 +324,33 @@ def strip_enclosing_quotes(string: str) -> str: return stripped +def fix_absolute_paths(build_compile_commands, base_dir): + """Update absolute paths in compile_commands.json to new location, if + compile_commands.json was created outside the Actions container + """ + + basedir = pathlib.Path(base_dir).resolve() + newbasedir = pathlib.Path(".").resolve() + + if basedir == newbasedir: + return + + print(f"Found '{build_compile_commands}', updating absolute paths") + # We might need to change some absolute paths if we're inside + # a docker container + with open(build_compile_commands, "r") as f: + compile_commands = json.load(f) + + print(f"Replacing '{basedir}' with '{newbasedir}'", flush=True) + + modified_compile_commands = json.dumps(compile_commands).replace( + str(basedir), str(newbasedir) + ) + + with open(build_compile_commands, "w") as f: + f.write(modified_compile_commands) + + if __name__ == "__main__": parser = argparse.ArgumentParser( description="Create a review from clang-tidy warnings" @@ -335,6 +363,11 @@ def strip_enclosing_quotes(string: str) -> str: parser.add_argument( "--build_dir", help="Directory with compile_commands.json", default="." ) + parser.add_argument( + "--base_dir", + help="Absolute path of initial working directory if compile_commands.json generated outside of Action", + default=".", + ) parser.add_argument( "--clang_tidy_checks", help="checks argument", @@ -407,36 +440,7 @@ def strip_enclosing_quotes(string: str) -> str: subprocess.run(cmake_command, shell=True, check=True) elif os.path.exists(build_compile_commands): - print(f"Found '{build_compile_commands}', updating absolute paths") - # We might need to change some absolute paths if we're inside - # a docker container - with open(build_compile_commands, "r") as f: - compile_commands = json.load(f) - - original_directory = compile_commands[0]["directory"] - - # directory should either end with the build directory, - # unless it's '.', in which case use original directory - if original_directory.endswith(args.build_dir): - build_dir_index = -(len(args.build_dir) + 1) - basedir = original_directory[:build_dir_index] - elif args.build_dir == ".": - basedir = original_directory - else: - raise RuntimeError( - f"compile_commands.json contains absolute paths that I don't know how to deal with: '{original_directory}'" - ) - - newbasedir = os.getcwd() - - print(f"Replacing '{basedir}' with '{newbasedir}'", flush=True) - - modified_compile_commands = json.dumps(compile_commands).replace( - basedir, newbasedir - ) - - with open(build_compile_commands, "w") as f: - f.write(modified_compile_commands) + fix_absolute_paths(build_compile_commands, args.base_dir) main( repo=args.repo, From 8905227b924f27e3e5d564c3f576be906cd4d4f3 Mon Sep 17 00:00:00 2001 From: Peter Hill Date: Fri, 4 Feb 2022 08:54:40 +0000 Subject: [PATCH 2/4] Strip quotes from cmake command before checking if it's empty --- review.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/review.py b/review.py index f32633f..07f6643 100755 --- a/review.py +++ b/review.py @@ -432,10 +432,11 @@ def fix_absolute_paths(build_compile_commands, base_dir): build_compile_commands = f"{args.build_dir}/compile_commands.json" + cmake_command = strip_enclosing_quotes(args.cmake_command) + # If we run CMake as part of the action, then we know the paths in # the compile_commands.json file are going to be correct - if args.cmake_command: - cmake_command = strip_enclosing_quotes(args.cmake_command) + if cmake_command: with message_group(f"Running cmake: {cmake_command}"): subprocess.run(cmake_command, shell=True, check=True) From 30e8842291eefa95f1ef14d990f42a380985d1e3 Mon Sep 17 00:00:00 2001 From: Peter Hill Date: Fri, 4 Feb 2022 09:04:58 +0000 Subject: [PATCH 3/4] Fix trying to serialise pathlib.Path --- review.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/review.py b/review.py index 07f6643..53b44a9 100755 --- a/review.py +++ b/review.py @@ -82,7 +82,7 @@ def make_review(contents, lookup): try: comments.append( { - "path": rel_path, + "path": str(rel_path), "body": comment_body, "position": lookup[rel_path][int(source_line)], } From 7332ba695d55a20dd491471a8cb762a1c0b36815 Mon Sep 17 00:00:00 2001 From: Peter Hill Date: Mon, 7 Feb 2022 15:19:38 +0000 Subject: [PATCH 4/4] Add some instructions for how to set `base_dir` in README --- README.md | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 92 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 7da7494..91029ca 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ at once, so `clang-tidy-review` will only attempt to post the first should be relative to `GITHUB_WORKSPACE` (the default place where your repository is cloned) - default: `'.'` -- `base_dir`: Absolute path to initial working directory. Almost always +- `base_dir`: Absolute path to initial working directory `GITHUB_WORKSPACE`. - default: `GITHUB_WORKSPACE` - `clang_tidy_version`: Version of clang-tidy to use; one of 6.0, 7, 8, 9, 10, 11 @@ -89,7 +89,8 @@ at once, so `clang-tidy-review` will only attempt to post the first - `apt_packages`: Comma-separated list of apt packages to install - default: '' - `cmake_command`: A CMake command to configure your project and generate - `compile_commands.json` in `build_dir` + `compile_commands.json` in `build_dir`. You _almost certainly_ want + to include `-DCMAKE_EXPORT_COMPILE_COMMANDS=ON`! - default: '' - `max_comments`: Maximum number of comments to post at once - default: '25' @@ -98,8 +99,97 @@ at once, so `clang-tidy-review` will only attempt to post the first - `total_comments`: Total number of warnings from clang-tidy +## Generating `compile_commands.json` inside the container + +Very simple projects can get away without a `compile_commands.json` +file, but for most projects `clang-tidy` needs this file in order to +find include paths and macro definitions. + +If you use the GitHub `ubuntu-latest` image as your normal `runs-on` +container, you only install packages from the system package manager, +and don't need to build or install other tools yourself, then you can +generate `compile_commands.json` as part of the `clang-tidy-review` +action: + +```yaml +name: clang-tidy-review +on: [pull_request] + +jobs: + build: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v2 + + - uses: ZedThree/clang-tidy-review@v0.8.0 + id: review + with: + # List of packages to install + apt_packages: liblapack-dev + # CMake command to run in order to generate compile_commands.json + cmake_command: cmake . -DCMAKE_EXPORT_COMPILE_COMMANDS=on +``` + +If you don't use CMake, this may still work for you if you can use a +tool like [bear](https://github.com/rizsotto/Bear) for example. + +## Use in a non-default location + +If you're using the `container` argument in your GitHub workflow, +downloading/building other tools manually, or not using CMake, you +will need to generate `compile_commands.json` before the +`clang-tidy-review` action. However, the Action is run inside another +container, and due to the way GitHub Actions work, `clang-tidy-review` +ends up running with a different absolute path. + +What this means is that if `compile_commands.json` contains absolute +paths, `clang-tidy-review` needs to adjust them to where it is being +run instead. By default, it replaces absolute paths that start with +the value of [`${GITHUB_WORKSPACE}`][env_vars] with the new working +directory. + +If you're running in a container other than a default GitHub +container, then you may need to pass the working directory to +`base_dir`. Unfortunately there's not an easy way for +`clang-tidy-review` to auto-detect this, so in order to pass the +current directory you will need to do something like the following: + +```yaml +name: clang-tidy-review +on: [pull_request] + +jobs: + build: + runs-on: ubuntu-latest + # Using another container changes the + # working directory from GITHUB_WORKSPACE + container: + image: my-container + + steps: + - uses: actions/checkout@v2 + + # Get the current working directory and set it + # as an environment variable + - name: Set base_dir + run: echo "base_dir=$(pwd)" >> $GITHUB_ENV + + - uses: ZedThree/clang-tidy-review@v0.8.0 + id: review + with: + # Tell clang-tidy-review the base directory. + # This will get replaced by the new working + # directory inside the action + base_dir: ${{ env.base_dir }} +``` + ## Real world project samples |Project|Workflow| |----------|-------| |[BOUT++](https://github.com/boutproject/BOUT-dev) |[CMake](https://github.com/boutproject/BOUT-dev/blob/master/.github/workflows/clang-tidy-review.yml) | |[Mudlet](https://github.com/Mudlet/Mudlet) |[CMake + Qt](https://github.com/Mudlet/Mudlet/blob/development/.github/workflows/clangtidy-diff-analysis.yml) | + + + +[env_vars]: https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables