chore: add PyLint and fix most issues (#999)

* chore: add PyLint and fix most issues

* fix: minor fixes after first push

* chore: remove reference to old toml lib

* chore: minor options cleanup

* ci: only fail with many pylint issues, annotate in GHA

* refactor: address review

* ci: force nox to report in CI

* refactor: use map instead of if chain

* refactor: simpler shell_with_arch

* refactor: even simpler shell_with_arch

* Restore if...elif...else blocks where guard-style is not intended

Co-authored-by: Joe Rickerby <joerick@mac.com>

.github/matchers/pylint.json

@@ -0,0 +1,32 @@
+{
+  "problemMatcher": [
+    {
+      "severity": "warning",
+      "pattern": [
+        {
+          "regexp": "^([^:]+):(\\d+):(\\d+): ([A-DF-Z]\\d+): \\033\\[[\\d;]+m([^\\033]+).*$",
+          "file": 1,
+          "line": 2,
+          "column": 3,
+          "code": 4,
+          "message": 5
+        }
+      ],
+      "owner": "pylint-warning"
+    },
+    {
+      "severity": "error",
+      "pattern": [
+        {
+          "regexp": "^([^:]+):(\\d+):(\\d+): (E\\d+): \\033\\[[\\d;]+m([^\\033]+).*$",
+          "file": 1,
+          "line": 2,
+          "column": 3,
+          "code": 4,
+          "message": 5
+        }
+      ],
+      "owner": "pylint-error"
+    }
+  ]
+}

.github/workflows/test.yml

@@ -15,18 +15,23 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
-  pre-commit:
+  lint:
-    name: Pre-commit checks (mypy, flake8, etc.)
+    name: Linters (mypy, flake8, etc.)
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v2
     - uses: actions/setup-python@v2
     - uses: pre-commit/action@v2.0.3
-    - run: pipx run nox -s check_manifest
+    - name: Check manifest
+      run: pipx run nox -s check_manifest
+    - name: PyLint checks
+      run: |
+        echo "::add-matcher::$GITHUB_WORKSPACE/.github/matchers/pylint.json"
+        pipx run nox -s pylint
 
   test:
     name: Test cibuildwheel on ${{ matrix.os }}
-    needs: pre-commit
+    needs: lint
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
@@ -67,7 +72,7 @@ jobs:
 
   test-emulated:
     name: Test emulated cibuildwheel using qemu
-    needs: pre-commit
+    needs: lint
     runs-on: ubuntu-latest
     timeout-minutes: 180
     steps:

cibuildwheel/__main__.py

@@ -237,7 +237,7 @@ def print_preamble(platform: str, options: Options, identifiers: List[str]) -> N
 
     print(f"Cache folder: {CIBW_CACHE_PATH}")
 
-    warnings = detect_warnings(platform=platform, options=options, identifiers=identifiers)
+    warnings = detect_warnings(options=options, identifiers=identifiers)
     if warnings:
         print("\nWarnings:")
         for warning in warnings:
@@ -273,7 +273,7 @@ def get_build_identifiers(
     return [config.identifier for config in python_configurations]
 
 
-def detect_warnings(platform: str, options: Options, identifiers: List[str]) -> List[str]:
+def detect_warnings(*, options: Options, identifiers: List[str]) -> List[str]:
     warnings = []
 
     # warn about deprecated {python} and {pip}

cibuildwheel/architecture.py

@@ -73,20 +73,18 @@ class Architecture(Enum):
 
     @staticmethod
     def all_archs(platform: PlatformName) -> "Set[Architecture]":
-        if platform == "linux":
+        all_archs_map = {
-            return {
+            "linux": {
                 Architecture.x86_64,
                 Architecture.i686,
                 Architecture.aarch64,
                 Architecture.ppc64le,
                 Architecture.s390x,
+            },
+            "macos": {Architecture.x86_64, Architecture.arm64, Architecture.universal2},
+            "windows": {Architecture.x86, Architecture.AMD64, Architecture.ARM64},
         }
-        elif platform == "macos":
+        return all_archs_map[platform]
-            return {Architecture.x86_64, Architecture.arm64, Architecture.universal2}
-        elif platform == "windows":
-            return {Architecture.x86, Architecture.AMD64, Architecture.ARM64}
-        else:
-            assert_never(platform)
 
     @staticmethod
     def bitness_archs(platform: PlatformName, bitness: Literal["64", "32"]) -> "Set[Architecture]":

cibuildwheel/docker_container.py

@@ -108,7 +108,9 @@ class DockerContainer:
 
         assert isinstance(self.name, str)
 
-        subprocess.run(["docker", "rm", "--force", "-v", self.name], stdout=subprocess.DEVNULL)
+        subprocess.run(
+            ["docker", "rm", "--force", "-v", self.name], stdout=subprocess.DEVNULL, check=False
+        )
         self.name = None
 
     def copy_into(self, from_path: Path, to_path: PurePath) -> None:

cibuildwheel/environment.py

@@ -66,7 +66,7 @@ class EnvironmentAssignmentRaw:
     def __repr__(self) -> str:
         return f"{self.name}: {self.value}"
 
-    def evaluated_value(self, **kwargs: Any) -> str:
+    def evaluated_value(self, **_: Any) -> str:
         return self.value
 
 

cibuildwheel/functools_cached_property_38.py

@@ -0,0 +1,65 @@
+from threading import RLock
+from typing import Any, Callable, Generic, Optional, Type, TypeVar, overload
+
+__all__ = ["cached_property"]
+
+_NOT_FOUND = object()
+
+_T = TypeVar("_T")
+
+
+class cached_property(Generic[_T]):
+    def __init__(self, func: Callable[[Any], _T]):
+        self.func = func
+        self.attrname: Optional[str] = None
+        self.__doc__ = func.__doc__
+        self.lock = RLock()
+
+    def __set_name__(self, owner: Type[Any], name: str) -> None:
+        if self.attrname is None:
+            self.attrname = name
+        elif name != self.attrname:
+            raise TypeError(
+                "Cannot assign the same cached_property to two different names "
+                f"({self.attrname!r} and {name!r})."
+            )
+
+    @overload
+    def __get__(self, instance: None, owner: Optional[Type[Any]] = ...) -> "cached_property[_T]":
+        ...
+
+    @overload
+    def __get__(self, instance: object, owner: Optional[Type[Any]] = ...) -> _T:
+        ...
+
+    def __get__(self, instance: Optional[object], owner: Optional[Type[Any]] = None) -> Any:
+        if instance is None:
+            return self
+        if self.attrname is None:
+            raise TypeError(
+                "Cannot use cached_property instance without calling __set_name__ on it."
+            )
+        try:
+            cache = instance.__dict__
+        except AttributeError:  # not all objects have __dict__ (e.g. class defines slots)
+            msg = (
+                f"No '__dict__' attribute on {type(instance).__name__!r} "
+                f"instance to cache {self.attrname!r} property."
+            )
+            raise TypeError(msg) from None
+        val = cache.get(self.attrname, _NOT_FOUND)
+        if val is _NOT_FOUND:
+            with self.lock:
+                # check if another thread filled cache while we awaited lock
+                val = cache.get(self.attrname, _NOT_FOUND)
+                if val is _NOT_FOUND:
+                    val = self.func(instance)
+                    try:
+                        cache[self.attrname] = val
+                    except TypeError:
+                        msg = (
+                            f"The '__dict__' attribute on {type(instance).__name__!r} instance "
+                            f"does not support item assignment for caching {self.attrname!r} property."
+                        )
+                        raise TypeError(msg) from None
+        return val

cibuildwheel/linux.py

@@ -303,11 +303,11 @@ def build_on_docker(
     log.step_end()
 
 
-def build(options: Options, tmp_path: Path) -> None:
+def build(options: Options, tmp_path: Path) -> None:  # pylint: disable=unused-argument
     try:
         # check docker is installed
         subprocess.run(["docker", "--version"], check=True, stdout=subprocess.DEVNULL)
-    except Exception:
+    except subprocess.CalledProcessError:
         print(
             "cibuildwheel: Docker not found. Docker is required to run Linux builds. "
             "If you're building on Travis CI, add `services: [docker]` to your .travis.yml."

cibuildwheel/logger.py

@@ -158,7 +158,8 @@ class Logger:
             sys.stdout.flush()
             self.active_fold_group_name = None
 
-    def _fold_group_identifier(self, name: str) -> str:
+    @staticmethod
+    def _fold_group_identifier(name: str) -> str:
         """
         Travis doesn't like fold groups identifiers that have spaces in. This
         method converts them to ascii identifiers
@@ -174,17 +175,11 @@ class Logger:
 
     @property
     def colors(self) -> "Colors":
-        if self.colors_enabled:
+        return Colors(enabled=self.colors_enabled)
-            return Colors(enabled=True)
-        else:
-            return Colors(enabled=False)
 
     @property
     def symbols(self) -> "Symbols":
-        if self.unicode_enabled:
+        return Symbols(unicode=self.unicode_enabled)
-            return Symbols(unicode=True)
-        else:
-            return Symbols(unicode=False)
 
 
 def build_description_from_identifier(identifier: str) -> str:
@@ -263,8 +258,6 @@ def file_supports_unicode(file_obj: IO[AnyStr]) -> bool:
     return "utf" in codec_info.name
 
 
-"""
+# Global instance of the Logger.
-Global instance of the Logger.
-"""
 # (there's only one stdout per-process, so a global instance is justified)
 log = Logger()

cibuildwheel/macos.py

@@ -1,3 +1,4 @@
+import functools
 import os
 import platform
 import re
@@ -5,7 +6,7 @@ import shutil
 import subprocess
 import sys
 from pathlib import Path
-from typing import Any, Dict, List, NamedTuple, Sequence, Set, Tuple, cast
+from typing import Dict, List, NamedTuple, Sequence, Set, Tuple, cast
 
 from filelock import FileLock
 
@@ -108,7 +109,7 @@ def install_cpython(tmp: Path, version: str, url: str) -> Path:
     return installation_path / "bin" / "python3"
 
 
-def install_pypy(tmp: Path, version: str, url: str) -> Path:
+def install_pypy(tmp: Path, url: str) -> Path:
     pypy_tar_bz2 = url.rsplit("/", 1)[-1]
     extension = ".tar.bz2"
     assert pypy_tar_bz2.endswith(extension)
@@ -136,7 +137,7 @@ def setup_python(
     if implementation_id.startswith("cp"):
         base_python = install_cpython(tmp, python_configuration.version, python_configuration.url)
     elif implementation_id.startswith("pp"):
-        base_python = install_pypy(tmp, python_configuration.version, python_configuration.url)
+        base_python = install_pypy(tmp, python_configuration.url)
     else:
         raise ValueError("Unknown Python implementation")
     assert base_python.exists()
@@ -460,17 +461,12 @@ def build(options: Options, tmp_path: Path) -> None:
                             # rosetta2 will provide the emulation with just the arch prefix.
                             arch_prefix = ["arch", "-x86_64"]
                         else:
-                            raise RuntimeError(
+                            msg = f"don't know how to emulate {testing_arch} on {machine_arch}"
-                                "don't know how to emulate {testing_arch} on {machine_arch}"
+                            raise RuntimeError(msg)
-                            )
 
                     # define a custom 'call' function that adds the arch prefix each time
-                    def call_with_arch(*args: PathOrStr, **kwargs: Any) -> None:
+                    call_with_arch = functools.partial(call, *arch_prefix)
-                        call(*arch_prefix, *args, **kwargs)
+                    shell_with_arch = functools.partial(shell, *arch_prefix)
-
-                    def shell_with_arch(command: str, **kwargs: Any) -> None:
-                        command = " ".join(arch_prefix) + " " + command
-                        shell(command, **kwargs)
 
                     # Use --no-download to ensure determinism by using seed libraries
                     # built into virtualenv

cibuildwheel/options.py

@@ -1,3 +1,4 @@
+import functools
 import os
 import sys
 import traceback
@@ -31,6 +32,7 @@ from .util import (
     BuildSelector,
     DependencyConstraints,
     TestSelector,
+    cached_property,
     resources_dir,
     selector_matches,
     strtobool,
@@ -139,7 +141,8 @@ def _dig_first(*pairs: Tuple[Mapping[str, Setting], str], ignore_empty: bool = F
 
             return value
 
-    raise KeyError(key)
+    last_key = pairs[-1][1]
+    raise KeyError(last_key)
 
 
 class OptionsReader:
@@ -310,13 +313,15 @@ class OptionsReader:
             if table is None:
                 raise ConfigOptionError(f"{name} does not accept a table")
             return table["sep"].join(table["item"].format(k=k, v=v) for k, v in result.items())
-        elif isinstance(result, list):
+
+        if isinstance(result, list):
             if sep is None:
                 raise ConfigOptionError(f"{name} does not accept a list")
             return sep.join(result)
-        elif isinstance(result, int):
+
+        if isinstance(result, int):
             return str(result)
-        else:
+
         return result
 
 
@@ -345,12 +350,10 @@ class Options:
 
         return None
 
-    @property
+    @cached_property
     def package_requires_python_str(self) -> Optional[str]:
-        if not hasattr(self, "_package_requires_python_str"):
         args = self.command_line_arguments
-            self._package_requires_python_str = get_requires_python_str(Path(args.package_dir))
+        return get_requires_python_str(Path(args.package_dir))
-        return self._package_requires_python_str
 
     @property
     def globals(self) -> GlobalOptions:
@@ -574,9 +577,7 @@ def compute_options(
     return options
 
 
-_all_pinned_docker_images: Optional[ConfigParser] = None
+@functools.lru_cache(maxsize=None)
-
-
 def _get_pinned_docker_images() -> Mapping[str, Mapping[str, str]]:
     """
     This looks like a dict of dicts, e.g.
@@ -585,13 +586,11 @@ def _get_pinned_docker_images() -> Mapping[str, Mapping[str, str]]:
       'pypy_x86_64': {'manylinux2010': '...' }
       ... }
     """
-    global _all_pinned_docker_images
 
-    if _all_pinned_docker_images is None:
     pinned_docker_images_file = resources_dir / "pinned_docker_images.cfg"
-        _all_pinned_docker_images = ConfigParser()
+    all_pinned_docker_images = ConfigParser()
-        _all_pinned_docker_images.read(pinned_docker_images_file)
+    all_pinned_docker_images.read(pinned_docker_images_file)
-    return _all_pinned_docker_images
+    return all_pinned_docker_images
 
 
 def deprecated_selectors(name: str, selector: str, *, error: bool = False) -> None:

cibuildwheel/projectfiles.py

@@ -23,11 +23,11 @@ class Analyzer(ast.NodeVisitor):
     def __init__(self) -> None:
         self.requires_python: Optional[str] = None
 
-    def visit(self, content: ast.AST) -> None:
+    def visit(self, node: ast.AST) -> None:
-        for node in ast.walk(content):
+        for inner_node in ast.walk(node):
-            for child in ast.iter_child_nodes(node):
+            for child in ast.iter_child_nodes(inner_node):
-                child.parent = node  # type: ignore[attr-defined]
+                child.parent = inner_node  # type: ignore[attr-defined]
-        super().visit(content)
+        super().visit(node)
 
     def visit_keyword(self, node: ast.keyword) -> None:
         self.generic_visit(node)
@@ -46,7 +46,7 @@ def setup_py_python_requires(content: str) -> Optional[str]:
         analyzer = Analyzer()
         analyzer.visit(tree)
         return analyzer.requires_python or None
-    except Exception:
+    except Exception:  # pylint: disable=broad-except
         return None
 
 

cibuildwheel/util.py

@@ -41,6 +41,20 @@ from platformdirs import user_cache_path
 
 from cibuildwheel.typing import Final, Literal, PathOrStr, PlatformName
 
+__all__ = [
+    "resources_dir",
+    "MANYLINUX_ARCHS",
+    "call",
+    "shell",
+    "format_safe",
+    "prepare_command",
+    "get_build_verbosity_extra_flags",
+    "read_python_configs",
+    "selector_matches",
+    "strtobool",
+    "cached_property",
+]
+
 resources_dir: Final = Path(__file__).parent / "resources"
 
 install_certifi_script: Final = resources_dir / "install_certifi.py"
@@ -118,8 +132,9 @@ def call(
 
 
 def shell(
-    command: str, env: Optional[Dict[str, str]] = None, cwd: Optional[PathOrStr] = None
+    *commands: str, env: Optional[Dict[str, str]] = None, cwd: Optional[PathOrStr] = None
 ) -> None:
+    command = " ".join(commands)
     print(f"+ {command}")
     subprocess.run(command, env=env, cwd=cwd, shell=True, check=True)
 
@@ -152,11 +167,11 @@ def format_safe(template: str, **kwargs: Any) -> str:
             re.VERBOSE,
         )
 
-        # we use a lambda for repl to prevent re.sub interpreting backslashes
+        # we use a function for repl to prevent re.sub interpreting backslashes
-        # in repl as escape sequences
+        # in repl as escape sequences.
         result = re.sub(
             pattern=find_pattern,
-            repl=lambda _: str(value),
+            repl=lambda _: str(value),  # pylint: disable=cell-var-from-loop
             string=result,
         )
 
@@ -288,18 +303,14 @@ def download(url: str, dest: Path) -> None:
     repeat_num = 3
     for i in range(repeat_num):
         try:
-            response = urllib.request.urlopen(url, context=context)
+            with urllib.request.urlopen(url, context=context) as response:
-        except Exception:
+                dest.write_bytes(response.read())
+                return
+
+        except urllib.error.URLError:
             if i == repeat_num - 1:
                 raise
             sleep(3)
-            continue
-        break
-
-    try:
-        dest.write_bytes(response.read())
-    finally:
-        response.close()
 
 
 class DependencyConstraints:
@@ -319,6 +330,7 @@ class DependencyConstraints:
         specific_stem = self.base_file_path.stem + f"-python{version_parts[0]}{version_parts[1]}"
         specific_name = specific_stem + self.base_file_path.suffix
         specific_file_path = self.base_file_path.with_name(specific_name)
+
         if specific_file_path.exists():
             return specific_file_path
         else:
@@ -477,7 +489,7 @@ def _parse_constraints_for_virtualenv(
         assert dependency_constraint_flags[0] == "-c"
         constraint_path = Path(dependency_constraint_flags[1])
         assert constraint_path.exists()
-        with constraint_path.open() as constraint_file:
+        with constraint_path.open(encoding="utf-8") as constraint_file:
             for line in constraint_file:
                 line = line.strip()
                 if len(line) == 0:
@@ -543,3 +555,9 @@ def virtualenv(
     env = os.environ.copy()
     env["PATH"] = os.pathsep.join(paths + [env["PATH"]])
     return env
+
+
+if sys.version_info >= (3, 8):
+    from functools import cached_property
+else:
+    from .functools_cached_property_38 import cached_property

noxfile.py

@@ -1,15 +1,19 @@
+import os
 import shutil
 import sys
 from pathlib import Path
 
 import nox
 
-nox.options.sessions = ["lint", "check_manifest", "tests"]
+nox.options.sessions = ["lint", "pylint", "check_manifest", "tests"]
 
 PYTHON_ALL_VERSIONS = ["3.6", "3.7", "3.8", "3.9", "3.10"]
 
 DIR = Path(__file__).parent.resolve()
 
+if os.environ.get("CI", None):
+    nox.options.error_on_missing_interpreters = True
+
 
 @nox.session
 def tests(session: nox.Session) -> None:
@@ -34,6 +38,16 @@ def lint(session: nox.Session) -> None:
     session.run("pre-commit", "run", "--all-files", *session.posargs)
 
 
+@nox.session
+def pylint(session: nox.Session) -> None:
+    """
+    Run pylint.
+    """
+
+    session.install("pylint", ".")
+    session.run("pylint", "cibuildwheel", *session.posargs)
+
+
 @nox.session
 def check_manifest(session: nox.Session) -> None:
     """

pyproject.toml

@@ -84,3 +84,37 @@ ignore = [
     "requirements-dev.txt",
     "noxfile.py",
 ]
+
+[tool.pylint]
+master.py-version = "3.6"
+master.jobs = "0"
+master.fail-on = ["E", "F"]
+master.fail-under = "9.8"
+reports.output-format = "colorized"
+messages_control.enable = [
+  "useless-suppression",
+]
+messages_control.disable = [
+  "abstract-class-instantiated",  # filelock triggers this
+  "duplicate-code",
+  "fixme",
+  "invalid-name",
+  "line-too-long",
+  "missing-class-docstring",
+  "missing-function-docstring",
+  "missing-module-docstring",
+  "no-else-break",
+  "no-else-return",
+  "protected-access",
+  "too-few-public-methods",
+  "too-many-arguments",
+  "too-many-branches",
+  "too-many-instance-attributes",
+  "too-many-lines",
+  "too-many-locals",
+  "too-many-nested-blocks",
+  "too-many-return-statements",
+  "too-many-statements",
+  "unsubscriptable-object",
+  "wrong-import-position",
+]

setup.py

@@ -32,7 +32,6 @@ extras = {
         "types-pyyaml",
         "types-click",
         "types-requests",
-        "types-toml",
     ],
 }