Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add base_dir argument to more accurately fix absolute paths #26

Merged
merged 4 commits into from
Feb 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 94 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
`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
Expand All @@ -86,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'
Expand All @@ -95,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
5 changes: 5 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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 }}'
Expand Down
71 changes: 38 additions & 33 deletions review.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import json
import os
from operator import itemgetter
import pathlib
import pprint
import re
import requests
Expand Down Expand Up @@ -81,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)],
}
Expand Down Expand Up @@ -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"
Expand All @@ -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",
Expand Down Expand Up @@ -399,44 +432,16 @@ def strip_enclosing_quotes(string: str) -> str:

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)

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,
Expand Down