Run pre-commit checks in CI (including the Xen-Bugtool Test Environment)

Add pre-commit to run in the GitHub Workflow on each push and PR update:

Many Python packages use the `pre-commit` Python Framework for sanity
checks when updating Python projects.

Additionally, you can run it locally for faster fixing of issues using:
```sh
$ pip3 install pre-commit -r requirements-dev.txt
$ pre-commit run
```

The most popular checks are:
- Don't commit to the master branch (only commit to private branches)
- Fix not adding trailing spaces to commits
- Fix terminating files with one trailing newline(and not more)
- Fix using conistent LF for line-endings, not a mix with CR/LF.
- Remove unused variables and unused imports and sort imports
- Apply consistent code formatting for the project (using black)
- Check that debug statements are not accitendially merged
- Check that executables have shebangs and scripts are exectuable
- Check that merge conflicts are properly resolved
- Check YAML syntax

These are also fast to run for small projects like this:
- Run quick tests (requires `pip3 install -r requirements-dev.txt)
- Run mypy to e.g. check that expected function arguments are passed
- Run pylint to check that developers added docstrings, etc

Add these checks to GitHub CI and fix the issues which needed fixing,
like missing docstrings.

Developers can also elect to run checks locally during development
See the header of the config file `.pre-commit-config.yaml` for details
how to install it locally.

Signed-off-by: Bernhard Kaindl <[email protected]>

.github/workflows/main.yml

@@ -34,3 +34,20 @@ jobs:
         run: python2 -m pylint xen-bugtool
       - name: Run python2 -m pytest to execute all unit and integration tests
         run: python2 -m pytest -v -rA
+      # https://www.python4data.science/en/latest/productive/git/advanced/hooks/ci.html
+      - uses: actions/setup-python@v4
+        with:
+          python-version: '3.11'
+          cache: 'pip'
+      - run: pip install -r requirements-dev.txt
+        name: Install the pytest dependencies for running the pytest suite using Python3
+      - uses: actions/cache@v3
+        name: Setup cache for running pre-commit fast
+        with:
+          path: ~/.cache/pre-commit
+          key: pre-commit|${{ env.pythonLocation }}|${{ hashFiles('.pre-commit-config.yaml') }}
+      - uses: pre-commit/[email protected]
+        name: Run pre-commit checks
+        env:
+          # Skip the no-commit-to-branch check inside of GitHub CI (for CI on merge to master)
+          SKIP: no-commit-to-branch

.pre-commit-config.yaml

@@ -0,0 +1,105 @@
+#
+# This is the configuration file of the pre-commit framework for this repository:
+# https://pypi.org/project/pre-commit
+#
+# pre-commit runs in the GitHub Workflow of this project on each push and PR.
+# Additionally, you can run it locally for faster fixing of issues using
+# $ pip3 install pre-commit -r requirements-dev.txt
+#
+# On the initial run, pre-commit downloads its checkers, subsequent runs are fast:
+#
+# $ pre-commit run    # automatically checks which checks are needed.
+# $ pre-commit run -a # runs all fixes and checks, even when not needed
+#
+# When this works, you can enable it as the pre-commit hook for this git clone:
+# $ pre-commit install
+#
+# You can skip checks if you commit very often you don't want them to run, e.g:
+# export SKIP=mypy,pylint;git commit -m "quick save" (or for --amend)
+#
+# For more customisation, see https://pre-commit.com/#temporarily-disabling-hooks
+# and https://pre-commit.com/#confining-hooks-to-run-at-certain-stages (e.g push)
+#
+# After this, the pre-commit fixes and checks run when you commit an update.
+#
+# You can also automatically set pre-commit as pre-commit hook for new git clones:
+# $ git config --global init.templateDir ~/.git-template
+# $ pre-commit init-templatedir ~/.git-template
+#
+# Further information:
+# https://pre-commit.com/#automatically-enabling-pre-commit-on-repositories
+# All hooks: https://pre-commit.com/hooks.html
+fail_fast: true
+default_stages: [commit, push]
+repos:
+-   repo: https://github.com/pre-commit/pre-commit-hooks
+    rev: v4.5.0
+    # https://github.com/pre-commit/pre-commit-hooks/blob/main/README.md:
+    hooks:
+    -   id: no-commit-to-branch
+        name: "ensure that you don't commit to the local master branch"
+        args: [--branch, master]
+        always_run: true
+    -   id: trailing-whitespace
+        name: 'check and fix files to have no trailing whitespace'
+    -   id: end-of-file-fixer
+        name: 'check and fix that files have a trailing newline'
+        exclude: tests/integration/sar-file-collection.test.sh
+    -   id: mixed-line-ending
+        args: ['--fix=lf']
+        name: 'check and fix that line endings are line feeds'
+    -   id: check-added-large-files
+        args: ['--maxkb=12']
+        name: 'check that no large files are added'
+    -   id: check-executables-have-shebangs
+    -   id: debug-statements
+        name: 'check for debugger imports and breakpoint calls'
+    -   id: check-shebang-scripts-are-executable
+    -   id: check-merge-conflict
+    -   id: check-yaml
+        name: 'check the syntax of yaml files'
+# Run "python3 -m pip install -r requirements-dev.txt" to run pytest or use "git commit --no-verify":
+-   repo: local
+    hooks:
+    -   id: pytest
+        name: check that the Xen-Bugtool Test Environment passes
+        entry: env PYTHONDEVMODE=yes python3 -m pytest
+        pass_filenames: false
+        language: system
+        types: [python]
+-   repo: https://github.com/PyCQA/autoflake
+    rev: v2.2.1
+    hooks:
+    -   id: autoflake
+        name: Run autoflake to automatically remove unused variables and imports
+        args: ["--in-place", "--remove-unused-variables", "--remove-all-unused-imports"]
+        language: python
+        files: \.py$
+-   repo: https://github.com/psf/black
+    rev: 23.11.0
+    hooks:
+    -   id: black
+        name: Ensure that all files (excluding xen-bugtool itself) are black-formatted
+        args: [--safe, --quiet]
+        exclude: xen-bugtool
+-   repo: https://github.com/akaihola/darker
+    rev: 1.7.2
+    hooks:
+    -   id: darker
+        name: Ensure that changes staged for updating xen-bugtool are black-formatted
+        files: xen-bugtool
+        args: [--isort, -tpy36]
+        additional_dependencies:
+          - isort
+-   repo: https://github.com/pre-commit/mirrors-mypy
+    rev: v1.7.0
+    hooks:
+    -   id: mypy
+        name: Run mypy to check e.g. that all expected arguments are passed to functions etc
+        additional_dependencies: [defusedxml, pytest, types-lxml]
+-   repo: https://github.com/pycqa/pylint
+    rev: v3.0.2
+    hooks:
+    -   id: pylint
+        name: Run pylint to check that docstrings are added (and all other enabled checks)
+        log_file: ".git/pre-commit-pylint.log"

.pylintrc

@@ -105,7 +105,8 @@ max-returns=11
 
 # Maximum number of statements in function / method body.
 # defaults to: max-statements=50
-max-statements=353
+# xen-bugtool:main() is currently at 376, but it has a dedicated pylint disable comment now:
+# max-statements=376
 
 [FORMAT]
 
@@ -160,10 +161,6 @@ disable=anomalous-backslash-in-string,
         invalid-name,
         invalid-name,
         len-as-condition,
-        line-too-long,
-        missing-docstring,
-        missing-function-docstring,
-        multiple-statements,
         no-else-break,
         no-else-return,
         no-name-in-module,
@@ -185,6 +182,8 @@ disable=anomalous-backslash-in-string,
         use-set-for-membership,
         # Skip complaining about checkers only present in older pylint for Python2
         useless-option-value,
+        # Skip complaining about checkers only present in newest pylint for Python3
+        bad-option-value,
         wrong-import-order,
         # Py2 compat: Python2 requires calls to super() to have it's arguments:
         super-with-arguments,

pyproject.toml

@@ -60,3 +60,44 @@ combine_as_imports = true
 ensure_newline_before_comments = false
 # extra standard libraries of Py2:
 extra_standard_library = "commands"
+
+[tool.mypy]
+files = ["xen-bugtool", "tests"]
+pretty = true
+error_summary = true
+strict_equality = true
+show_error_codes = true
+show_error_context = true
+# Check the contents of untyped functions in all modules by default:
+check_untyped_defs = true
+scripts_are_modules = true
+python_version = "3.11"
+warn_return_any = true
+warn_unreachable = true
+warn_unused_configs = true
+warn_redundant_casts = true
+disallow_any_explicit = true
+disallow_any_generics = true
+disallow_any_unimported = true
+disallow_subclassing_any = true
+
+[[tool.mypy.overrides]]
+module = ["xen-bugtool"]
+# Disabled for now, may get fixed by later changes:
+disable_error_code = [
+    "union-attr",
+    "arg-type",
+    "attr-defined",
+    "name-defined",
+    "import-not-found",
+    "import-untyped",
+    "operator",
+    "var-annotated",
+    "assignment",
+    "misc",
+    "no-any-unimported",
+    "no-untyped-call",
+    "no-untyped-def",
+    "str-format", # Will be easy to fix
+    "unreachable"
+]

tests/integration/conftest.py

@@ -6,7 +6,7 @@
 import pytest
 
 from namespace_container import activate_private_test_namespace, mount, umount
-from utils import BUGTOOL_DOM0_TEMPL, run
+from utils import BUGTOOL_DOM0_TEMPL, BUGTOOL_OUTPUT_DIR, run
 
 
 @pytest.fixture(autouse=True, scope="session")
@@ -25,14 +25,15 @@ def output_archive_type(request):
 
 @pytest.fixture(autouse=True, scope="function")
 def run_test_functions_with_private_tmpfs_output_directory():
-    """Run each test function with a private bugtool output directory using tmpfs"""
+    """Generator fixture to run each test function with a private bugtool output directory using tmpfs"""
     # Works in conjunction of having entered a private test namespace for the entire pytest session before:
     mount(target="/var", fs="tmpfs", options="size=128M")
     # To provide test files below /var, subdirectores can be bind-mounted/created here
     # (or the tmpfs mount above could be done on BUGTOOL_OUTPUT_DIR)
     # run_bugtool_entry() will chdir to the output directory, so change back afterwards:
     srcdir = os.getcwd()
     yield
+    os.chdir(BUGTOOL_OUTPUT_DIR)
     # Assert that the test case did not leave any unchecked output fileas in the output directory:
     remaining_files = []
     for currentpath, _, files in os.walk("."):
@@ -41,8 +42,11 @@ def run_test_functions_with_private_tmpfs_output_directory():
     if remaining_files:
         print("Remaining (possibly unchecked) files found:")
         print(remaining_files)
+        os.chdir(BUGTOOL_OUTPUT_DIR)
         run(["find", "-type", "f"])
+        os.chdir(srcdir)
         print("Ensure that these files are checked, remove them when checked.")
+        umount("/var")
         raise RuntimeError("Remaining (possibly unchecked) files found. Run 'pytest -rF' for logs")
     os.chdir(srcdir)
     umount("/var")

tests/integration/namespace_container.py

@@ -2,7 +2,6 @@
 import ctypes
 import os
 
-from utils import run
 
 CLONE_NEWUSER = 0x10000000
 CLONE_NEWNET = 0x40000000

tests/integration/utils.py

@@ -12,7 +12,7 @@
 from lxml import etree
 
 if sys.version_info.major == 2:
-    from commands import getoutput  # pyright: ignore[reportMissingImports]
+    from commands import getoutput  # type:ignore[import-not-found] # pyright: ignore[reportMissingImports]
 else:
     from subprocess import getoutput
 
@@ -27,7 +27,7 @@ def run(command):
     returncode = process.wait()
     print("# " + " ".join(command) + ":")
     if returncode:
-        raise Exception(returncode, stderr)
+        raise RuntimeError(returncode, stderr)
     print(stdout, stderr)
     return stdout, stderr
 
@@ -60,6 +60,27 @@ def verify_content_from_dom0_template(path):
         shutil.rmtree(path)
 
 
+def extract(zip_or_tar_archive, archive_type):
+    """Extract a passed zip, tar or tar.bz2 archive into the current working directory"""
+    if sys.version_info > (3, 0):
+        if archive_type == "zip" and os.environ.get("GITHUB_ACTION"):
+            run(["unzip", zip_or_tar_archive])  # GitHub's Py3 is missing cp437 for unpack_archive()
+        else:
+            shutil.unpack_archive(zip_or_tar_archive)
+    else:  # Python2.7 does not have shutil.unpack_archive():
+        if archive_type == "zip":
+            archive = zipfile.ZipFile(zip_or_tar_archive)  # type: zipfile.ZipFile|tarfile.TarFile
+        elif archive_type == "tar":
+            archive = tarfile.open(zip_or_tar_archive)
+        elif archive_type == "tar.bz2":
+            archive = tarfile.open(zip_or_tar_archive, "r:bz2")
+        else:
+            raise RuntimeError("Unsupported output archive type: %s" % archive_type)
+        archive.extractall()
+        archive.close()
+    os.unlink(zip_or_tar_archive)
+
+
 def run_bugtool_entry(archive_type, test_entries):
     """Run bugtool for the given entry or entries, extract the output, and chdir to it"""
     os.environ["XENRT_BUGTOOL_BASENAME"] = test_entries
@@ -71,17 +92,7 @@ def run_bugtool_entry(archive_type, test_entries):
     os.chdir(BUGTOOL_OUTPUT_DIR)
     output_file = test_entries + "." + archive_type
     print("# Unpacking " + BUGTOOL_OUTPUT_DIR + output_file + " and verifying inventory.xml")
-    # Python2.7 does not have shutil.unpack_archive():
-    # shutil.unpack_archive(output_file):
-    if archive_type == "zip":
-        archive = zipfile.ZipFile(output_file)
-    elif archive_type == "tar":
-        archive = tarfile.open(output_file)
-    elif archive_type == "tar.bz2":
-        archive = tarfile.open(output_file, "r:bz2")
-    else:
-        raise RuntimeError("Unsupported output archive type: %s" % archive_type)
-    archive.extractall()
+    extract(output_file, archive_type)
     os.chdir(test_entries)
     # Validate the extracted inventory.xml using the XML schema from the test framework:
     with open(srcdir + "/tests/integration/inventory.xsd") as xmlschema:

tests/mocks/xen/lowlevel/xc/__init__.py

@@ -1,11 +1,13 @@
 """Mock xen.lowlevel.xc for testing the xen-bugtool python application"""
 Error = None
 
+
 class xc:
+    """Mock xc class as a stand-in for the real xen.lowlevel.xc which only works in a real Xen Dom0"""
+
     def __init__(self):
-        pass
-        # print("Mock xen.lowlevel.xc.xc() instantiated.")
+        """Mock for the constructor of xen.lowlevel.xc.xc() constructor"""
 
     def domain_getinfo(self):
-        # print("Mock xen.lowlevel.xc.xc().domain_getinfo() called.")
+        """Mock for xen.lowlevel.xc.xc().domain_getinfo()"""
         return []

tests/unit/test_xapidb_filter.py

@@ -1,3 +1,4 @@
+"""tests/unit/test_xapidb_filter.py: Ensure that the xen-bugtool.DBFilter() filters the XAPI DB properly"""
 # This uses the deprecated imp module because it has to run with Python2.7 for now:
 import os
 import sys
@@ -54,7 +55,7 @@
 """
 
 
-@pytest.mark.skipif(sys.version_info >= (3,0), reason="requires python2")
+@pytest.mark.skipif(sys.version_info >= (3, 0), reason="requires python2")
 def test_xapi_database_filter():
     """Assert that bugtool.DBFilter().output() filters the xAPI database as expected"""
     import imp  # pylint: disable=deprecated-module  # pyright: ignore[reportMissingImports]

xen-bugtool

@@ -15,7 +15,6 @@
 #
 # Copyright (c) 2005, 2007 XenSource Ltd.
 
-
 #
 # To add new entries to the bugtool, you need to:
 #
@@ -33,6 +32,13 @@
 # or func_output().
 #
 
+# Special pylint disables for latest pylint on xen-bugtool itself (for the moment):
+# The old Pylint-1.9.x thinks some of these are useless:
+# pylint: disable=useless-suppression
+# pylint: disable=missing-docstring,line-too-long,multiple-statements,unnecessary-pass
+# pylint: disable=broad-exception-raised,missing-type-doc,useless-object-inheritance
+# pylint: disable=undefined-variable,unnecessary-comprehension,super-init-not-called
+
 from __future__ import print_function
 
 import fcntl
@@ -65,7 +71,7 @@ import defusedxml.sax
 
 if sys.version_info.major == 2:
     from commands import getoutput  # pyright: ignore[reportMissingImports]
-    from urllib import urlopen
+    from urllib import urlopen  # type:ignore[attr-defined]
 else:
     from subprocess import getoutput
     from urllib.request import urlopen
@@ -614,7 +620,7 @@ Capture information to help diagnose bugs.
  -d, --debug         enable debug output
  --help              this help'''
 
-def main(argv = None):
+def main(argv = None):  # pylint: disable=too-many-statements
     global ANSWER_YES_TO_ALL, SILENT_MODE
     global entries, dbg
     global unlimited_data, unlimited_time