From 79d68cacbc5bf99704e8e71c08e6b31c440fe3fb Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 2 Oct 2025 22:03:01 +0100 Subject: [PATCH 01/21] Handle invalid Cargo manifest parsing Catch TOML decode failures in the manifest reader script and cover it\nwith unit tests that exercise the CLI and supported fields. --- .github/workflows/scripts/read_manifest.py | 2 +- tests_python/test_read_manifest.py | 125 +++++++++++++++++++++ 2 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 tests_python/test_read_manifest.py diff --git a/.github/workflows/scripts/read_manifest.py b/.github/workflows/scripts/read_manifest.py index 33336785..92f99163 100755 --- a/.github/workflows/scripts/read_manifest.py +++ b/.github/workflows/scripts/read_manifest.py @@ -60,7 +60,7 @@ def main() -> int: try: manifest = read_manifest(Path(manifest_path)) value = get_field(manifest, args.field) - except (KeyError, FileNotFoundError) as exc: + except (KeyError, FileNotFoundError, tomllib.TOMLDecodeError) as exc: print(exc, file=sys.stderr) return 1 print(value, end="") diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py new file mode 100644 index 00000000..855f4328 --- /dev/null +++ b/tests_python/test_read_manifest.py @@ -0,0 +1,125 @@ +"""Tests for the read_manifest helper script.""" + +from __future__ import annotations + +import importlib.util +import os +import subprocess +import sys +from pathlib import Path +from tempfile import TemporaryDirectory +from textwrap import dedent +from typing import Any + +import unittest + + +REPO_ROOT = Path(__file__).resolve().parent.parent +SCRIPT_PATH = REPO_ROOT / ".github" / "workflows" / "scripts" / "read_manifest.py" + + +def load_script_module() -> Any: + spec = importlib.util.spec_from_file_location("read_manifest", SCRIPT_PATH) + module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] + assert spec and spec.loader + spec.loader.exec_module(module) # type: ignore[assignment] + return module + + +class ReadManifestTests(unittest.TestCase): + @classmethod + def setUpClass(cls) -> None: + cls.module = load_script_module() + + def setUp(self) -> None: + self.tempdir = TemporaryDirectory() + self.temp_path = Path(self.tempdir.name) + + def tearDown(self) -> None: + self.tempdir.cleanup() + + def _write_manifest(self, content: str) -> Path: + manifest = self.temp_path / "Cargo.toml" + manifest.write_text(dedent(content), encoding="utf-8") + return manifest + + def test_get_field_returns_name(self) -> None: + manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} + self.assertEqual(self.module.get_field(manifest, "name"), "netsuke") + + def test_get_field_returns_version(self) -> None: + manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} + self.assertEqual(self.module.get_field(manifest, "version"), "1.2.3") + + def test_get_field_raises_when_missing(self) -> None: + manifest = {"package": {"name": "netsuke"}} + with self.assertRaises(KeyError): + self.module.get_field(manifest, "version") + + def test_main_reads_manifest_path_argument(self) -> None: + manifest = self._write_manifest( + """ + [package] + name = "netsuke" + version = "1.2.3" + """ + ) + result = subprocess.run( + [sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(manifest)], + check=False, + capture_output=True, + text=True, + ) + self.assertEqual(result.returncode, 0) + self.assertEqual(result.stdout, "netsuke") + self.assertEqual(result.stderr, "") + + def test_main_prefers_environment_manifest_path(self) -> None: + manifest = self._write_manifest( + """ + [package] + name = "netsuke" + version = "1.2.3" + """ + ) + env = os.environ.copy() + env["CARGO_TOML_PATH"] = str(manifest) + result = subprocess.run( + [sys.executable, str(SCRIPT_PATH), "version"], + check=False, + capture_output=True, + text=True, + env=env, + cwd=self.temp_path, + ) + self.assertEqual(result.returncode, 0) + self.assertEqual(result.stdout, "1.2.3") + self.assertEqual(result.stderr, "") + + def test_main_reports_missing_manifest(self) -> None: + missing = self.temp_path / "missing.toml" + result = subprocess.run( + [sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(missing)], + check=False, + capture_output=True, + text=True, + ) + self.assertNotEqual(result.returncode, 0) + self.assertIn("does not exist", result.stderr) + self.assertEqual(result.stdout, "") + + def test_main_reports_invalid_toml(self) -> None: + manifest = self._write_manifest("not = [valid") + result = subprocess.run( + [sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(manifest)], + check=False, + capture_output=True, + text=True, + ) + self.assertNotEqual(result.returncode, 0) + self.assertTrue(result.stderr) + self.assertEqual(result.stdout, "") + + +if __name__ == "__main__": + unittest.main() From 3ec789a268a836a7ad136c7b39940ee2722d7838 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 3 Oct 2025 09:39:30 +0100 Subject: [PATCH 02/21] Extract release asset upload automation --- .../actions/upload-release-assets/action.yml | 30 +++ .github/workflows/release-dry-run.yml | 18 +- .github/workflows/release.yml | 50 +---- scripts/tests/test_upload_release_assets.py | 102 +++++++++ scripts/upload_release_assets.py | 196 ++++++++++++++++++ 5 files changed, 340 insertions(+), 56 deletions(-) create mode 100644 .github/actions/upload-release-assets/action.yml create mode 100644 scripts/tests/test_upload_release_assets.py create mode 100755 scripts/upload_release_assets.py diff --git a/.github/actions/upload-release-assets/action.yml b/.github/actions/upload-release-assets/action.yml new file mode 100644 index 00000000..19eb3bcc --- /dev/null +++ b/.github/actions/upload-release-assets/action.yml @@ -0,0 +1,30 @@ +name: Upload release artefacts +description: Upload staged artefacts to a GitHub release or validate them in dry-run mode. +inputs: + release-tag: + description: Git tag identifying the release to publish to. + required: true + bin-name: + description: Binary name used to derive artefact names. + required: true + dist-dir: + description: Directory containing staged artefacts. + required: false + default: dist + dry-run: + description: When true, only validate artefacts and print the upload plan. + required: false + default: "false" +runs: + using: composite + steps: + - name: Upload release artefacts + shell: bash + env: + INPUT_RELEASE_TAG: ${{ inputs.release-tag }} + INPUT_BIN_NAME: ${{ inputs.bin-name }} + INPUT_DIST_DIR: ${{ inputs.dist-dir }} + INPUT_DRY_RUN: ${{ inputs.dry-run }} + run: | + set -euo pipefail + "$GITHUB_WORKSPACE/scripts/upload_release_assets.py" diff --git a/.github/workflows/release-dry-run.yml b/.github/workflows/release-dry-run.yml index 17fa02b3..02ebf842 100644 --- a/.github/workflows/release-dry-run.yml +++ b/.github/workflows/release-dry-run.yml @@ -27,14 +27,10 @@ jobs: with: path: dist pattern: ${{ env.REPO_NAME }}-* - - name: Ensure artefacts exist - shell: bash - run: | - set -euo pipefail - shopt -s nullglob - mapfile -t files < <(find dist -type f) - if [ "${#files[@]}" -eq 0 ]; then - echo '::error title=Missing artefacts::No artefacts downloaded' - exit 1 - fi - printf 'Found %s artefact(s) in dist/\n' "${#files[@]}" + - name: Validate artefacts + uses: ./.github/actions/upload-release-assets + with: + release-tag: ${{ github.ref_name }} + bin-name: ${{ needs.release.outputs.bin_name }} + dist-dir: dist + dry-run: "true" diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6ab5dc3f..076027ec 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -299,51 +299,11 @@ jobs: path: dist pattern: ${{ env.REPO_NAME }}-* - name: Upload artefacts to release - shell: bash - run: | - set -euo pipefail - shopt -s nullglob - bin_name="${BIN_NAME:?BIN_NAME must be provided}" - mapfile -t files < <( - find dist -type f \ - \( -name "*.deb" -o -name "*.rpm" -o -name "*.pkg" -o -name "*.msi" \ - -o -name "${bin_name}" -o -name "${bin_name}.exe" \ - -o -name "${bin_name}.1" -o -name "*.sha256" \) - -print - ) - if [ "${#files[@]}" -eq 0 ]; then - message='::error title=No artefacts uploaded::No files found in ' - message+='dist/' - echo "$message" - exit 1 - fi - declare -A seen=() - uploaded=0 - for file in "${files[@]}"; do - dir_name="$(basename "$(dirname "$file")")" - base_name="$(basename "$file")" - if [[ "$base_name" =~ \.(deb|rpm|pkg)$ ]]; then - asset_name="$base_name" - else - asset_name="${dir_name}-${base_name}" - fi - if [[ -n "${seen[$asset_name]:-}" ]]; then - printf '::error title=Duplicate release asset::Asset name '\''%s'\'' would be uploaded more than once\n' \ - "${asset_name}" - exit 1 - fi - seen[$asset_name]=1 - gh release upload "${{ github.ref_name }}" \ - "$file#${asset_name}" --clobber \ - && uploaded=$((uploaded + 1)) - done - if [ "$uploaded" -eq 0 ]; then - message='::error title=No artefacts uploaded::No files were ' - message+='published to the release' - echo "$message" - exit 1 - fi + uses: ./.github/actions/upload-release-assets + with: + release-tag: ${{ github.ref_name }} + bin-name: ${{ needs.metadata.outputs.bin_name }} + dist-dir: dist env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - BIN_NAME: ${{ needs.metadata.outputs.bin_name }} diff --git a/scripts/tests/test_upload_release_assets.py b/scripts/tests/test_upload_release_assets.py new file mode 100644 index 00000000..5b987509 --- /dev/null +++ b/scripts/tests/test_upload_release_assets.py @@ -0,0 +1,102 @@ +"""Tests for the upload_release_assets helper script.""" + +from __future__ import annotations + +import importlib.util +import subprocess +import sys +from pathlib import Path + +import pytest + + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT_PATH = REPO_ROOT / "scripts" / "upload_release_assets.py" + + +@pytest.fixture(scope="session") +def module(): + spec = importlib.util.spec_from_file_location( + "upload_release_assets", SCRIPT_PATH + ) + module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] + assert spec and spec.loader + sys.modules[spec.name] = module + spec.loader.exec_module(module) # type: ignore[assignment] + return module + + +def create_file(path: Path, content: bytes = b"data") -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(content) + + +def test_discover_assets_collects_expected_files(module, tmp_path: Path) -> None: + dist = tmp_path / "dist" + create_file(dist / "linux" / "netsuke", b"binary") + create_file(dist / "linux" / "netsuke.sha256", b"checksum") + create_file(dist / "linux" / "netsuke.deb", b"deb") + create_file(dist / "windows" / "netsuke.exe", b"exe") + create_file(dist / "windows" / "netsuke.msi", b"msi") + create_file(dist / "man" / "netsuke.1", b"man") + + assets = module.discover_assets(dist, bin_name="netsuke") + + assert [asset.asset_name for asset in assets] == [ + "linux-netsuke", + "netsuke.deb", + "linux-netsuke.sha256", + "man-netsuke.1", + "windows-netsuke.exe", + "windows-netsuke.msi", + ] + + +def test_discover_assets_rejects_duplicates(module, tmp_path: Path) -> None: + dist = tmp_path / "dist" + create_file(dist / "a" / "netsuke.pkg", b"pkg-a") + create_file(dist / "b" / "netsuke.pkg", b"pkg-b") + + with pytest.raises(module.AssetError) as exc: + module.discover_assets(dist, bin_name="netsuke") + + assert "Asset name collision" in str(exc.value) + + +def test_discover_assets_rejects_empty_files(module, tmp_path: Path) -> None: + dist = tmp_path / "dist" + create_file(dist / "linux" / "netsuke", b"") + + with pytest.raises(module.AssetError) as exc: + module.discover_assets(dist, bin_name="netsuke") + + assert "is empty" in str(exc.value) + + +def test_cli_dry_run_outputs_summary(module, tmp_path: Path) -> None: + dist = tmp_path / "dist" + create_file(dist / "linux" / "netsuke", b"binary") + create_file(dist / "linux" / "netsuke.sha256", b"checksum") + + result = subprocess.run( + [ + sys.executable, + str(SCRIPT_PATH), + "--release-tag", + "v1.2.3", + "--bin-name", + "netsuke", + "--dist-dir", + str(dist), + "--dry-run", + ], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0 + assert "Planned uploads:" in result.stdout + assert "linux-netsuke" in result.stdout + assert "linux-netsuke.sha256" in result.stdout + assert "[dry-run] gh release upload v1.2.3" in result.stdout diff --git a/scripts/upload_release_assets.py b/scripts/upload_release_assets.py new file mode 100755 index 00000000..61b49940 --- /dev/null +++ b/scripts/upload_release_assets.py @@ -0,0 +1,196 @@ +#!/usr/bin/env -S uv run python +# /// script +# requires-python = ">=3.13" +# dependencies = ["cyclopts>=2.9", "plumbum"] +# /// + +"""Upload packaged release artefacts to a GitHub release. + +The script discovers artefacts in a staging directory, validates their +filenames and sizes, and optionally uploads them using the GitHub CLI. It is +idempotent and supports a dry-run mode used by the release dry-run workflow to +assert expected asset names without mutating state. + +Examples +-------- +Upload artefacts to the ``v1.2.3`` release:: + + upload_release_assets --release-tag v1.2.3 --bin-name netsuke + +Inspect the planned uploads without publishing anything:: + + upload_release_assets --release-tag v1.2.3 --bin-name netsuke --dry-run +""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path +from typing import Annotated, Iterable +import sys + +import cyclopts +from cyclopts import App, Parameter +from plumbum import local + + +class AssetError(RuntimeError): + """Raised when the staged artefacts are invalid.""" + + +@dataclass(frozen=True) +class ReleaseAsset: + """Artefact staged for upload to a GitHub release.""" + + path: Path + asset_name: str + size: int + + +app = App(config=cyclopts.config.Env("INPUT_", command=False)) + + +def _is_candidate(path: Path, bin_name: str) -> bool: + name = path.name + if name in {bin_name, f"{bin_name}.exe", f"{bin_name}.1"}: + return True + if name.endswith(".sha256"): + return True + return path.suffix in {".deb", ".rpm", ".pkg", ".msi"} + + +def _resolve_asset_name(path: Path) -> str: + suffix = path.suffix.lower() + if suffix in {".deb", ".rpm", ".pkg"}: + return path.name + return f"{path.parent.name}-{path.name}" + + +def discover_assets(dist_dir: Path, *, bin_name: str) -> list[ReleaseAsset]: + """Return the artefacts that should be published. + + Parameters + ---------- + dist_dir: + Root directory that contains the staged artefacts. + bin_name: + Binary name used to match platform-specific artefacts. + + Returns + ------- + list[ReleaseAsset] + Ordered collection of artefacts ready to upload. + + Raises + ------ + AssetError + If no artefacts are found, an artefact is empty, or multiple files would + upload with the same asset name. + """ + + if not dist_dir.exists(): + raise AssetError(f"Artefact directory {dist_dir} does not exist") + + assets: list[ReleaseAsset] = [] + seen: dict[str, Path] = {} + + for path in sorted(p for p in dist_dir.rglob("*") if p.is_file()): + if not _is_candidate(path, bin_name): + continue + size = path.stat().st_size + if size <= 0: + raise AssetError(f"Artefact {path} is empty") + asset_name = _resolve_asset_name(path) + previous = seen.get(asset_name) + if previous: + raise AssetError( + "Asset name collision: " + f"{asset_name} would upload both {previous} and {path}" + ) + seen[asset_name] = path + assets.append(ReleaseAsset(path=path, asset_name=asset_name, size=size)) + + if not assets: + raise AssetError(f"No artefacts discovered in {dist_dir}") + + return assets + + +def _render_summary(assets: Iterable[ReleaseAsset]) -> str: + lines = ["Planned uploads:"] + for asset in assets: + lines.append( + f" - {asset.asset_name} ({asset.size} bytes) -> {asset.path}" + ) + return "\n".join(lines) + + +def upload_assets( + *, release_tag: str, assets: Iterable[ReleaseAsset], dry_run: bool = False +) -> None: + """Upload artefacts to GitHub using the ``gh`` CLI.""" + + gh_cmd = None + for asset in assets: + descriptor = f"{asset.path}#{asset.asset_name}" + if dry_run: + print(f"[dry-run] gh release upload {release_tag} {descriptor} --clobber") + continue + if gh_cmd is None: + gh_cmd = local["gh"] + gh_cmd[ + "release", + "upload", + release_tag, + descriptor, + "--clobber", + ]() + + +def main( + *, + release_tag: str, + bin_name: str, + dist_dir: Path = Path("dist"), + dry_run: bool = False, +) -> int: + """Entry point shared by the CLI and tests.""" + + try: + assets = discover_assets(dist_dir, bin_name=bin_name) + except AssetError as exc: + print(exc, file=sys.stderr) + return 1 + + if dry_run: + print(_render_summary(assets)) + + try: + upload_assets(release_tag=release_tag, assets=assets, dry_run=dry_run) + except Exception as exc: # pragma: no cover - surfaced by plumbum + print(exc, file=sys.stderr) + return 1 + + return 0 + + +@app.default +def cli( + *, + release_tag: Annotated[str, Parameter(required=True)], + bin_name: Annotated[str, Parameter(required=True)], + dist_dir: Path = Path("dist"), + dry_run: bool = False, +) -> int: + """Cyclopts-bound CLI entry point.""" + + return main( + release_tag=release_tag, + bin_name=bin_name, + dist_dir=dist_dir, + dry_run=dry_run, + ) + + +if __name__ == "__main__": # pragma: no cover - exercised via CLI + raise SystemExit(app()) From fe1c2c4d73255e4d0cb7ab8b415fd322d8f5e07a Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 3 Oct 2025 20:13:24 +0100 Subject: [PATCH 03/21] Harden manifest helper tests --- tests_python/test_read_manifest.py | 116 +++++++++++++++++++++-------- 1 file changed, 83 insertions(+), 33 deletions(-) diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 855f4328..80ac8814 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -2,9 +2,11 @@ from __future__ import annotations +from contextlib import ExitStack, contextmanager, redirect_stderr, redirect_stdout +from dataclasses import dataclass import importlib.util +from io import StringIO import os -import subprocess import sys from pathlib import Path from tempfile import TemporaryDirectory @@ -12,12 +14,34 @@ from typing import Any import unittest +from unittest.mock import patch REPO_ROOT = Path(__file__).resolve().parent.parent SCRIPT_PATH = REPO_ROOT / ".github" / "workflows" / "scripts" / "read_manifest.py" +@dataclass(slots=True) +class CLIResult: + """Result container returned by :func:`ReadManifestTests._invoke_cli`.""" + + exit_code: int + stdout: str + stderr: str + + +@contextmanager +def change_directory(path: Path) -> Any: + """Temporarily change the working directory for the current process.""" + + original = Path.cwd() + os.chdir(path) + try: + yield + finally: + os.chdir(original) + + def load_script_module() -> Any: spec = importlib.util.spec_from_file_location("read_manifest", SCRIPT_PATH) module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] @@ -43,6 +67,27 @@ def _write_manifest(self, content: str) -> Path: manifest.write_text(dedent(content), encoding="utf-8") return manifest + def _invoke_cli( + self, + *args: str, + env: dict[str, str] | None = None, + cwd: Path | None = None, + ) -> CLIResult: + """Execute the CLI and capture its exit code and output streams.""" + + stdout = StringIO() + stderr = StringIO() + with ExitStack() as stack: + stack.enter_context(patch.object(sys, "argv", [str(SCRIPT_PATH), *args])) + if env: + stack.enter_context(patch.dict(os.environ, env, clear=False)) + if cwd: + stack.enter_context(change_directory(cwd)) + stack.enter_context(redirect_stdout(stdout)) + stack.enter_context(redirect_stderr(stderr)) + exit_code = self.module.main() + return CLIResult(exit_code=exit_code, stdout=stdout.getvalue(), stderr=stderr.getvalue()) + def test_get_field_returns_name(self) -> None: manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} self.assertEqual(self.module.get_field(manifest, "name"), "netsuke") @@ -56,6 +101,22 @@ def test_get_field_raises_when_missing(self) -> None: with self.assertRaises(KeyError): self.module.get_field(manifest, "version") + def test_get_field_rejects_non_string_values(self) -> None: + manifest = { + "package": { + "name": "netsuke", + "version": 123, + "authors": ["alice", "bob"], + "metadata": {"license": "MIT"}, + } + } + with self.assertRaises(KeyError): + self.module.get_field(manifest, "version") + with self.assertRaises(KeyError): + self.module.get_field(manifest, "authors") + with self.assertRaises(KeyError): + self.module.get_field(manifest, "metadata") + def test_main_reads_manifest_path_argument(self) -> None: manifest = self._write_manifest( """ @@ -64,13 +125,8 @@ def test_main_reads_manifest_path_argument(self) -> None: version = "1.2.3" """ ) - result = subprocess.run( - [sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(manifest)], - check=False, - capture_output=True, - text=True, - ) - self.assertEqual(result.returncode, 0) + result = self._invoke_cli("name", "--manifest-path", str(manifest)) + self.assertEqual(result.exit_code, 0) self.assertEqual(result.stdout, "netsuke") self.assertEqual(result.stderr, "") @@ -82,44 +138,38 @@ def test_main_prefers_environment_manifest_path(self) -> None: version = "1.2.3" """ ) - env = os.environ.copy() - env["CARGO_TOML_PATH"] = str(manifest) - result = subprocess.run( - [sys.executable, str(SCRIPT_PATH), "version"], - check=False, - capture_output=True, - text=True, - env=env, - cwd=self.temp_path, - ) - self.assertEqual(result.returncode, 0) + env = {"CARGO_TOML_PATH": str(manifest)} + result = self._invoke_cli("version", env=env, cwd=self.temp_path) + self.assertEqual(result.exit_code, 0) self.assertEqual(result.stdout, "1.2.3") self.assertEqual(result.stderr, "") def test_main_reports_missing_manifest(self) -> None: missing = self.temp_path / "missing.toml" - result = subprocess.run( - [sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(missing)], - check=False, - capture_output=True, - text=True, - ) - self.assertNotEqual(result.returncode, 0) + result = self._invoke_cli("name", "--manifest-path", str(missing)) + self.assertNotEqual(result.exit_code, 0) self.assertIn("does not exist", result.stderr) self.assertEqual(result.stdout, "") def test_main_reports_invalid_toml(self) -> None: manifest = self._write_manifest("not = [valid") - result = subprocess.run( - [sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(manifest)], - check=False, - capture_output=True, - text=True, - ) - self.assertNotEqual(result.returncode, 0) + result = self._invoke_cli("name", "--manifest-path", str(manifest)) + self.assertNotEqual(result.exit_code, 0) self.assertTrue(result.stderr) self.assertEqual(result.stdout, "") + def test_main_reports_valid_toml_with_unexpected_structure(self) -> None: + manifest = self._write_manifest( + """ + [unexpected_section] + foo = "bar" + """ + ) + result = self._invoke_cli("name", "--manifest-path", str(manifest)) + self.assertNotEqual(result.exit_code, 0) + self.assertIn("missing", result.stderr.lower()) + self.assertEqual(result.stdout, "") + if __name__ == "__main__": unittest.main() From 96ab6c1de49bd3714bd8592876f71874ae79abcd Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 3 Oct 2025 20:19:09 +0100 Subject: [PATCH 04/21] Factor manifest error assertions --- tests_python/test_read_manifest.py | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 80ac8814..022e553f 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -7,6 +7,7 @@ import importlib.util from io import StringIO import os +import subprocess import sys from pathlib import Path from tempfile import TemporaryDirectory @@ -88,6 +89,24 @@ def _invoke_cli( exit_code = self.module.main() return CLIResult(exit_code=exit_code, stdout=stdout.getvalue(), stderr=stderr.getvalue()) + def _assert_manifest_error( + self, + manifest_path: Path, + expected_stderr_fragment: str | None = None, + ) -> None: + result = subprocess.run( + [sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(manifest_path)], + check=False, + capture_output=True, + text=True, + ) + self.assertNotEqual(result.returncode, 0) + if expected_stderr_fragment is not None: + self.assertIn(expected_stderr_fragment, result.stderr) + else: + self.assertTrue(result.stderr) + self.assertEqual(result.stdout, "") + def test_get_field_returns_name(self) -> None: manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} self.assertEqual(self.module.get_field(manifest, "name"), "netsuke") @@ -146,17 +165,11 @@ def test_main_prefers_environment_manifest_path(self) -> None: def test_main_reports_missing_manifest(self) -> None: missing = self.temp_path / "missing.toml" - result = self._invoke_cli("name", "--manifest-path", str(missing)) - self.assertNotEqual(result.exit_code, 0) - self.assertIn("does not exist", result.stderr) - self.assertEqual(result.stdout, "") + self._assert_manifest_error(missing, "does not exist") def test_main_reports_invalid_toml(self) -> None: manifest = self._write_manifest("not = [valid") - result = self._invoke_cli("name", "--manifest-path", str(manifest)) - self.assertNotEqual(result.exit_code, 0) - self.assertTrue(result.stderr) - self.assertEqual(result.stdout, "") + self._assert_manifest_error(manifest) def test_main_reports_valid_toml_with_unexpected_structure(self) -> None: manifest = self._write_manifest( From f2afdbd800b565facad8e825ca48b657250344d5 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 3 Oct 2025 22:20:16 +0100 Subject: [PATCH 05/21] Add helper for manifest CLI success assertions --- tests_python/test_read_manifest.py | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 022e553f..6756f622 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -107,6 +107,23 @@ def _assert_manifest_error( self.assertTrue(result.stderr) self.assertEqual(result.stdout, "") + def _assert_successful_field_read( + self, + manifest_content: str, + field: str, + expected_value: str, + *, + cli_args: tuple[str, ...] | None = None, + env: dict[str, str] | None = None, + cwd: Path | None = None, + ) -> None: + manifest = self._write_manifest(manifest_content) + args = cli_args or (field, "--manifest-path", str(manifest)) + result = self._invoke_cli(*args, env=env, cwd=cwd) + self.assertEqual(result.exit_code, 0) + self.assertEqual(result.stdout, expected_value) + self.assertEqual(result.stderr, "") + def test_get_field_returns_name(self) -> None: manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} self.assertEqual(self.module.get_field(manifest, "name"), "netsuke") @@ -137,17 +154,15 @@ def test_get_field_rejects_non_string_values(self) -> None: self.module.get_field(manifest, "metadata") def test_main_reads_manifest_path_argument(self) -> None: - manifest = self._write_manifest( + self._assert_successful_field_read( """ [package] name = "netsuke" version = "1.2.3" - """ + """, + field="name", + expected_value="netsuke", ) - result = self._invoke_cli("name", "--manifest-path", str(manifest)) - self.assertEqual(result.exit_code, 0) - self.assertEqual(result.stdout, "netsuke") - self.assertEqual(result.stderr, "") def test_main_prefers_environment_manifest_path(self) -> None: manifest = self._write_manifest( From 20c6277b563645ba7ab8f0c333b570b11d2f8b28 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 3 Oct 2025 23:00:48 +0100 Subject: [PATCH 06/21] Adopt pytest harness for read_manifest tests --- tests_python/test_read_manifest.py | 272 +++++++++++++++++------------ 1 file changed, 161 insertions(+), 111 deletions(-) diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 6756f622..bef16c4b 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -2,27 +2,26 @@ from __future__ import annotations -from contextlib import ExitStack, contextmanager, redirect_stderr, redirect_stdout -from dataclasses import dataclass +import dataclasses import importlib.util -from io import StringIO import os import subprocess import sys +import types +import typing as typ +from contextlib import ExitStack, contextmanager, redirect_stderr, redirect_stdout +from io import StringIO from pathlib import Path -from tempfile import TemporaryDirectory from textwrap import dedent -from typing import Any - -import unittest -from unittest.mock import patch +from unittest import mock +import pytest REPO_ROOT = Path(__file__).resolve().parent.parent SCRIPT_PATH = REPO_ROOT / ".github" / "workflows" / "scripts" / "read_manifest.py" -@dataclass(slots=True) +@dataclasses.dataclass(slots=True) class CLIResult: """Result container returned by :func:`ReadManifestTests._invoke_cli`.""" @@ -32,9 +31,8 @@ class CLIResult: @contextmanager -def change_directory(path: Path) -> Any: +def change_directory(path: Path) -> typ.Iterator[None]: """Temporarily change the working directory for the current process.""" - original = Path.cwd() os.chdir(path) try: @@ -43,27 +41,26 @@ def change_directory(path: Path) -> Any: os.chdir(original) -def load_script_module() -> Any: +def load_script_module() -> types.ModuleType: + """Import the read_manifest script as a module for reuse in tests.""" spec = importlib.util.spec_from_file_location("read_manifest", SCRIPT_PATH) module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] - assert spec and spec.loader + assert spec is not None + assert spec.loader is not None spec.loader.exec_module(module) # type: ignore[assignment] + assert isinstance(module, types.ModuleType) return module -class ReadManifestTests(unittest.TestCase): - @classmethod - def setUpClass(cls) -> None: - cls.module = load_script_module() +@dataclasses.dataclass(slots=True) +class ReadManifestTests: + """Helpers that exercise the manifest-reading CLI in different scenarios.""" - def setUp(self) -> None: - self.tempdir = TemporaryDirectory() - self.temp_path = Path(self.tempdir.name) - - def tearDown(self) -> None: - self.tempdir.cleanup() + module: types.ModuleType + temp_path: Path def _write_manifest(self, content: str) -> Path: + """Write ``content`` to ``Cargo.toml`` in the temporary directory.""" manifest = self.temp_path / "Cargo.toml" manifest.write_text(dedent(content), encoding="utf-8") return manifest @@ -75,37 +72,49 @@ def _invoke_cli( cwd: Path | None = None, ) -> CLIResult: """Execute the CLI and capture its exit code and output streams.""" - stdout = StringIO() stderr = StringIO() with ExitStack() as stack: - stack.enter_context(patch.object(sys, "argv", [str(SCRIPT_PATH), *args])) + stack.enter_context( + mock.patch.object(sys, "argv", [str(SCRIPT_PATH), *args]) + ) if env: - stack.enter_context(patch.dict(os.environ, env, clear=False)) + stack.enter_context(mock.patch.dict(os.environ, env, clear=False)) if cwd: stack.enter_context(change_directory(cwd)) stack.enter_context(redirect_stdout(stdout)) stack.enter_context(redirect_stderr(stderr)) exit_code = self.module.main() - return CLIResult(exit_code=exit_code, stdout=stdout.getvalue(), stderr=stderr.getvalue()) + return CLIResult( + exit_code=exit_code, + stdout=stdout.getvalue(), + stderr=stderr.getvalue(), + ) def _assert_manifest_error( self, manifest_path: Path, expected_stderr_fragment: str | None = None, ) -> None: - result = subprocess.run( - [sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(manifest_path)], + """Assert that invoking the CLI fails for ``manifest_path``.""" + result = subprocess.run( # noqa: S603 - executed with trusted inputs in tests + [ + sys.executable, + str(SCRIPT_PATH), + "name", + "--manifest-path", + str(manifest_path), + ], check=False, capture_output=True, text=True, ) - self.assertNotEqual(result.returncode, 0) + assert result.returncode != 0 if expected_stderr_fragment is not None: - self.assertIn(expected_stderr_fragment, result.stderr) + assert expected_stderr_fragment in result.stderr else: - self.assertTrue(result.stderr) - self.assertEqual(result.stdout, "") + assert result.stderr + assert result.stdout == "" def _assert_successful_field_read( self, @@ -117,87 +126,128 @@ def _assert_successful_field_read( env: dict[str, str] | None = None, cwd: Path | None = None, ) -> None: + """Assert that the CLI prints ``expected_value`` for ``field``.""" manifest = self._write_manifest(manifest_content) args = cli_args or (field, "--manifest-path", str(manifest)) result = self._invoke_cli(*args, env=env, cwd=cwd) - self.assertEqual(result.exit_code, 0) - self.assertEqual(result.stdout, expected_value) - self.assertEqual(result.stderr, "") - - def test_get_field_returns_name(self) -> None: - manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} - self.assertEqual(self.module.get_field(manifest, "name"), "netsuke") - - def test_get_field_returns_version(self) -> None: - manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} - self.assertEqual(self.module.get_field(manifest, "version"), "1.2.3") - - def test_get_field_raises_when_missing(self) -> None: - manifest = {"package": {"name": "netsuke"}} - with self.assertRaises(KeyError): - self.module.get_field(manifest, "version") - - def test_get_field_rejects_non_string_values(self) -> None: - manifest = { - "package": { - "name": "netsuke", - "version": 123, - "authors": ["alice", "bob"], - "metadata": {"license": "MIT"}, - } - } - with self.assertRaises(KeyError): - self.module.get_field(manifest, "version") - with self.assertRaises(KeyError): - self.module.get_field(manifest, "authors") - with self.assertRaises(KeyError): - self.module.get_field(manifest, "metadata") - - def test_main_reads_manifest_path_argument(self) -> None: - self._assert_successful_field_read( - """ - [package] - name = "netsuke" - version = "1.2.3" - """, - field="name", - expected_value="netsuke", - ) + assert result.exit_code == 0 + assert result.stdout == expected_value + assert result.stderr == "" - def test_main_prefers_environment_manifest_path(self) -> None: - manifest = self._write_manifest( - """ - [package] - name = "netsuke" - version = "1.2.3" - """ - ) - env = {"CARGO_TOML_PATH": str(manifest)} - result = self._invoke_cli("version", env=env, cwd=self.temp_path) - self.assertEqual(result.exit_code, 0) - self.assertEqual(result.stdout, "1.2.3") - self.assertEqual(result.stderr, "") - - def test_main_reports_missing_manifest(self) -> None: - missing = self.temp_path / "missing.toml" - self._assert_manifest_error(missing, "does not exist") - - def test_main_reports_invalid_toml(self) -> None: - manifest = self._write_manifest("not = [valid") - self._assert_manifest_error(manifest) - - def test_main_reports_valid_toml_with_unexpected_structure(self) -> None: - manifest = self._write_manifest( - """ - [unexpected_section] - foo = "bar" - """ - ) - result = self._invoke_cli("name", "--manifest-path", str(manifest)) - self.assertNotEqual(result.exit_code, 0) - self.assertIn("missing", result.stderr.lower()) - self.assertEqual(result.stdout, "") + +@pytest.fixture(scope="module") +def read_manifest_module() -> types.ModuleType: + """Load the read_manifest script once for all tests.""" + return load_script_module() + + +@pytest.fixture +def read_manifest_tests( + read_manifest_module: types.ModuleType, + tmp_path: Path, +) -> ReadManifestTests: + """Provide helpers that operate within a temporary working directory.""" + return ReadManifestTests(module=read_manifest_module, temp_path=tmp_path) + + +def test_get_field_returns_name(read_manifest_module: types.ModuleType) -> None: + """It returns the package name from the manifest.""" + manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} + assert read_manifest_module.get_field(manifest, "name") == "netsuke" -if __name__ == "__main__": - unittest.main() +def test_get_field_returns_version(read_manifest_module: types.ModuleType) -> None: + """It returns the package version from the manifest.""" + manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} + assert read_manifest_module.get_field(manifest, "version") == "1.2.3" + + +def test_get_field_raises_when_missing(read_manifest_module: types.ModuleType) -> None: + """It raises when the requested field is absent.""" + manifest = {"package": {"name": "netsuke"}} + with pytest.raises(KeyError): + read_manifest_module.get_field(manifest, "version") + + +def test_get_field_rejects_non_string_values( + read_manifest_module: types.ModuleType, +) -> None: + """It rejects non-string manifest entries.""" + manifest = { + "package": { + "name": "netsuke", + "version": 123, + "authors": ["alice", "bob"], + "metadata": {"license": "MIT"}, + } + } + with pytest.raises(KeyError): + read_manifest_module.get_field(manifest, "version") + with pytest.raises(KeyError): + read_manifest_module.get_field(manifest, "authors") + with pytest.raises(KeyError): + read_manifest_module.get_field(manifest, "metadata") + + +def test_main_reads_manifest_path_argument( + read_manifest_tests: ReadManifestTests, +) -> None: + """It reads manifests from the path provided via CLI arguments.""" + read_manifest_tests._assert_successful_field_read( + """ + [package] + name = "netsuke" + version = "1.2.3" + """, + field="name", + expected_value="netsuke", + ) + + +def test_main_prefers_environment_manifest_path( + read_manifest_tests: ReadManifestTests, +) -> None: + """It prefers the manifest path supplied via environment variable.""" + manifest = read_manifest_tests._write_manifest( + """ + [package] + name = "netsuke" + version = "1.2.3" + """ + ) + env = {"CARGO_TOML_PATH": str(manifest)} + result = read_manifest_tests._invoke_cli( + "version", + env=env, + cwd=read_manifest_tests.temp_path, + ) + assert result.exit_code == 0 + assert result.stdout == "1.2.3" + assert result.stderr == "" + + +def test_main_reports_missing_manifest( + read_manifest_tests: ReadManifestTests, +) -> None: + """It surfaces errors when the manifest file does not exist.""" + missing = read_manifest_tests.temp_path / "missing.toml" + read_manifest_tests._assert_manifest_error(missing, "does not exist") + + +def test_main_reports_invalid_toml(read_manifest_tests: ReadManifestTests) -> None: + """It surfaces errors for invalid TOML content.""" + manifest = read_manifest_tests._write_manifest("not = [valid") + read_manifest_tests._assert_manifest_error(manifest) + + +def test_main_reports_valid_toml_with_unexpected_structure( + read_manifest_tests: ReadManifestTests, +) -> None: + """It reports a descriptive error when required sections are missing.""" + manifest = read_manifest_tests._write_manifest( + """ + [unexpected_section] + foo = "bar" + """ + ) + read_manifest_tests._assert_manifest_error(manifest, "missing") From 5e88c5ea213ad31b27765da888b53b2acb84f341 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 3 Oct 2025 23:00:53 +0100 Subject: [PATCH 07/21] Parametrize manifest field tests --- tests_python/test_read_manifest.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index bef16c4b..f79c9109 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -150,16 +150,21 @@ def read_manifest_tests( return ReadManifestTests(module=read_manifest_module, temp_path=tmp_path) -def test_get_field_returns_name(read_manifest_module: types.ModuleType) -> None: - """It returns the package name from the manifest.""" - manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} - assert read_manifest_module.get_field(manifest, "name") == "netsuke" - - -def test_get_field_returns_version(read_manifest_module: types.ModuleType) -> None: - """It returns the package version from the manifest.""" +@pytest.mark.parametrize( + ("field", "expected"), + ( + ("name", "netsuke"), + ("version", "1.2.3"), + ), +) +def test_get_field_returns_value( + read_manifest_module: types.ModuleType, + field: str, + expected: str, +) -> None: + """It returns the requested package metadata from the manifest.""" manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} - assert read_manifest_module.get_field(manifest, "version") == "1.2.3" + assert read_manifest_module.get_field(manifest, field) == expected def test_get_field_raises_when_missing(read_manifest_module: types.ModuleType) -> None: From d96e8d60cdc109b6416cba27db58f61c3857269a Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 3 Oct 2025 23:16:45 +0100 Subject: [PATCH 08/21] Refine release asset tooling per review --- .github/workflows/release.yml | 7 ++ scripts/tests/test_upload_release_assets.py | 10 ++- scripts/upload_release_assets.py | 88 ++++++++++++++++----- tests_python/test_read_manifest.py | 22 ++++-- 4 files changed, 99 insertions(+), 28 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 076027ec..8d76707b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -15,6 +15,13 @@ name: Release Binary required: false type: boolean default: false + outputs: + bin_name: + description: Binary name derived from Cargo metadata. + value: ${{ jobs.metadata.outputs.bin_name }} + version: + description: Crate version resolved from Cargo.toml. + value: ${{ jobs.metadata.outputs.version }} env: REPO_NAME: ${{ github.event.repository.name }} diff --git a/scripts/tests/test_upload_release_assets.py b/scripts/tests/test_upload_release_assets.py index 5b987509..7c61f787 100644 --- a/scripts/tests/test_upload_release_assets.py +++ b/scripts/tests/test_upload_release_assets.py @@ -19,10 +19,10 @@ def module(): spec = importlib.util.spec_from_file_location( "upload_release_assets", SCRIPT_PATH ) - module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] - assert spec and spec.loader + module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] # FIXME: stdlib typing lacks precise module_from_spec signature + assert spec and spec.loader, "Failed to load upload_release_assets module spec" sys.modules[spec.name] = module - spec.loader.exec_module(module) # type: ignore[assignment] + spec.loader.exec_module(module) # type: ignore[assignment] # FIXME: Loader.exec_module typing incomplete upstream return module @@ -32,6 +32,7 @@ def create_file(path: Path, content: bytes = b"data") -> None: def test_discover_assets_collects_expected_files(module, tmp_path: Path) -> None: + """Verify that asset discovery preserves expected ordering and names.""" dist = tmp_path / "dist" create_file(dist / "linux" / "netsuke", b"binary") create_file(dist / "linux" / "netsuke.sha256", b"checksum") @@ -53,6 +54,7 @@ def test_discover_assets_collects_expected_files(module, tmp_path: Path) -> None def test_discover_assets_rejects_duplicates(module, tmp_path: Path) -> None: + """It surfaces collisions when multiple files resolve to the same asset.""" dist = tmp_path / "dist" create_file(dist / "a" / "netsuke.pkg", b"pkg-a") create_file(dist / "b" / "netsuke.pkg", b"pkg-b") @@ -64,6 +66,7 @@ def test_discover_assets_rejects_duplicates(module, tmp_path: Path) -> None: def test_discover_assets_rejects_empty_files(module, tmp_path: Path) -> None: + """It refuses to publish zero-byte artefacts.""" dist = tmp_path / "dist" create_file(dist / "linux" / "netsuke", b"") @@ -74,6 +77,7 @@ def test_discover_assets_rejects_empty_files(module, tmp_path: Path) -> None: def test_cli_dry_run_outputs_summary(module, tmp_path: Path) -> None: + """It prints the planned gh commands instead of uploading during dry runs.""" dist = tmp_path / "dist" create_file(dist / "linux" / "netsuke", b"binary") create_file(dist / "linux" / "netsuke.sha256", b"checksum") diff --git a/scripts/upload_release_assets.py b/scripts/upload_release_assets.py index 61b49940..81741667 100755 --- a/scripts/upload_release_assets.py +++ b/scripts/upload_release_assets.py @@ -26,12 +26,14 @@ from dataclasses import dataclass from pathlib import Path -from typing import Annotated, Iterable +from typing import Annotated, Iterable, Iterator import sys import cyclopts from cyclopts import App, Parameter from plumbum import local +from plumbum.commands import CommandNotFound, ProcessExecutionError +from plumbum.commands.base import BoundCommand class AssetError(RuntimeError): @@ -66,6 +68,29 @@ def _resolve_asset_name(path: Path) -> str: return f"{path.parent.name}-{path.name}" +def _iter_candidate_paths(dist_dir: Path, bin_name: str) -> Iterator[Path]: + for path in sorted(dist_dir.rglob("*")): + if path.is_file() and _is_candidate(path, bin_name): + yield path + + +def _require_non_empty(path: Path) -> int: + size = path.stat().st_size + if size <= 0: + raise AssetError(f"Artefact {path} is empty") + return size + + +def _register_asset(asset_name: str, path: Path, seen: dict[str, Path]) -> None: + previous = seen.get(asset_name) + if previous: + raise AssetError( + "Asset name collision: " + f"{asset_name} would upload both {previous} and {path}" + ) + seen[asset_name] = path + + def discover_assets(dist_dir: Path, *, bin_name: str) -> list[ReleaseAsset]: """Return the artefacts that should be published. @@ -94,20 +119,10 @@ def discover_assets(dist_dir: Path, *, bin_name: str) -> list[ReleaseAsset]: assets: list[ReleaseAsset] = [] seen: dict[str, Path] = {} - for path in sorted(p for p in dist_dir.rglob("*") if p.is_file()): - if not _is_candidate(path, bin_name): - continue - size = path.stat().st_size - if size <= 0: - raise AssetError(f"Artefact {path} is empty") + for path in _iter_candidate_paths(dist_dir, bin_name): + size = _require_non_empty(path) asset_name = _resolve_asset_name(path) - previous = seen.get(asset_name) - if previous: - raise AssetError( - "Asset name collision: " - f"{asset_name} would upload both {previous} and {path}" - ) - seen[asset_name] = path + _register_asset(asset_name, path, seen) assets.append(ReleaseAsset(path=path, asset_name=asset_name, size=size)) if not assets: @@ -128,9 +143,27 @@ def _render_summary(assets: Iterable[ReleaseAsset]) -> str: def upload_assets( *, release_tag: str, assets: Iterable[ReleaseAsset], dry_run: bool = False ) -> None: - """Upload artefacts to GitHub using the ``gh`` CLI.""" + """Upload artefacts to GitHub using the ``gh`` CLI. + + Parameters + ---------- + release_tag: + Git tag identifying the release that should receive the artefacts. + assets: + Iterable of artefacts to publish. + dry_run: + When ``True``, print the planned ``gh`` invocations without executing + them. + + Raises + ------ + ProcessExecutionError + If ``gh`` returns a non-zero status while uploading. + CommandNotFound + If the ``gh`` executable is not available in ``PATH``. + """ - gh_cmd = None + gh_cmd: BoundCommand | None = None for asset in assets: descriptor = f"{asset.path}#{asset.asset_name}" if dry_run: @@ -154,7 +187,26 @@ def main( dist_dir: Path = Path("dist"), dry_run: bool = False, ) -> int: - """Entry point shared by the CLI and tests.""" + """Entry point shared by the CLI and tests. + + Parameters + ---------- + release_tag: + Git tag identifying the release to publish to. + bin_name: + Binary name used to derive artefact names during discovery. + dist_dir: + Directory containing staged artefacts. + dry_run: + When ``True``, validate artefacts and print the upload plan without + uploading. + + Returns + ------- + int + Exit code: ``0`` on success, ``1`` when artefact discovery or upload + fails. + """ try: assets = discover_assets(dist_dir, bin_name=bin_name) @@ -167,7 +219,7 @@ def main( try: upload_assets(release_tag=release_tag, assets=assets, dry_run=dry_run) - except Exception as exc: # pragma: no cover - surfaced by plumbum + except (ProcessExecutionError, CommandNotFound) as exc: # pragma: no cover print(exc, file=sys.stderr) return 1 diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index f79c9109..28e8614e 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -30,6 +30,15 @@ class CLIResult: stderr: str +@dataclasses.dataclass(slots=True) +class CLIInvocationConfig: + """Configuration for invoking the CLI when reading manifest fields.""" + + cli_args: tuple[str, ...] | None = None + env: dict[str, str] | None = None + cwd: Path | None = None + + @contextmanager def change_directory(path: Path) -> typ.Iterator[None]: """Temporarily change the working directory for the current process.""" @@ -44,10 +53,10 @@ def change_directory(path: Path) -> typ.Iterator[None]: def load_script_module() -> types.ModuleType: """Import the read_manifest script as a module for reuse in tests.""" spec = importlib.util.spec_from_file_location("read_manifest", SCRIPT_PATH) - module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] + module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] # FIXME: stdlib typing lacks precise module_from_spec signature assert spec is not None assert spec.loader is not None - spec.loader.exec_module(module) # type: ignore[assignment] + spec.loader.exec_module(module) # type: ignore[assignment] # FIXME: Loader.exec_module missing precise type hints assert isinstance(module, types.ModuleType) return module @@ -122,14 +131,13 @@ def _assert_successful_field_read( field: str, expected_value: str, *, - cli_args: tuple[str, ...] | None = None, - env: dict[str, str] | None = None, - cwd: Path | None = None, + config: CLIInvocationConfig | None = None, ) -> None: """Assert that the CLI prints ``expected_value`` for ``field``.""" + config = config or CLIInvocationConfig() manifest = self._write_manifest(manifest_content) - args = cli_args or (field, "--manifest-path", str(manifest)) - result = self._invoke_cli(*args, env=env, cwd=cwd) + args = config.cli_args or (field, "--manifest-path", str(manifest)) + result = self._invoke_cli(*args, env=config.env, cwd=config.cwd) assert result.exit_code == 0 assert result.stdout == expected_value assert result.stderr == "" From 1228f3dcca88841b4d09f42af2bf3ca54c71b444 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 4 Oct 2025 08:06:34 +0100 Subject: [PATCH 09/21] Document manifest test helpers Expand the read_manifest test module docstrings, describe helper\nconfiguration fields, and document the upload asset test helper. Update\nthe release workflow output description to clarify the bin name source. --- .github/workflows/release.yml | 2 +- scripts/tests/test_upload_release_assets.py | 1 + tests_python/test_read_manifest.py | 87 ++++++++++++++++++++- 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 8d76707b..52e132b4 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -17,7 +17,7 @@ name: Release Binary default: false outputs: bin_name: - description: Binary name derived from Cargo metadata. + description: Binary name extracted from Cargo.toml. value: ${{ jobs.metadata.outputs.bin_name }} version: description: Crate version resolved from Cargo.toml. diff --git a/scripts/tests/test_upload_release_assets.py b/scripts/tests/test_upload_release_assets.py index 7c61f787..45cb3289 100644 --- a/scripts/tests/test_upload_release_assets.py +++ b/scripts/tests/test_upload_release_assets.py @@ -27,6 +27,7 @@ def module(): def create_file(path: Path, content: bytes = b"data") -> None: + """Create a file with ``content``, creating parent directories as needed.""" path.parent.mkdir(parents=True, exist_ok=True) path.write_bytes(content) diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 28e8614e..86ee3d84 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -1,4 +1,58 @@ -"""Tests for the read_manifest helper script.""" +""" +Summary +------- +Tests for the ``read_manifest`` helper script that interrogates Cargo +metadata. + +Purpose +------- +Exercise ``.github/workflows/scripts/read_manifest.py`` to confirm it +extracts package fields, honours CLI arguments and environment overrides, +and surfaces descriptive errors for missing manifests, invalid TOML, and +unexpected structure. + +Usage +----- +Run the full suite:: + + python -m pytest tests_python/test_read_manifest.py + +Execute a targeted test:: + + python -m pytest tests_python/test_read_manifest.py::test_get_field_returns_value + +Examples +-------- +Create a minimal manifest and query the package name:: + + from pathlib import Path + from textwrap import dedent + import subprocess + import sys + + path = Path("/tmp/Cargo.toml") + path.write_text(dedent( + """ + [package] + name = "netsuke" + version = "1.2.3" + """ + ), encoding="utf-8") + + completed = subprocess.run( + [ + sys.executable, + ".github/workflows/scripts/read_manifest.py", + "name", + "--manifest-path", + str(path), + ], + check=False, + capture_output=True, + text=True, + ) + assert completed.stdout == "netsuke" +""" from __future__ import annotations @@ -32,7 +86,19 @@ class CLIResult: @dataclasses.dataclass(slots=True) class CLIInvocationConfig: - """Configuration for invoking the CLI when reading manifest fields.""" + """ + Configuration for invoking the CLI when reading manifest fields. + + Attributes + ---------- + cli_args : tuple[str, ...] | None + Optional CLI arguments to pass; defaults to the field and manifest + path pair used by the helpers. + env : dict[str, str] | None + Optional environment overrides provided to the subprocess. + cwd : Path | None + Optional working directory applied during invocation. + """ cli_args: tuple[str, ...] | None = None env: dict[str, str] | None = None @@ -63,7 +129,22 @@ def load_script_module() -> types.ModuleType: @dataclasses.dataclass(slots=True) class ReadManifestTests: - """Helpers that exercise the manifest-reading CLI in different scenarios.""" + """ + Helpers that exercise the manifest-reading CLI in different scenarios. + + Methods + ------- + _write_manifest(content: str) -> Path + Persist TOML content to ``Cargo.toml`` within the temporary + workspace. + _invoke_cli(*args, env=None, cwd=None) -> CLIResult + Execute the CLI in-process while capturing output and exit status. + _assert_manifest_error(manifest_path, expected_stderr_fragment=None) -> None + Assert the CLI fails for a given manifest path and examine stderr. + _assert_successful_field_read(manifest_content, field, expected_value, + *, config=None) -> None + Assert the CLI surfaces the expected field value without errors. + """ module: types.ModuleType temp_path: Path From 1537cc488a84a717ef609c7fa059a501f7ab5d2f Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 4 Oct 2025 08:06:40 +0100 Subject: [PATCH 10/21] Clarify release outputs and test helpers Expose the bin and version metadata from the reusable release workflow and align the manifest test helpers with the requested numpy-style documentation while documenting the test fixture helper. --- .github/workflows/release.yml | 4 ++-- scripts/tests/test_upload_release_assets.py | 4 ++-- tests_python/test_read_manifest.py | 25 ++++++++++----------- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 52e132b4..d8c33a2c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -17,10 +17,10 @@ name: Release Binary default: false outputs: bin_name: - description: Binary name extracted from Cargo.toml. + description: Binary name extracted from Cargo.toml value: ${{ jobs.metadata.outputs.bin_name }} version: - description: Crate version resolved from Cargo.toml. + description: Crate version resolved from Cargo.toml value: ${{ jobs.metadata.outputs.version }} env: diff --git a/scripts/tests/test_upload_release_assets.py b/scripts/tests/test_upload_release_assets.py index 45cb3289..176b2b59 100644 --- a/scripts/tests/test_upload_release_assets.py +++ b/scripts/tests/test_upload_release_assets.py @@ -27,7 +27,7 @@ def module(): def create_file(path: Path, content: bytes = b"data") -> None: - """Create a file with ``content``, creating parent directories as needed.""" + """Create a file with the given content, ensuring parent directories exist.""" path.parent.mkdir(parents=True, exist_ok=True) path.write_bytes(content) @@ -83,7 +83,7 @@ def test_cli_dry_run_outputs_summary(module, tmp_path: Path) -> None: create_file(dist / "linux" / "netsuke", b"binary") create_file(dist / "linux" / "netsuke.sha256", b"checksum") - result = subprocess.run( + result = subprocess.run( # noqa: S603 - trusted arguments within tests [ sys.executable, str(SCRIPT_PATH), diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 86ee3d84..9e6ab4d4 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -32,11 +32,11 @@ path = Path("/tmp/Cargo.toml") path.write_text(dedent( - """ + ''' [package] name = "netsuke" version = "1.2.3" - """ + ''' ), encoding="utf-8") completed = subprocess.run( @@ -92,12 +92,11 @@ class CLIInvocationConfig: Attributes ---------- cli_args : tuple[str, ...] | None - Optional CLI arguments to pass; defaults to the field and manifest - path pair used by the helpers. + Optional CLI arguments to pass; defaults to field and manifest path. env : dict[str, str] | None - Optional environment overrides provided to the subprocess. + Optional environment variables to set during invocation. cwd : Path | None - Optional working directory applied during invocation. + Optional working directory for the invocation. """ cli_args: tuple[str, ...] | None = None @@ -135,15 +134,15 @@ class ReadManifestTests: Methods ------- _write_manifest(content: str) -> Path - Persist TOML content to ``Cargo.toml`` within the temporary - workspace. + Write TOML content to a temporary ``Cargo.toml``. _invoke_cli(*args, env=None, cwd=None) -> CLIResult - Execute the CLI in-process while capturing output and exit status. + Execute the CLI in-process and capture output. _assert_manifest_error(manifest_path, expected_stderr_fragment=None) -> None - Assert the CLI fails for a given manifest path and examine stderr. - _assert_successful_field_read(manifest_content, field, expected_value, - *, config=None) -> None - Assert the CLI surfaces the expected field value without errors. + Assert the CLI fails for the given manifest path. + _assert_successful_field_read( + manifest_content, field, expected_value, *, config=None + ) -> None + Assert the CLI prints the expected value for the field. """ module: types.ModuleType From 690e4ae8037ef6300efd2d3a45c07fd1e1e39602 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 4 Oct 2025 08:57:34 +0100 Subject: [PATCH 11/21] Document manifest loader helper - expand the load_script_module docstring to follow the NumPy template - add FIXME rationales to subprocess suppressions in CLI tests --- scripts/tests/test_upload_release_assets.py | 2 +- tests_python/test_read_manifest.py | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/scripts/tests/test_upload_release_assets.py b/scripts/tests/test_upload_release_assets.py index 176b2b59..a59560a9 100644 --- a/scripts/tests/test_upload_release_assets.py +++ b/scripts/tests/test_upload_release_assets.py @@ -83,7 +83,7 @@ def test_cli_dry_run_outputs_summary(module, tmp_path: Path) -> None: create_file(dist / "linux" / "netsuke", b"binary") create_file(dist / "linux" / "netsuke.sha256", b"checksum") - result = subprocess.run( # noqa: S603 - trusted arguments within tests + result = subprocess.run( # noqa: S603 # FIXME: subprocess required for CLI integration test with trusted arguments [ sys.executable, str(SCRIPT_PATH), diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 9e6ab4d4..46ff427b 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -116,7 +116,21 @@ def change_directory(path: Path) -> typ.Iterator[None]: def load_script_module() -> types.ModuleType: - """Import the read_manifest script as a module for reuse in tests.""" + """ + Import the read_manifest script as a module for reuse in tests. + + Returns + ------- + types.ModuleType + The loaded ``read_manifest`` module exposing its helper functions. + + Examples + -------- + >>> module = load_script_module() + >>> manifest = {"package": {"name": "foo"}} + >>> module.get_field(manifest, "name") + 'foo' + """ spec = importlib.util.spec_from_file_location("read_manifest", SCRIPT_PATH) module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] # FIXME: stdlib typing lacks precise module_from_spec signature assert spec is not None @@ -186,7 +200,7 @@ def _assert_manifest_error( expected_stderr_fragment: str | None = None, ) -> None: """Assert that invoking the CLI fails for ``manifest_path``.""" - result = subprocess.run( # noqa: S603 - executed with trusted inputs in tests + result = subprocess.run( # noqa: S603 # FIXME: executed with trusted inputs in tests [ sys.executable, str(SCRIPT_PATH), From 244f1373887e49b926c8b6b79d11dde5fbee2f21 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 4 Oct 2025 08:57:40 +0100 Subject: [PATCH 12/21] Expose workflow outputs and expand docstring Surface the reusable release workflow outputs so callers can read the binary name, version, and publish flag. Align the manifest loader helper with the review guidance by providing a full NumPy-style docstring. --- .github/workflows/release.yml | 3 +++ tests_python/test_read_manifest.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index d8c33a2c..167d0b68 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -22,6 +22,9 @@ name: Release Binary version: description: Crate version resolved from Cargo.toml value: ${{ jobs.metadata.outputs.version }} + should_publish: + description: Whether artefacts should be published to the release + value: ${{ jobs.metadata.outputs.should_publish }} env: REPO_NAME: ${{ github.event.repository.name }} diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 46ff427b..2f854897 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -122,7 +122,7 @@ def load_script_module() -> types.ModuleType: Returns ------- types.ModuleType - The loaded ``read_manifest`` module exposing its helper functions. + The loaded read_manifest module with all its functions and constants. Examples -------- From a883684d3a64600ac19112c54101309e1c6f5f47 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 4 Oct 2025 18:14:50 +0100 Subject: [PATCH 13/21] Refine manifest helpers and release asset tests --- .github/workflows/scripts/read_manifest.py | 31 ++++++----- scripts/tests/test_upload_release_assets.py | 58 ++++++++++++++------- scripts/upload_release_assets.py | 48 +++++++++-------- tests_python/test_read_manifest.py | 21 +++++--- 4 files changed, 97 insertions(+), 61 deletions(-) diff --git a/.github/workflows/scripts/read_manifest.py b/.github/workflows/scripts/read_manifest.py index 92f99163..6d127155 100755 --- a/.github/workflows/scripts/read_manifest.py +++ b/.github/workflows/scripts/read_manifest.py @@ -10,18 +10,19 @@ import tomllib +PARSER_DESCRIPTION = " ".join( + [ + "Read selected fields from a Cargo.toml manifest and print them to", + "stdout.", + ] +) + def parse_args() -> argparse.Namespace: - parser = argparse.ArgumentParser( - description=( - "Read selected fields from a Cargo.toml manifest and print them to " - "stdout." - ) - ) + """Return the parsed CLI arguments for manifest field extraction.""" + parser = argparse.ArgumentParser(description=PARSER_DESCRIPTION) parser.add_argument( - "field", - choices=("name", "version"), - help="The manifest field to print." + "field", choices=("name", "version"), help="The manifest field to print." ) parser.add_argument( "--manifest-path", @@ -36,23 +37,29 @@ def parse_args() -> argparse.Namespace: def read_manifest(path: Path) -> dict[str, object]: + """Load and return the parsed Cargo manifest as a dictionary.""" if not path.is_file(): - raise FileNotFoundError(f"Manifest {path} does not exist") + message = f"Manifest {path} does not exist" + raise FileNotFoundError(message) with path.open("rb") as handle: return tomllib.load(handle) def get_field(manifest: dict[str, object], field: str) -> str: + """Extract a package field from the manifest, raising if it is missing.""" package = manifest.get("package") or {} if not isinstance(package, dict): - raise KeyError("package table missing from manifest") + message = "package table missing from manifest" + raise KeyError(message) value = package.get(field, "") if not isinstance(value, str) or not value: - raise KeyError(f"package.{field} is missing") + message = f"package.{field} is missing" + raise KeyError(message) return value def main() -> int: + """Entry point for the manifest reader CLI.""" args = parse_args() manifest_path = args.manifest_path or os.environ.get( "CARGO_TOML_PATH", "Cargo.toml" diff --git a/scripts/tests/test_upload_release_assets.py b/scripts/tests/test_upload_release_assets.py index a59560a9..354cb51f 100644 --- a/scripts/tests/test_upload_release_assets.py +++ b/scripts/tests/test_upload_release_assets.py @@ -5,24 +5,34 @@ import importlib.util import subprocess import sys +import typing as typ from pathlib import Path import pytest - REPO_ROOT = Path(__file__).resolve().parents[2] SCRIPT_PATH = REPO_ROOT / "scripts" / "upload_release_assets.py" +if typ.TYPE_CHECKING: + from types import ModuleType +else: + ModuleType = type(sys) + + @pytest.fixture(scope="session") -def module(): - spec = importlib.util.spec_from_file_location( - "upload_release_assets", SCRIPT_PATH - ) - module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] # FIXME: stdlib typing lacks precise module_from_spec signature - assert spec and spec.loader, "Failed to load upload_release_assets module spec" +def module() -> ModuleType: + """Load the upload_release_assets script once for reuse across tests.""" + spec = importlib.util.spec_from_file_location("upload_release_assets", SCRIPT_PATH) + if spec is None: + message = "Failed to create module spec for upload_release_assets" + raise RuntimeError(message) + if spec.loader is None: + message = "Module spec missing loader for upload_release_assets" + raise RuntimeError(message) + module = importlib.util.module_from_spec(spec) sys.modules[spec.name] = module - spec.loader.exec_module(module) # type: ignore[assignment] # FIXME: Loader.exec_module typing incomplete upstream + spec.loader.exec_module(module) return module @@ -32,7 +42,9 @@ def create_file(path: Path, content: bytes = b"data") -> None: path.write_bytes(content) -def test_discover_assets_collects_expected_files(module, tmp_path: Path) -> None: +def test_discover_assets_collects_expected_files( + module: ModuleType, tmp_path: Path +) -> None: """Verify that asset discovery preserves expected ordering and names.""" dist = tmp_path / "dist" create_file(dist / "linux" / "netsuke", b"binary") @@ -51,10 +63,10 @@ def test_discover_assets_collects_expected_files(module, tmp_path: Path) -> None "man-netsuke.1", "windows-netsuke.exe", "windows-netsuke.msi", - ] + ], "Asset discovery order does not match expected sequence" -def test_discover_assets_rejects_duplicates(module, tmp_path: Path) -> None: +def test_discover_assets_rejects_duplicates(module: ModuleType, tmp_path: Path) -> None: """It surfaces collisions when multiple files resolve to the same asset.""" dist = tmp_path / "dist" create_file(dist / "a" / "netsuke.pkg", b"pkg-a") @@ -66,7 +78,9 @@ def test_discover_assets_rejects_duplicates(module, tmp_path: Path) -> None: assert "Asset name collision" in str(exc.value) -def test_discover_assets_rejects_empty_files(module, tmp_path: Path) -> None: +def test_discover_assets_rejects_empty_files( + module: ModuleType, tmp_path: Path +) -> None: """It refuses to publish zero-byte artefacts.""" dist = tmp_path / "dist" create_file(dist / "linux" / "netsuke", b"") @@ -77,13 +91,13 @@ def test_discover_assets_rejects_empty_files(module, tmp_path: Path) -> None: assert "is empty" in str(exc.value) -def test_cli_dry_run_outputs_summary(module, tmp_path: Path) -> None: +def test_cli_dry_run_outputs_summary(module: ModuleType, tmp_path: Path) -> None: """It prints the planned gh commands instead of uploading during dry runs.""" dist = tmp_path / "dist" create_file(dist / "linux" / "netsuke", b"binary") create_file(dist / "linux" / "netsuke.sha256", b"checksum") - result = subprocess.run( # noqa: S603 # FIXME: subprocess required for CLI integration test with trusted arguments + result = subprocess.run( # noqa: S603 # Security: CLI invocation uses trusted arguments in tests. [ sys.executable, str(SCRIPT_PATH), @@ -100,8 +114,14 @@ def test_cli_dry_run_outputs_summary(module, tmp_path: Path) -> None: check=False, ) - assert result.returncode == 0 - assert "Planned uploads:" in result.stdout - assert "linux-netsuke" in result.stdout - assert "linux-netsuke.sha256" in result.stdout - assert "[dry-run] gh release upload v1.2.3" in result.stdout + assert result.returncode == 0, "CLI dry-run should exit successfully" + assert "Planned uploads:" in result.stdout, ( + "Dry-run output should include upload summary" + ) + assert "linux-netsuke" in result.stdout, "Dry-run output should list linux binary" + assert "linux-netsuke.sha256" in result.stdout, ( + "Dry-run output should list checksum file" + ) + assert "[dry-run] gh release upload v1.2.3" in result.stdout, ( + "Dry-run output should show gh command" + ) diff --git a/scripts/upload_release_assets.py b/scripts/upload_release_assets.py index 81741667..ea74a201 100755 --- a/scripts/upload_release_assets.py +++ b/scripts/upload_release_assets.py @@ -24,23 +24,27 @@ from __future__ import annotations -from dataclasses import dataclass -from pathlib import Path -from typing import Annotated, Iterable, Iterator +import dataclasses as dc import sys +import typing as typ +from pathlib import Path import cyclopts from cyclopts import App, Parameter from plumbum import local from plumbum.commands import CommandNotFound, ProcessExecutionError -from plumbum.commands.base import BoundCommand + +if typ.TYPE_CHECKING: + import collections.abc as cabc + + from plumbum.commands.base import BoundCommand class AssetError(RuntimeError): """Raised when the staged artefacts are invalid.""" -@dataclass(frozen=True) +@dc.dataclass(frozen=True) class ReleaseAsset: """Artefact staged for upload to a GitHub release.""" @@ -68,7 +72,7 @@ def _resolve_asset_name(path: Path) -> str: return f"{path.parent.name}-{path.name}" -def _iter_candidate_paths(dist_dir: Path, bin_name: str) -> Iterator[Path]: +def _iter_candidate_paths(dist_dir: Path, bin_name: str) -> cabc.Iterator[Path]: for path in sorted(dist_dir.rglob("*")): if path.is_file() and _is_candidate(path, bin_name): yield path @@ -77,17 +81,19 @@ def _iter_candidate_paths(dist_dir: Path, bin_name: str) -> Iterator[Path]: def _require_non_empty(path: Path) -> int: size = path.stat().st_size if size <= 0: - raise AssetError(f"Artefact {path} is empty") + message = f"Artefact {path} is empty" + raise AssetError(message) return size def _register_asset(asset_name: str, path: Path, seen: dict[str, Path]) -> None: previous = seen.get(asset_name) if previous: - raise AssetError( + message = ( "Asset name collision: " f"{asset_name} would upload both {previous} and {path}" ) + raise AssetError(message) seen[asset_name] = path @@ -112,9 +118,9 @@ def discover_assets(dist_dir: Path, *, bin_name: str) -> list[ReleaseAsset]: If no artefacts are found, an artefact is empty, or multiple files would upload with the same asset name. """ - if not dist_dir.exists(): - raise AssetError(f"Artefact directory {dist_dir} does not exist") + message = f"Artefact directory {dist_dir} does not exist" + raise AssetError(message) assets: list[ReleaseAsset] = [] seen: dict[str, Path] = {} @@ -126,22 +132,23 @@ def discover_assets(dist_dir: Path, *, bin_name: str) -> list[ReleaseAsset]: assets.append(ReleaseAsset(path=path, asset_name=asset_name, size=size)) if not assets: - raise AssetError(f"No artefacts discovered in {dist_dir}") + message = f"No artefacts discovered in {dist_dir}" + raise AssetError(message) return assets -def _render_summary(assets: Iterable[ReleaseAsset]) -> str: +def _render_summary(assets: cabc.Iterable[ReleaseAsset]) -> str: lines = ["Planned uploads:"] - for asset in assets: - lines.append( - f" - {asset.asset_name} ({asset.size} bytes) -> {asset.path}" - ) + lines.extend( + f" - {asset.asset_name} ({asset.size} bytes) -> {asset.path}" + for asset in assets + ) return "\n".join(lines) def upload_assets( - *, release_tag: str, assets: Iterable[ReleaseAsset], dry_run: bool = False + *, release_tag: str, assets: cabc.Iterable[ReleaseAsset], dry_run: bool = False ) -> None: """Upload artefacts to GitHub using the ``gh`` CLI. @@ -162,7 +169,6 @@ def upload_assets( CommandNotFound If the ``gh`` executable is not available in ``PATH``. """ - gh_cmd: BoundCommand | None = None for asset in assets: descriptor = f"{asset.path}#{asset.asset_name}" @@ -207,7 +213,6 @@ def main( Exit code: ``0`` on success, ``1`` when artefact discovery or upload fails. """ - try: assets = discover_assets(dist_dir, bin_name=bin_name) except AssetError as exc: @@ -229,13 +234,12 @@ def main( @app.default def cli( *, - release_tag: Annotated[str, Parameter(required=True)], - bin_name: Annotated[str, Parameter(required=True)], + release_tag: typ.Annotated[str, Parameter(required=True)], + bin_name: typ.Annotated[str, Parameter(required=True)], dist_dir: Path = Path("dist"), dry_run: bool = False, ) -> int: """Cyclopts-bound CLI entry point.""" - return main( release_tag=release_tag, bin_name=bin_name, diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 2f854897..7938265b 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -1,4 +1,5 @@ -""" +"""Comprehensive tests for the read_manifest helper script. + Summary ------- Tests for the ``read_manifest`` helper script that interrogates Cargo @@ -132,10 +133,14 @@ def load_script_module() -> types.ModuleType: 'foo' """ spec = importlib.util.spec_from_file_location("read_manifest", SCRIPT_PATH) - module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] # FIXME: stdlib typing lacks precise module_from_spec signature - assert spec is not None - assert spec.loader is not None - spec.loader.exec_module(module) # type: ignore[assignment] # FIXME: Loader.exec_module missing precise type hints + if spec is None: + message = "Failed to create module spec for read_manifest" + raise RuntimeError(message) + if spec.loader is None: + message = "Module spec missing loader for read_manifest" + raise RuntimeError(message) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) assert isinstance(module, types.ModuleType) return module @@ -200,7 +205,7 @@ def _assert_manifest_error( expected_stderr_fragment: str | None = None, ) -> None: """Assert that invoking the CLI fails for ``manifest_path``.""" - result = subprocess.run( # noqa: S603 # FIXME: executed with trusted inputs in tests + result = subprocess.run( # noqa: S603 # Security: executed with trusted inputs in tests. [ sys.executable, str(SCRIPT_PATH), @@ -254,10 +259,10 @@ def read_manifest_tests( @pytest.mark.parametrize( ("field", "expected"), - ( + [ ("name", "netsuke"), ("version", "1.2.3"), - ), + ], ) def test_get_field_returns_value( read_manifest_module: types.ModuleType, From bb24d348e2cbf1df6dc6040f7f7a253cee7fe8cf Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 4 Oct 2025 18:30:51 +0100 Subject: [PATCH 14/21] Document manifest helpers and test fixture --- .github/workflows/scripts/read_manifest.py | 56 +++++++++++++++++++++- tests_python/test_read_manifest.py | 21 +++++++- 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/.github/workflows/scripts/read_manifest.py b/.github/workflows/scripts/read_manifest.py index 6d127155..a97871a0 100755 --- a/.github/workflows/scripts/read_manifest.py +++ b/.github/workflows/scripts/read_manifest.py @@ -37,7 +37,34 @@ def parse_args() -> argparse.Namespace: def read_manifest(path: Path) -> dict[str, object]: - """Load and return the parsed Cargo manifest as a dictionary.""" + """ + Load and return the parsed Cargo manifest as a dictionary. + + Parameters + ---------- + path : Path + Path to the ``Cargo.toml`` file. + + Returns + ------- + dict[str, object] + Parsed manifest fields keyed by section. + + Raises + ------ + FileNotFoundError + If the manifest file does not exist. + tomllib.TOMLDecodeError + If the manifest contains invalid TOML syntax. + + Examples + -------- + >>> from pathlib import Path + >>> manifest_path = Path("Cargo.toml") + >>> data = read_manifest(manifest_path) + >>> "package" in data + True + """ if not path.is_file(): message = f"Manifest {path} does not exist" raise FileNotFoundError(message) @@ -46,7 +73,32 @@ def read_manifest(path: Path) -> dict[str, object]: def get_field(manifest: dict[str, object], field: str) -> str: - """Extract a package field from the manifest, raising if it is missing.""" + """ + Extract a package field from the manifest, raising if it is missing. + + Parameters + ---------- + manifest : dict[str, object] + The parsed Cargo manifest dictionary. + field : str + The package field to extract, such as ``"name"`` or ``"version"``. + + Returns + ------- + str + The non-empty field value from the package section. + + Raises + ------ + KeyError + If the package table is missing or the field is absent or blank. + + Examples + -------- + >>> manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} + >>> get_field(manifest, "name") + 'netsuke' + """ package = manifest.get("package") or {} if not isinstance(package, dict): message = "package table missing from manifest" diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 7938265b..5336f4f9 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -107,7 +107,26 @@ class CLIInvocationConfig: @contextmanager def change_directory(path: Path) -> typ.Iterator[None]: - """Temporarily change the working directory for the current process.""" + """ + Temporarily change the working directory for the current process. + + Parameters + ---------- + path : Path + Target directory that should become the working directory. + + Yields + ------ + None + Control to the caller while the working directory is set to ``path``. + + Examples + -------- + >>> from pathlib import Path + >>> with change_directory(Path("/tmp")): + ... Path.cwd().as_posix() + '/tmp' + """ original = Path.cwd() os.chdir(path) try: From 2f2c53845664c56fac7a3604837edfcc2fb10b77 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 5 Oct 2025 01:08:19 +0100 Subject: [PATCH 15/21] Address review feedback on workflow outputs --- .github/workflows/release.yml | 2 +- ruff.toml | 3 ++- tests_python/test_read_manifest.py | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 167d0b68..d940ce94 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,7 +1,7 @@ --- name: Release Binary -'on': +on: push: tags: - 'v*.*.*' diff --git a/ruff.toml b/ruff.toml index d1db8276..50c5da8c 100644 --- a/ruff.toml +++ b/ruff.toml @@ -38,6 +38,7 @@ select = [ ] per-file-ignores = {"**/test_*.py" = ["S101"]} ignore = ["D205"] +task-tags = ["TODO", "FIXME"] [lint.flake8-import-conventions] # Declare the banned `from` imports. @@ -61,5 +62,5 @@ msgspec = "ms" typing = "typ" [lint.pydocstyle] -# Enforce NumPy docstring +# Enforce NumPy docstring convention = "numpy" diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 5336f4f9..66ca7ad6 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -224,7 +224,7 @@ def _assert_manifest_error( expected_stderr_fragment: str | None = None, ) -> None: """Assert that invoking the CLI fails for ``manifest_path``.""" - result = subprocess.run( # noqa: S603 # Security: executed with trusted inputs in tests. + result = subprocess.run( # noqa: S603 # TODO(release-ci): FIXME: Security false positive; executed with trusted inputs in tests. https://github.com/leynos/netsuke/pull/179#discussion_r2404108802 [ sys.executable, str(SCRIPT_PATH), From f9d67ac46c3fc34e076fd01ced7667c094d276a9 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 5 Oct 2025 01:52:44 +0100 Subject: [PATCH 16/21] Document CLIResult attributes --- tests_python/test_read_manifest.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index 66ca7ad6..b4011a62 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -78,7 +78,17 @@ @dataclasses.dataclass(slots=True) class CLIResult: - """Result container returned by :func:`ReadManifestTests._invoke_cli`.""" + """CLI invocation outcome. + + Attributes + ---------- + exit_code : int + Exit status returned by the CLI process. + stdout : str + Captured standard output emitted by the CLI. + stderr : str + Captured standard error emitted by the CLI. + """ exit_code: int stdout: str From 2c61bf9da4c2444562cf88dd8f40fb10cc4fc570 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 5 Oct 2025 01:53:29 +0100 Subject: [PATCH 17/21] Clarify CLIResult documentation --- .github/workflows/scripts/read_manifest.py | 73 +++++++++++++++++++++- tests_python/test_read_manifest.py | 3 +- 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/.github/workflows/scripts/read_manifest.py b/.github/workflows/scripts/read_manifest.py index a97871a0..477909c1 100755 --- a/.github/workflows/scripts/read_manifest.py +++ b/.github/workflows/scripts/read_manifest.py @@ -1,5 +1,43 @@ #!/usr/bin/env python3 -"""Utility helpers for extracting fields from Cargo.toml.""" +""" +Utility helpers for extracting fields from Cargo.toml. + +Summary +------- +Parse and extract package metadata fields from Cargo manifest files. + +Purpose +------- +Provide both a CLI tool and programmatic API for reading ``name`` and +``version`` fields from Cargo.toml manifests, with robust error handling +for missing files, invalid TOML, and unexpected structure. + +Usage +----- +CLI invocation:: + + python read_manifest.py name --manifest-path /path/to/Cargo.toml + python read_manifest.py version + +Programmatic usage:: + + from pathlib import Path + manifest = read_manifest(Path("Cargo.toml")) + name = get_field(manifest, "name") + +Examples +-------- +Extract the package name from a manifest:: + + $ python read_manifest.py name --manifest-path Cargo.toml + netsuke + +Use the CARGO_TOML_PATH environment variable:: + + $ export CARGO_TOML_PATH=/path/to/Cargo.toml + $ python read_manifest.py version + 1.2.3 +""" from __future__ import annotations @@ -19,7 +57,21 @@ def parse_args() -> argparse.Namespace: - """Return the parsed CLI arguments for manifest field extraction.""" + """ + Return the parsed CLI arguments for manifest field extraction. + + Returns + ------- + argparse.Namespace + Parsed arguments containing ``field`` (str) and optional + ``manifest_path`` (str or None). + + Examples + -------- + >>> args = parse_args() # With sys.argv = ["script.py", "name"] + >>> args.field + 'name' + """ parser = argparse.ArgumentParser(description=PARSER_DESCRIPTION) parser.add_argument( "field", choices=("name", "version"), help="The manifest field to print." @@ -111,7 +163,22 @@ def get_field(manifest: dict[str, object], field: str) -> str: def main() -> int: - """Entry point for the manifest reader CLI.""" + """ + Entry point for the manifest reader CLI. + + Returns + ------- + int + Exit code: 0 for success, 1 for errors (missing file, invalid + TOML, or missing fields). + + Examples + -------- + Typical CLI invocation:: + + $ python read_manifest.py name --manifest-path Cargo.toml + netsuke + """ args = parse_args() manifest_path = args.manifest_path or os.environ.get( "CARGO_TOML_PATH", "Cargo.toml" diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index b4011a62..eaad458f 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -78,7 +78,8 @@ @dataclasses.dataclass(slots=True) class CLIResult: - """CLI invocation outcome. + """ + Result container returned by ReadManifestTests._invoke_cli. Attributes ---------- From 745eba0eed64a3b01e4d27b5f968385024eb7863 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 5 Oct 2025 14:22:04 +0100 Subject: [PATCH 18/21] Simplify asset collision guard --- scripts/upload_release_assets.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/upload_release_assets.py b/scripts/upload_release_assets.py index ea74a201..91110fdd 100755 --- a/scripts/upload_release_assets.py +++ b/scripts/upload_release_assets.py @@ -87,8 +87,7 @@ def _require_non_empty(path: Path) -> int: def _register_asset(asset_name: str, path: Path, seen: dict[str, Path]) -> None: - previous = seen.get(asset_name) - if previous: + if previous := seen.get(asset_name): message = ( "Asset name collision: " f"{asset_name} would upload both {previous} and {path}" From f04d2632dd16ae63c74836d6589a1a112826789a Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 5 Oct 2025 14:23:29 +0100 Subject: [PATCH 19/21] Suppress semgrep false positive in manifest tests --- tests_python/test_read_manifest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests_python/test_read_manifest.py b/tests_python/test_read_manifest.py index eaad458f..94033974 100644 --- a/tests_python/test_read_manifest.py +++ b/tests_python/test_read_manifest.py @@ -235,6 +235,7 @@ def _assert_manifest_error( expected_stderr_fragment: str | None = None, ) -> None: """Assert that invoking the CLI fails for ``manifest_path``.""" + # nosemgrep: python.lang.security.audit.dangerous-subprocess-use-audit.dangerous-subprocess-use-audit -- test-only; argv fully controlled # noqa: E501 result = subprocess.run( # noqa: S603 # TODO(release-ci): FIXME: Security false positive; executed with trusted inputs in tests. https://github.com/leynos/netsuke/pull/179#discussion_r2404108802 [ sys.executable, From 5187b3115b17f6a8ad63378f32d7a4c1dbdf0f40 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 5 Oct 2025 15:25:47 +0100 Subject: [PATCH 20/21] Harden release asset uploader --- .../actions/upload-release-assets/action.yml | 31 +++++++++-- .github/workflows/release-dry-run.yml | 9 +++- .github/workflows/release.yml | 9 +++- scripts/tests/test_upload_release_assets.py | 51 ++++++++++++++++--- scripts/upload_release_assets.py | 34 ++++++++++--- 5 files changed, 113 insertions(+), 21 deletions(-) diff --git a/.github/actions/upload-release-assets/action.yml b/.github/actions/upload-release-assets/action.yml index 19eb3bcc..39c42aad 100644 --- a/.github/actions/upload-release-assets/action.yml +++ b/.github/actions/upload-release-assets/action.yml @@ -15,10 +15,18 @@ inputs: description: When true, only validate artefacts and print the upload plan. required: false default: "false" +outputs: + upload-error: + description: Whether the invocation detected an error. + value: ${{ steps.invoke.outputs.upload-error }} + error-message: + description: Summary of the error when ``upload-error`` is ``true``. + value: ${{ steps.invoke.outputs.error-message }} runs: using: composite steps: - - name: Upload release artefacts + - id: invoke + name: Upload release artefacts shell: bash env: INPUT_RELEASE_TAG: ${{ inputs.release-tag }} @@ -26,5 +34,22 @@ runs: INPUT_DIST_DIR: ${{ inputs.dist-dir }} INPUT_DRY_RUN: ${{ inputs.dry-run }} run: | - set -euo pipefail - "$GITHUB_WORKSPACE/scripts/upload_release_assets.py" + set -uo pipefail + log_file="$(mktemp)" + if "$GITHUB_WORKSPACE/scripts/upload_release_assets.py" >"$log_file" 2>&1; then + cat "$log_file" + { + echo "upload-error=false" + echo "error-message=" + } >>"$GITHUB_OUTPUT" + else + status=$? + cat "$log_file" + { + echo "upload-error=true" + echo "error-message<<'EOF'" + cat "$log_file" + printf 'Exit code: %s\n' "$status" + echo 'EOF' + } >>"$GITHUB_OUTPUT" + fi diff --git a/.github/workflows/release-dry-run.yml b/.github/workflows/release-dry-run.yml index 02ebf842..4dfe3353 100644 --- a/.github/workflows/release-dry-run.yml +++ b/.github/workflows/release-dry-run.yml @@ -27,10 +27,17 @@ jobs: with: path: dist pattern: ${{ env.REPO_NAME }}-* - - name: Validate artefacts + - id: validate + name: Validate artefacts uses: ./.github/actions/upload-release-assets with: release-tag: ${{ github.ref_name }} bin-name: ${{ needs.release.outputs.bin_name }} dist-dir: dist dry-run: "true" + - name: Check validation errors + if: steps.validate.outputs.upload-error == 'true' + run: | + echo "Error validating release assets:" + printf '%s\n' "${{ steps.validate.outputs.error-message }}" + exit 1 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index d940ce94..877a2033 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -308,7 +308,8 @@ jobs: with: path: dist pattern: ${{ env.REPO_NAME }}-* - - name: Upload artefacts to release + - id: upload_assets + name: Upload artefacts to release uses: ./.github/actions/upload-release-assets with: release-tag: ${{ github.ref_name }} @@ -317,3 +318,9 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Check asset upload errors + if: steps.upload_assets.outputs.upload-error == 'true' + run: | + echo "Error uploading release assets:" + printf '%s\n' "${{ steps.upload_assets.outputs.error-message }}" + exit 1 diff --git a/scripts/tests/test_upload_release_assets.py b/scripts/tests/test_upload_release_assets.py index 354cb51f..52c12d52 100644 --- a/scripts/tests/test_upload_release_assets.py +++ b/scripts/tests/test_upload_release_assets.py @@ -58,7 +58,7 @@ def test_discover_assets_collects_expected_files( assert [asset.asset_name for asset in assets] == [ "linux-netsuke", - "netsuke.deb", + "linux-netsuke.deb", "linux-netsuke.sha256", "man-netsuke.1", "windows-netsuke.exe", @@ -66,16 +66,22 @@ def test_discover_assets_collects_expected_files( ], "Asset discovery order does not match expected sequence" -def test_discover_assets_rejects_duplicates(module: ModuleType, tmp_path: Path) -> None: - """It surfaces collisions when multiple files resolve to the same asset.""" +def test_discover_assets_disambiguates_duplicate_filenames( + module: ModuleType, tmp_path: Path +) -> None: + """It prefixes nested directories to avoid collisions for package files.""" dist = tmp_path / "dist" - create_file(dist / "a" / "netsuke.pkg", b"pkg-a") - create_file(dist / "b" / "netsuke.pkg", b"pkg-b") + create_file(dist / "linux" / "pkg" / "netsuke.pkg", b"pkg-linux") + create_file(dist / "windows" / "pkg" / "netsuke.pkg", b"pkg-windows") - with pytest.raises(module.AssetError) as exc: - module.discover_assets(dist, bin_name="netsuke") + assets = module.discover_assets(dist, bin_name="netsuke") - assert "Asset name collision" in str(exc.value) + assert [ + asset.asset_name for asset in assets if asset.asset_name.endswith(".pkg") + ] == [ + "linux__pkg-netsuke.pkg", + "windows__pkg-netsuke.pkg", + ] def test_discover_assets_rejects_empty_files( @@ -91,6 +97,35 @@ def test_discover_assets_rejects_empty_files( assert "is empty" in str(exc.value) +@pytest.mark.parametrize( + ("value", "expected"), + [ + (True, True), + (False, False), + ("true", True), + ("false", False), + ("YES", True), + ("no", False), + ("1", True), + ("0", False), + (" on ", True), + (" off ", False), + ("", False), + ], +) +def test_coerce_bool_handles_common_inputs( + module: ModuleType, value: object, expected: object +) -> None: + """The coercion helper accepts boolean strings in various casings.""" + assert module._coerce_bool(value) is expected + + +def test_coerce_bool_rejects_unknown_input(module: ModuleType) -> None: + """Unexpected values surface a descriptive error.""" + with pytest.raises(ValueError, match="Cannot interpret 'maybe'"): + module._coerce_bool("maybe") + + def test_cli_dry_run_outputs_summary(module: ModuleType, tmp_path: Path) -> None: """It prints the planned gh commands instead of uploading during dry runs.""" dist = tmp_path / "dist" diff --git a/scripts/upload_release_assets.py b/scripts/upload_release_assets.py index 91110fdd..0844cc98 100755 --- a/scripts/upload_release_assets.py +++ b/scripts/upload_release_assets.py @@ -65,11 +65,29 @@ def _is_candidate(path: Path, bin_name: str) -> bool: return path.suffix in {".deb", ".rpm", ".pkg", ".msi"} -def _resolve_asset_name(path: Path) -> str: - suffix = path.suffix.lower() - if suffix in {".deb", ".rpm", ".pkg"}: - return path.name - return f"{path.parent.name}-{path.name}" +def _coerce_bool(value: object) -> bool: + """Return ``value`` as a strict boolean.""" + if isinstance(value, bool): + return value + if not isinstance(value, str): + message = f"Cannot interpret {value!r} as a boolean" + raise TypeError(message) + normalised = value.strip().lower() + if normalised in {"", "false", "0", "no", "off"}: + return False + if normalised in {"true", "1", "yes", "on"}: + return True + message = f"Cannot interpret {value!r} as a boolean" + raise ValueError(message) + + +def _resolve_asset_name(path: Path, *, dist_dir: Path) -> str: + """Return a unique asset name derived from ``path`` within ``dist_dir``.""" + relative_path = path.relative_to(dist_dir) + if relative_path.parent == Path(): + return relative_path.name + prefix = relative_path.parent.as_posix().replace("/", "__") + return f"{prefix}-{relative_path.name}" def _iter_candidate_paths(dist_dir: Path, bin_name: str) -> cabc.Iterator[Path]: @@ -126,7 +144,7 @@ def discover_assets(dist_dir: Path, *, bin_name: str) -> list[ReleaseAsset]: for path in _iter_candidate_paths(dist_dir, bin_name): size = _require_non_empty(path) - asset_name = _resolve_asset_name(path) + asset_name = _resolve_asset_name(path, dist_dir=dist_dir) _register_asset(asset_name, path, seen) assets.append(ReleaseAsset(path=path, asset_name=asset_name, size=size)) @@ -236,14 +254,14 @@ def cli( release_tag: typ.Annotated[str, Parameter(required=True)], bin_name: typ.Annotated[str, Parameter(required=True)], dist_dir: Path = Path("dist"), - dry_run: bool = False, + dry_run: bool | str = False, ) -> int: """Cyclopts-bound CLI entry point.""" return main( release_tag=release_tag, bin_name=bin_name, dist_dir=dist_dir, - dry_run=dry_run, + dry_run=_coerce_bool(dry_run), ) From ab39c6a88ea8ec3bfdf1264cb7c59d7c46328f4f Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 5 Oct 2025 15:52:54 +0100 Subject: [PATCH 21/21] Clarify uploader docstrings --- scripts/upload_release_assets.py | 41 +++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/scripts/upload_release_assets.py b/scripts/upload_release_assets.py index 0844cc98..d69330b0 100755 --- a/scripts/upload_release_assets.py +++ b/scripts/upload_release_assets.py @@ -119,9 +119,9 @@ def discover_assets(dist_dir: Path, *, bin_name: str) -> list[ReleaseAsset]: Parameters ---------- - dist_dir: + dist_dir : Path Root directory that contains the staged artefacts. - bin_name: + bin_name : str Binary name used to match platform-specific artefacts. Returns @@ -134,6 +134,11 @@ def discover_assets(dist_dir: Path, *, bin_name: str) -> list[ReleaseAsset]: AssetError If no artefacts are found, an artefact is empty, or multiple files would upload with the same asset name. + + Examples + -------- + >>> discover_assets(Path("dist"), bin_name="netsuke") # doctest: +SKIP + [ReleaseAsset(path=PosixPath('dist/netsuke'), ...)] """ if not dist_dir.exists(): message = f"Artefact directory {dist_dir} does not exist" @@ -171,11 +176,11 @@ def upload_assets( Parameters ---------- - release_tag: + release_tag : str Git tag identifying the release that should receive the artefacts. - assets: + assets : Iterable[ReleaseAsset] Iterable of artefacts to publish. - dry_run: + dry_run : bool When ``True``, print the planned ``gh`` invocations without executing them. @@ -185,6 +190,14 @@ def upload_assets( If ``gh`` returns a non-zero status while uploading. CommandNotFound If the ``gh`` executable is not available in ``PATH``. + + Examples + -------- + >>> upload_assets( # doctest: +SKIP + ... release_tag="v1.2.3", + ... assets=[ReleaseAsset(Path("dist/netsuke"), "netsuke", 1024)], + ... dry_run=True, + ... ) """ gh_cmd: BoundCommand | None = None for asset in assets: @@ -214,13 +227,13 @@ def main( Parameters ---------- - release_tag: + release_tag : str Git tag identifying the release to publish to. - bin_name: + bin_name : str Binary name used to derive artefact names during discovery. - dist_dir: + dist_dir : Path Directory containing staged artefacts. - dry_run: + dry_run : bool When ``True``, validate artefacts and print the upload plan without uploading. @@ -229,6 +242,16 @@ def main( int Exit code: ``0`` on success, ``1`` when artefact discovery or upload fails. + + Examples + -------- + >>> main( # doctest: +SKIP + ... release_tag="v1.2.3", + ... bin_name="netsuke", + ... dist_dir=Path("dist"), + ... dry_run=True, + ... ) + 0 """ try: assets = discover_assets(dist_dir, bin_name=bin_name)