Skip to content

Commit

Permalink
Add ruff as linter/code formatter; apply some suggested fixes
Browse files Browse the repository at this point in the history
With heavy inspiration from
freedomofpress/securedrop#6961
  • Loading branch information
eloquence committed Jan 9, 2024
1 parent 6d90cf7 commit bf5dadc
Show file tree
Hide file tree
Showing 9 changed files with 129 additions and 41 deletions.
16 changes: 16 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -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
Expand Down
58 changes: 58 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -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"
]
7 changes: 2 additions & 5 deletions scripts/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
"""
Expand Down
1 change: 1 addition & 0 deletions test-requirements.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pytest
pytest-mock
virtualenv<16
ruff
8 changes: 4 additions & 4 deletions tests/test_deb_package.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand All @@ -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() == ""

8 changes: 4 additions & 4 deletions tests/test_reproducible_debian_packages.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import pytest
import subprocess
import os
import subprocess

import pytest

PACKAGE_BUILD_TARGETS = {
"securedrop-client": "main",
Expand All @@ -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()

Expand Down
17 changes: 13 additions & 4 deletions tests/test_reproducible_wheels.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import pytest
import subprocess

import pytest

# These are the SDW repositories that we build wheels for.
REPOS_WITH_WHEELS = [
Expand All @@ -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())
Expand Down
37 changes: 22 additions & 15 deletions tests/test_update_requirements.py
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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"))
Expand All @@ -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"))
Expand All @@ -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
18 changes: 9 additions & 9 deletions tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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():
Expand All @@ -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():
Expand Down

0 comments on commit bf5dadc

Please sign in to comment.