Merge pull request #69 from ZedThree/tests

Add some basic tests; refactor project layout

.dockerignore

@@ -1,3 +1,2 @@
 **/venv
 **/__pycache__
-.git

.github/workflows/black.yml

@@ -28,7 +28,7 @@ jobs:
         black --version
     - name: Run black
       run: |
-        black review.py
+        black .
     - uses: stefanzweifel/git-auto-commit-action@v4
       with:
         commit_message: "Apply black changes"

.github/workflows/test.yml

@@ -0,0 +1,26 @@
+name: tests
+
+on:
+  push:
+    paths:
+      - '**.py'
+  pull_request:
+    paths:
+      - '**.py'
+
+jobs:
+  pytest:
+    runs-on: ubuntu-latest
+
+    steps:
+    - uses: actions/checkout@v3
+    - name: Set up Python
+      uses: actions/setup-python@v4
+      with:
+        python-version: "3.10"
+    - name: Install dependencies
+      run: |
+        python -m pip install --upgrade pip
+        pip install post/clang_tidy_review[tests]
+    - name: Test with pytest
+      run: "pytest -v"

.gitignore

@@ -1,5 +1,163 @@
+# Byte-compiled / optimized / DLL files
 __pycache__/
-venv/
+*.py[cod]
+*$py.class
+
+# C extensions
+*.so
+
+# Distribution / packaging
+.Python
+build/
+develop-eggs/
+dist/
+downloads/
+eggs/
+.eggs/
+lib/
+lib64/
+parts/
+sdist/
+var/
+wheels/
+share/python-wheels/
+*.egg-info/
+.installed.cfg
+*.egg
+MANIFEST
+
+# PyInstaller
+#  Usually these files are written by a python script from a template
+#  before PyInstaller builds the exe, so as to inject date/other infos into it.
+*.manifest
+*.spec
+
+# Installer logs
+pip-log.txt
+pip-delete-this-directory.txt
+
+# Unit test / coverage reports
+htmlcov/
+.tox/
+.nox/
+.coverage
+.coverage.*
+.cache
+nosetests.xml
+coverage.xml
+*.cover
+*.py,cover
+.hypothesis/
+.pytest_cache/
+cover/
+
+# Translations
+*.mo
+*.pot
+
+# Django stuff:
+*.log
+local_settings.py
+db.sqlite3
+db.sqlite3-journal
+
+# Flask stuff:
+instance/
+.webassets-cache
+
+# Scrapy stuff:
+.scrapy
+
+# Sphinx documentation
+docs/_build/
+
+# PyBuilder
+.pybuilder/
+target/
+
+# Jupyter Notebook
+.ipynb_checkpoints
+
+# IPython
+profile_default/
+ipython_config.py
+
+# pyenv
+#   For a library or package, you might want to ignore these files since the code is
+#   intended to run in multiple environments; otherwise, check them in:
+# .python-version
+
+# pipenv
+#   According to pypa/pipenv#598, it is recommended to include Pipfile.lock in version control.
+#   However, in case of collaboration, if having platform-specific dependencies or dependencies
+#   having no cross-platform support, pipenv may install dependencies that don't work, or not
+#   install all needed dependencies.
+#Pipfile.lock
+
+# poetry
+#   Similar to Pipfile.lock, it is generally recommended to include poetry.lock in version control.
+#   This is especially recommended for binary packages to ensure reproducibility, and is more
+#   commonly ignored for libraries.
+#   https://python-poetry.org/docs/basic-usage/#commit-your-poetrylock-file-to-version-control
+#poetry.lock
+
+# pdm
+#   Similar to Pipfile.lock, it is generally recommended to include pdm.lock in version control.
+#pdm.lock
+#   pdm stores project-wide configurations in .pdm.toml, but it is recommended to not include it
+#   in version control.
+#   https://pdm.fming.dev/#use-with-ide
+.pdm.toml
+
+# PEP 582; used by e.g. github.com/David-OConnor/pyflow and github.com/pdm-project/pdm
+__pypackages__/
+
+# Celery stuff
+celerybeat-schedule
+celerybeat.pid
+
+# SageMath parsed files
+*.sage.py
+
+# Environments
+.env
+.venv
+env/
+venv*/
+ENV/
+env.bak/
+venv.bak/
+
+# Spyder project settings
+.spyderproject
+.spyproject
+
+# Rope project settings
+.ropeproject
+
+# mkdocs documentation
+/site
+
+# mypy
+.mypy_cache/
+.dmypy.json
+dmypy.json
+
+# Pyre type checker
+.pyre/
+
+# pytype static type analyzer
+.pytype/
+
+# Cython debug symbols
+cython_debug/
+
+# PyCharm
+#  JetBrains specific template is maintained in a separate JetBrains.gitignore that can
+#  be found at https://github.com/github/gitignore/blob/main/Global/JetBrains.gitignore
+#  and can be added to the global gitignore or merged into this file.  For a more nuclear
+#  option (not recommended) you can uncomment the following to ignore the entire idea folder.
+#.idea/
 
 # Generated by review action
 clang-tidy-review-output.json

Dockerfile

@@ -1,7 +1,5 @@
 FROM ubuntu:22.04
 
-COPY requirements.txt /requirements.txt
-
 RUN apt update && \
     DEBIAN_FRONTEND=noninteractive \
     apt-get install -y --no-install-recommends\
@@ -11,17 +9,13 @@ RUN apt update && \
     clang-tidy-12 \
     clang-tidy-13 \
     clang-tidy-14 \
-    python3 python3-pip && \
-    pip3 install --upgrade pip && \
-    pip3 install -r requirements.txt && \
-    rm -rf /var/lib/apt/lists/
-
-WORKDIR /action
+    python3 \
+    python3-pip \
+    && rm -rf /var/lib/apt/lists/
 
-COPY review.py /action/review.py
+COPY . .git /clang_tidy_review/
 
-# Include the entirety of the post directory for simplicity's sake
-# Technically we only need the clang_tidy_review directory but this keeps things consistent for running the command locally during development and in the docker image
-COPY post /action/post
+RUN python3 -m pip install --upgrade pip && \
+    python3 -m pip install /clang_tidy_review/post/clang_tidy_review
 
-ENTRYPOINT ["/action/review.py"]
+ENTRYPOINT ["review"]

README.md

@@ -267,8 +267,44 @@ jobs:
       - uses: ZedThree/clang-tidy-review/[email protected]
 ```
 
-The lint workflow runs with limited permissions, while the post comments workflow has the required permissions because it's triggered by the `workflow_run` event.  
-Read more about workflow security limitations [here](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/).
+The lint workflow runs with limited permissions, while the post
+comments workflow has the required permissions because it's triggered
+by the `workflow_run` event.
+
+Read more about workflow security limitations
+[here](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/).
+
+## Project layout
+
+This project is laid out as follows:
+
+```
+.
+├── action.yml       # The `review` Action
+├── Dockerfile
+└── post
+    ├── action.yml   # The `post` Action
+    ├── Dockerfile
+    └── clang_tidy_review      # Common python package
+        └── clang_tidy_review
+            ├── __init__.py
+            ├── post.py        # Entry point for `post`
+            └── review.py      # Entry point for `review`
+```
+
+In order to accommodate the split workflow, the `review` and `post`
+actions must have their own Action metadata files. GitHub requires
+this file to be named exactly `action.yml`, so they have to be in
+separate directories. The associated `Dockerfile`s must also be named
+exactly `Dockerfile`, so they also have to be separate directories.
+
+Lastly, we want to be able to reuse the python package between the two
+Actions, which means it must be in a subdirectory of _both_
+`Dockerfile`s because they can't see parent directories.
+
+Which is why we've ended up with this slightly strange structure! This
+way, we can `COPY` the python package into both Docker images.
+
 
 ## Real world project samples
 |Project|Workflow|

post/Dockerfile

@@ -1,13 +1,8 @@
 FROM python:3
 
-COPY requirements.txt /requirements.txt
+COPY clang_tidy_review /clang_tidy_review
 
 RUN pip3 install --upgrade pip && \
-    pip3 install -r requirements.txt
+    pip3 install /clang_tidy_review
 
-WORKDIR /action
-
-COPY post.py /action/post.py
-COPY clang_tidy_review /action/clang_tidy_review
-
-ENTRYPOINT ["/action/post.py"]
+ENTRYPOINT ["post"]

post/clang_tidy_review/__init__.py → post/clang_tidy_review/clang_tidy_review/__init__.py

@@ -3,7 +3,6 @@
 # SPDX-License-Identifier: MIT
 # See LICENSE for more information
 
-import argparse
 import fnmatch
 import itertools
 import json
@@ -247,7 +246,6 @@ def make_file_offset_lookup(filenames):
 
 
 def get_diagnostic_file_path(clang_tidy_diagnostic, build_dir):
-
     # Sometimes, clang-tidy gives us an absolute path, so everything is fine.
     # Sometimes however it gives us a relative path that is realtive to the
     # build directory, so we prepend that.
@@ -484,6 +482,33 @@ def try_relative(path):
         return pathlib.Path(path).resolve()
 
 
+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)
+
+
 def format_notes(notes, offset_lookup):
     """Format an array of notes into a single string"""
 
@@ -622,6 +647,19 @@ def create_review_file(
     return review
 
 
+def filter_files(diff, include: List[str], exclude: List[str]) -> List:
+    changed_files = [filename.target_file[2:] for filename in diff]
+    files = []
+    for pattern in include:
+        files.extend(fnmatch.filter(changed_files, pattern))
+        print(f"include: {pattern}, file list now: {files}")
+    for pattern in exclude:
+        files = [f for f in files if not fnmatch.fnmatch(f, pattern)]
+        print(f"exclude: {pattern}, file list now: {files}")
+
+    return files
+
+
 def create_review(
     pull_request: PullRequest,
     build_dir: str,
@@ -639,14 +677,7 @@ def create_review(
     diff = pull_request.get_pr_diff()
     print(f"\nDiff from GitHub PR:\n{diff}\n")
 
-    changed_files = [filename.target_file[2:] for filename in diff]
-    files = []
-    for pattern in include:
-        files.extend(fnmatch.filter(changed_files, pattern))
-        print(f"include: {pattern}, file list now: {files}")
-    for pattern in exclude:
-        files = [f for f in files if not fnmatch.fnmatch(f, pattern)]
-        print(f"exclude: {pattern}, file list now: {files}")
+    files = filter_files(diff, include, exclude)
 
     if files == []:
         print("No files to check!")
@@ -802,6 +833,7 @@ def set_output(key: str, val: str) -> bool:
 
     return True
 
+
 def post_review(
     pull_request: PullRequest,
     review: Optional[PRReview],