From bf5dadc245b73cc4f1fe0c6093fb35807373cb0e Mon Sep 17 00:00:00 2001 From: Erik Moeller Date: Mon, 8 Jan 2024 17:19:01 -0800 Subject: [PATCH 1/3] Add ruff as linter/code formatter; apply some suggested fixes With heavy inspiration from https://github.com/freedomofpress/securedrop/pull/6961 --- Makefile | 16 ++++++ pyproject.toml | 58 ++++++++++++++++++++++ scripts/utils.py | 7 +-- test-requirements.txt | 1 + tests/test_deb_package.py | 8 +-- tests/test_reproducible_debian_packages.py | 8 +-- tests/test_reproducible_wheels.py | 17 +++++-- tests/test_update_requirements.py | 37 ++++++++------ tests/test_utils.py | 18 +++---- 9 files changed, 129 insertions(+), 41 deletions(-) create mode 100644 pyproject.toml diff --git a/Makefile b/Makefile index 815ac85a..cafb590e 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,22 @@ DEFAULT_GOAL: help SHELL := /bin/bash +.PHONY: check-lint +check-lint: + @ruff check . + +.PHONY: fix-lint +fix-lint: + @ruff check . --fix + +.PHONY: check-format +check-format: + @ruff format --check . + +.PHONY: fix-format +fix-format: + @ruff format . + .PHONY: securedrop-proxy securedrop-proxy: ## Builds Debian package for securedrop-proxy code PKG_NAME="securedrop-proxy" ./scripts/build-debianpackage diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 00000000..2b7ce4a1 --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,58 @@ +[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", + # Ruff formatter makes a best effort, but may result in some lines exceeding 100 characters + "E501", +] + +[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" +] diff --git a/scripts/utils.py b/scripts/utils.py index d0683bc0..0f7d345f 100644 --- a/scripts/utils.py +++ b/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. """ diff --git a/test-requirements.txt b/test-requirements.txt index e1ce9504..f07ee4ba 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,3 +1,4 @@ pytest pytest-mock virtualenv<16 +ruff \ No newline at end of file diff --git a/tests/test_deb_package.py b/tests/test_deb_package.py index 84c62970..8d98985f 100644 --- a/tests/test_deb_package.py +++ b/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() == "" - diff --git a/tests/test_reproducible_debian_packages.py b/tests/test_reproducible_debian_packages.py index 5fa334a5..9703413e 100644 --- a/tests/test_reproducible_debian_packages.py +++ b/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() diff --git a/tests/test_reproducible_wheels.py b/tests/test_reproducible_wheels.py index 6add0303..08d1ea15 100644 --- a/tests/test_reproducible_wheels.py +++ b/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} --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()) diff --git a/tests/test_update_requirements.py b/tests/test_update_requirements.py index 1fa8bb5d..4cfc8eb3 100644 --- a/tests/test_update_requirements.py +++ b/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,15 +47,17 @@ 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 @@ -60,14 +65,16 @@ def test_shasums_skips_sources(tmpdir): 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 diff --git a/tests/test_utils.py b/tests/test_utils.py index cd9f45b4..e353c936 100644 --- a/tests/test_utils.py +++ b/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(): From 4129f0f7715c6cc11b70d809a5dc964828012485 Mon Sep 17 00:00:00 2001 From: Erik Moeller Date: Tue, 9 Jan 2024 16:50:08 -0800 Subject: [PATCH 2/3] Add GitHub action --- .github/workflows/ci.yml | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000..00c41c5c --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,23 @@ +--- +name: CI + +on: [push, pull_request] + +jobs: + checks: + runs-on: ubuntu-latest + # We use a standard Debian image to mirror a typical developer environment. + # This should be updated whenever a new Debian stable version is available. + 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 check + run: | + make check-lint + - name: Run format check + run: | + make check-format From 19fa957012e2997e72a4c15f0d6626cbe738a316 Mon Sep 17 00:00:00 2001 From: Erik Moeller Date: Tue, 23 Jan 2024 17:53:04 -0800 Subject: [PATCH 3/3] Simplify targets; add Dependabot GHA config; address other review comments --- .github/dependabot.yml | 6 ++++++ .github/workflows/ci.yml | 9 ++------- Makefile | 16 +++++----------- pyproject.toml | 2 -- tests/test_reproducible_wheels.py | 6 +++--- 5 files changed, 16 insertions(+), 23 deletions(-) create mode 100644 .github/dependabot.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 00000000..5ace4600 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,6 @@ +version: 2 +updates: + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 00c41c5c..cba9abba 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,8 +6,6 @@ on: [push, pull_request] jobs: checks: runs-on: ubuntu-latest - # We use a standard Debian image to mirror a typical developer environment. - # This should be updated whenever a new Debian stable version is available. container: debian:bullseye steps: - uses: actions/checkout@v4 @@ -15,9 +13,6 @@ jobs: run: | apt-get update && apt-get install --yes --no-install-recommends make python3-pip pip install -r test-requirements.txt - - name: Run lint check + - name: Run lint checks run: | - make check-lint - - name: Run format check - run: | - make check-format + make lint \ No newline at end of file diff --git a/Makefile b/Makefile index cafb590e..b307d33a 100644 --- a/Makefile +++ b/Makefile @@ -1,20 +1,14 @@ DEFAULT_GOAL: help SHELL := /bin/bash -.PHONY: check-lint -check-lint: +.PHONY: lint +lint: @ruff check . - -.PHONY: fix-lint -fix-lint: - @ruff check . --fix - -.PHONY: check-format -check-format: @ruff format --check . -.PHONY: fix-format -fix-format: +.PHONY: fix +fix: + @ruff check . --fix @ruff format . .PHONY: securedrop-proxy diff --git a/pyproject.toml b/pyproject.toml index 2b7ce4a1..e17cb4a1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -42,8 +42,6 @@ ignore = [ "SIM108", # Flags any subprocess use "S603", - # Ruff formatter makes a best effort, but may result in some lines exceeding 100 characters - "E501", ] [tool.ruff.lint.per-file-ignores] diff --git a/tests/test_reproducible_wheels.py b/tests/test_reproducible_wheels.py index 08d1ea15..c5ab58a0 100644 --- a/tests/test_reproducible_wheels.py +++ b/tests/test_reproducible_wheels.py @@ -22,9 +22,9 @@ def test_wheel_builds_match_version_control(name): ] ) build_cmd = ( - f"./scripts/build-sync-wheels --pkg-dir /tmp/monorepo-{name}/{name} --project securedrop-{name}" - " --clobber".split() - ) + 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())