Merge pull request #484 from freedomofpress/add-ruff

Add ruff for lint & formatting checks; apply some suggested fixes

.github/dependabot.yml

@@ -0,0 +1,6 @@
+version: 2
+updates:
+  - package-ecosystem: "github-actions"
+    directory: "/"
+    schedule:
+      interval: "weekly"

.github/workflows/ci.yml

@@ -0,0 +1,18 @@
+---
+name: CI
+
+on: [push, pull_request]
+
+jobs:
+  checks:
+    runs-on: ubuntu-latest
+    container: debian:bullseye
+    steps:
+      - uses: actions/checkout@v4
+      - name: Install dependencies
+        run: |
+          apt-get update && apt-get install --yes --no-install-recommends make python3-pip
+          pip install -r test-requirements.txt
+      - name: Run lint checks
+        run: |
+          make lint

Makefile

@@ -1,6 +1,16 @@
 DEFAULT_GOAL: help
 SHELL := /bin/bash
 
+.PHONY: lint
+lint:
+	@ruff check .
+	@ruff format --check .
+
+.PHONY: fix
+fix:
+	@ruff check . --fix
+	@ruff format .
+
 .PHONY: securedrop-proxy
 securedrop-proxy: ## Builds Debian package for securedrop-proxy code
 	PKG_NAME="securedrop-proxy" ./scripts/build-debianpackage

pyproject.toml

@@ -0,0 +1,56 @@
+[project]
+name = "securedrop-builder"
+version = "0.1.0"
+requires-python = ">=3.9"
+
+[tool.ruff]
+line-length = 100
+select = [
+    # pycodestyle errors
+    "E",
+    # pyflakes
+    "F",
+    # isort
+    "I",
+    # flake8-gettext
+    "INT",
+    # flake8-pie
+    "PIE",
+    # pylint
+    "PL",
+    # flake8-pytest-style
+    "PT",
+    # flake8-pyi
+    "PYI",
+    # flake8-return
+    "RET",
+    # flake8-bandit
+    "S",
+    # flake8-simplify
+    "SIM",
+    # pyupgrade
+    "UP",
+    # pycodestyle warnings
+    "W",
+    # Unused noqa directive
+    "RUF100",
+]
+ignore = [
+    # Find contextlib.suppress() is harder to read
+    "SIM105",
+    # Find ternary statements harder to read
+    "SIM108",
+    # Flags any subprocess use
+    "S603",
+]
+
+[tool.ruff.lint.per-file-ignores]
+# Asserts in tests are fine
+"**/tests/*" = [
+    # use of `assert`
+    "S101",
+    # insecure temporary file/directory
+    "S108",
+    # code needs to be reorganized into modules - see https://github.com/freedomofpress/securedrop-builder/issues/465
+    "E402"
+]

scripts/utils.py

@@ -123,8 +123,7 @@ def get_poetry_hashes(
         if package_name in relevant_dependencies:
             package_name_and_version = f"{package_name}=={package['version']}"
             dependencies[package_name_and_version] = [
-                file_dict["hash"].replace("sha256:", "")
-                for file_dict in package["files"]
+                file_dict["hash"].replace("sha256:", "") for file_dict in package["files"]
             ]
 
     return dependencies
@@ -160,9 +159,7 @@ def get_requirements_hashes(path_to_requirements_file: Path) -> dict[str, list[s
     return result_dict
 
 
-def get_requirements_from_poetry(
-    path_to_poetry_lock: Path, path_to_pyproject_toml: Path
-) -> str:
+def get_requirements_from_poetry(path_to_poetry_lock: Path, path_to_pyproject_toml: Path) -> str:
     """
     Returns a multiline string in requirements.txt format for a set of Poetry main dependencies.
     """

test-requirements.txt

@@ -1,3 +1,4 @@
 pytest
 pytest-mock
 virtualenv<16
+ruff

tests/test_deb_package.py

@@ -5,10 +5,11 @@
 import pytest
 
 SECUREDROP_ROOT = Path(
-    subprocess.check_output(["git", "rev-parse", "--show-toplevel"]).decode().strip()
+    subprocess.check_output(["/usr/bin/git", "rev-parse", "--show-toplevel"]).decode().strip()
 )
 DEB_PATHS = list((SECUREDROP_ROOT / "build/debbuild/packaging").glob("*.deb"))
 
+
 @pytest.mark.parametrize("deb", DEB_PATHS)
 def test_securedrop_keyring_removes_conffiles(deb: Path):
     """
@@ -20,13 +21,12 @@ def test_securedrop_keyring_removes_conffiles(deb: Path):
     When `securedrop-keyring.gpg` is shipped in `/usr/share/keyrings`, this
     test can be removed.
     """
-    if not deb.name.startswith(("securedrop-keyring")):
+    if not deb.name.startswith("securedrop-keyring"):
         return
 
     with tempfile.TemporaryDirectory() as tmpdir:
-        subprocess.check_call(["dpkg-deb", "--control", deb, tmpdir])
+        subprocess.check_call(["/usr/bin/dpkg-deb", "--control", deb, tmpdir])
         conffiles_path = Path(tmpdir) / "conffiles"
         assert conffiles_path.exists()
         # No files are currently allow-listed to be conffiles
         assert conffiles_path.read_text().rstrip() == ""
-

tests/test_reproducible_debian_packages.py

@@ -1,7 +1,7 @@
-import pytest
-import subprocess
 import os
+import subprocess
 
+import pytest
 
 PACKAGE_BUILD_TARGETS = {
     "securedrop-client": "main",
@@ -16,8 +16,8 @@
 
 def get_repo_root():
     cmd = "git rev-parse --show-toplevel".split()
-    top_level = subprocess.check_output(cmd).decode("utf-8").rstrip()
-    return top_level
+    return subprocess.check_output(cmd).decode("utf-8").rstrip()
+
 
 repo_root = get_repo_root()
 

tests/test_reproducible_wheels.py

@@ -1,6 +1,6 @@
-import pytest
 import subprocess
 
+import pytest
 
 # These are the SDW repositories that we build wheels for.
 REPOS_WITH_WHEELS = [
@@ -13,9 +13,18 @@
 
 @pytest.mark.parametrize("name", REPOS_WITH_WHEELS)
 def test_wheel_builds_match_version_control(name):
-    subprocess.check_call(["git", "clone", "https://github.com/freedomofpress/securedrop-client", f"/tmp/monorepo-{name}"])
-    build_cmd = f"./scripts/build-sync-wheels --pkg-dir /tmp/monorepo-{name}/{name} --project securedrop-{name}" \
-                " --clobber".split()
+    subprocess.check_call(
+        [
+            "/usr/bin/git",
+            "clone",
+            "https://github.com/freedomofpress/securedrop-client",
+            f"/tmp/monorepo-{name}",
+        ]
+    )
+    build_cmd = (
+        f"./scripts/build-sync-wheels --pkg-dir /tmp/monorepo-{name}/{name} "
+        f"--project securedrop-{name} --clobber"
+    ).split()
     subprocess.check_call(build_cmd)
     # Check for modified files (won't catch new, untracked files)
     subprocess.check_call("git diff --exit-code".split())

tests/test_update_requirements.py

@@ -1,9 +1,10 @@
-from importlib.machinery import SourceFileLoader
 import os
 import sys
-import pytest
-from pathlib import Path
 import types
+from importlib.machinery import SourceFileLoader
+from pathlib import Path
+
+import pytest
 
 # This below stanza is necessary because the scripts are not
 # structured as a module.
@@ -16,15 +17,17 @@
 loader = SourceFileLoader("update-requirements", path_to_script)
 loader.exec_module(update_requirements)
 
-TEST_DEPENDENCIES = [('pathlib2', '2.3.2')]
+TEST_DEPENDENCIES = [("pathlib2", "2.3.2")]
 TEST_SOURCE_HASH = "8eb170f8d0d61825e09a95b38be068299ddeda82f35e96c3301a8a5e7604cb83"
 TEST_WHEEL_HASH = "8e276e2bf50a9a06c36e20f03b050e59b63dfe0678e37164333deb87af03b6ad"
-TEST_SHASUM_LINES = ["\n{}  pathlib2-2.3.2-py2.py3-none-any.whl".format(TEST_WHEEL_HASH),
-                     "\n{}  pathlib2-2.3.2.tar.gz".format(TEST_SOURCE_HASH)]
+TEST_SHASUM_LINES = [
+    f"\n{TEST_WHEEL_HASH}  pathlib2-2.3.2-py2.py3-none-any.whl",
+    f"\n{TEST_SOURCE_HASH}  pathlib2-2.3.2.tar.gz",
+]
 
 
 def test_build_fails_if_sha256_sums_absent(tmpdir, mocker):
-    mocker.patch('os.path.exists', return_value=False)
+    mocker.patch("os.path.exists", return_value=False)
 
     with pytest.raises(SystemExit) as exc_info:
         update_requirements.verify_sha256sums_file(Path("foo"))
@@ -34,7 +37,7 @@ def test_build_fails_if_sha256_sums_absent(tmpdir, mocker):
 
 
 def test_build_fails_if_sha256_signature_absent(tmpdir, mocker):
-    mocker.patch('os.path.exists', side_effect=[True, False])
+    mocker.patch("os.path.exists", side_effect=[True, False])
 
     with pytest.raises(SystemExit) as exc_info:
         update_requirements.verify_sha256sums_file(Path("foo"))
@@ -44,30 +47,34 @@ def test_build_fails_if_sha256_signature_absent(tmpdir, mocker):
 
 
 def test_shasums_skips_sources(tmpdir):
-    path_test_shasums = os.path.join(tmpdir, 'test-shasums.txt')
-    with open(path_test_shasums, 'w') as f:
+    path_test_shasums = os.path.join(tmpdir, "test-shasums.txt")
+    with open(path_test_shasums, "w") as f:
         f.writelines(TEST_SHASUM_LINES)
 
     path_result = os.path.join(tmpdir, "test-req.txt")
 
-    update_requirements.add_sha256sums(Path(path_result), TEST_DEPENDENCIES, Path(path_test_shasums), Path("foo"))
+    update_requirements.add_sha256sums(
+        Path(path_result), TEST_DEPENDENCIES, Path(path_test_shasums), Path("foo")
+    )
 
-    with open(path_result, 'r') as f:
+    with open(path_result) as f:
         result = f.read()
 
     assert TEST_WHEEL_HASH in result
     assert TEST_SOURCE_HASH not in result
 
 
 def test_build_fails_if_missing_wheels(tmpdir):
-    path_test_shasums = os.path.join(tmpdir, 'test-shasums.txt')
-    with open(path_test_shasums, 'w') as f:
+    path_test_shasums = os.path.join(tmpdir, "test-shasums.txt")
+    with open(path_test_shasums, "w") as f:
         f.writelines([])
 
     path_result = os.path.join(tmpdir, "test-req.txt")
 
     with pytest.raises(SystemExit) as exc_info:
-        update_requirements.add_sha256sums(Path(path_result), TEST_DEPENDENCIES, Path(path_test_shasums), Path("foo"))
+        update_requirements.add_sha256sums(
+            Path(path_result), TEST_DEPENDENCIES, Path(path_test_shasums), Path("foo")
+        )
 
     exit_code = exc_info.value.args[0]
     assert exit_code == 1

tests/test_utils.py

@@ -1,21 +1,20 @@
 import os
-import pytest
 import sys
 from pathlib import Path
 
+import pytest
+
 # Adjusting the path to import utils module
-path_to_script = os.path.join(
-    os.path.dirname(os.path.abspath(__file__)), "../scripts/utils.py"
-)
+path_to_script = os.path.join(os.path.dirname(os.path.abspath(__file__)), "../scripts/utils.py")
 sys.path.append(os.path.dirname(path_to_script))
 
 from utils import (
-    get_requirements_names_and_versions,
+    get_poetry_hashes,
     get_poetry_names_and_versions,
     get_relevant_poetry_dependencies,
-    get_poetry_hashes,
-    get_requirements_hashes,
     get_requirements_from_poetry,
+    get_requirements_hashes,
+    get_requirements_names_and_versions,
 )
 
 # These tests generally verify that our utility functions correctly parse the
@@ -33,6 +32,8 @@
 ]
 EXPECTED_DEPENDENCY_NAMES = [name for name, _ in EXPECTED_DEPENDENCIES]
 EXPECTED_KEYS = [f"{name}=={version}" for name, version in EXPECTED_DEPENDENCIES]
+# Hex-encoded SHA-256 hashes are 64 characters long
+SHA256_HASH_LENGTH = 64
 
 
 def test_get_requirements_names_and_versions():
@@ -58,9 +59,8 @@ def _check_hashes(output):
     for _, hashes in output.items():
         # We should have at least one hash per dependency
         assert len(hashes) > 0
-        # Hex-encoded SHA-256 hashes are 64 characters long
         for hash in hashes:
-            assert len(hash) == 64
+            assert len(hash) == SHA256_HASH_LENGTH
 
 
 def test_get_poetry_hashes():