From a6022c5d70b17a975e90e6d0e27a3c4a927b0624 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 10 Jun 2026 01:11:36 +0200 Subject: [PATCH] Remove dead lockfile.refresh_lockfile; fix module docstring refresh_lockfile ran cargo generate-lockfile but was called only by its own unit tests: the bump path regenerates lockfiles via bump_lockfiles.regenerate_lockfiles (cargo update --workspace). The lockfile.py module docstring also described a call graph that does not exist, claiming lading bump calls discover_tracked_lockfiles followed by refresh_lockfile. Delete refresh_lockfile, its LockfileRefreshError class, and their tests. Correct the module docstring to describe the real call graph and explain why the two cargo strategies differ: bump refreshes pinned versions in place after manifest rewrites, while publish only probes freshness read-only under --locked. Update the developers' guide and the exceptions catalogue to match. Closes #99 --- docs/developers-guide.md | 25 +++---- lading/commands/lockfile.py | 75 ++++--------------- lading/exceptions.py | 2 +- .../test_command_failure_messages.ambr | 3 - tests/unit/test_command_failure_messages.py | 13 +--- tests/unit/test_lockfile.py | 65 ---------------- 6 files changed, 25 insertions(+), 158 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index a7bf9c33..b233cb04 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -225,16 +225,12 @@ this helper for consistent path handling, including `lading.cli`, Private helpers `_handle_git_ls_files_failure` and `_lockfiles_with_manifests` perform the error-handling and path-filtering passes respectively. -`refresh_lockfile(manifest_path, runner)` runs -`cargo generate-lockfile --manifest-path` for the supplied manifest and raises -`LockfileRefreshError` on non-zero exit. `_refresh_lockfiles` in `bump.py` -calls it after manifest rewrites. The refresh loop is intentionally not -transactional: if a later lockfile refresh fails, previously rewritten -manifests and refreshed lockfiles remain on disk, and the operator should fix -the Cargo error, then run -`cargo generate-lockfile --manifest-path /Cargo.toml` for each affected -crate manifest. Rerunning `lading bump` will not refresh lockfiles once the -manifests are already rewritten. +Lockfile regeneration after `lading bump` is owned by +`lading.commands.bump_lockfiles.regenerate_lockfiles`, which runs +`cargo update --workspace` per configured manifest. The two cargo strategies +differ deliberately: bump refreshes existing pinned versions in place after +manifest rewrites, while publish only probes freshness read-only via +`cargo metadata --locked` and never regenerates. `validate_lockfile_freshness(manifest_path, runner)` runs `cargo metadata --locked --manifest-path ... --format-version=1`. It returns a @@ -243,8 +239,8 @@ Cargo says need updating under `--locked`, and unrelated Cargo failures. `_validate_lockfile_freshness` in `publish_preflight.py` calls it before the cargo check/test pre-flight. -`LockfileDiscoveryError` and `LockfileRefreshError` inherit `LadingError`; -their messages include git or Cargo details respectively. +`LockfileDiscoveryError` inherits `LadingError`; its messages include the git +failure detail. ### `load_cargo_metadata` @@ -349,7 +345,6 @@ area so existing handling remains precise: | `CargoMetadataError` | `lading.workspace.metadata` | Base for cargo metadata execution and parsing failures. | | `CommandSpawnError` | `lading.runtime.subprocess_runner` | Raised when the subprocess runner cannot spawn an external command. | | `LockfileDiscoveryError` | `lading.commands.lockfile` | Raised when git cannot list tracked lockfiles. | -| `LockfileRefreshError` | `lading.commands.lockfile` | Raised when `cargo generate-lockfile` fails. | | `LockfileRegenerationError` | `lading.commands.bump_lockfiles` | Raised when configured bump lockfile manifests are invalid or `cargo update --workspace` fails. | | `PublishPlanError` | `lading.commands.publish_plan` | Raised when a publish plan cannot be constructed. | | `ReadmeTranspositionError` | `lading.commands.bump_readme` | Raised when the workspace README cannot be transposed into a crate during `lading bump`. | @@ -380,7 +375,7 @@ Usage guidance: - CLI and integration boundaries may catch `LadingError` to render expected lading failures as user-facing diagnostics. - Feature code should catch the narrowest local exception it can handle, such - as `PublishPreflightError` or `LockfileRefreshError`, and let unrelated + as `PublishPreflightError` or `LockfileRegenerationError`, and let unrelated `LadingError` subclasses propagate. - Tests should assert the specific exception type for the behaviour under test, then use `LadingError` only when verifying common boundary handling. @@ -637,8 +632,6 @@ Defined metrics: | -------------------------------- | ----------------------------- | --------------------------------------------------------------------------------------------------------------------- | | `publish.index_lookup_downgrade` | `subcommand`, `missing_crate` | `_handle_index_missing_version` downgrades a crates.io index-lookup failure to a warning (in-plan, override enabled). | | `lockfile.discovered` | (none) | Incremented by the number of tracked lockfiles each `discover_tracked_lockfiles` call returns. | -| `lockfile.refresh` | `outcome` | One increment per `refresh_lockfile` invocation; `outcome` is `success` or `failure`. | -| `lockfile.refresh.duration` | (none) | Duration observation around each `cargo generate-lockfile` invocation. | | `lockfile.validate` | `outcome` | One increment per `validate_lockfile_freshness` call; `outcome` is `fresh`, `stale`, or `failed`. | | `lockfile.validate.duration` | (none) | Duration observation around each `cargo metadata --locked` probe. | diff --git a/lading/commands/lockfile.py b/lading/commands/lockfile.py index 734d0513..9a2e5c0d 100644 --- a/lading/commands/lockfile.py +++ b/lading/commands/lockfile.py @@ -1,27 +1,29 @@ -"""Cargo lockfile discovery, refresh, and freshness validation helpers. +"""Cargo lockfile discovery and freshness validation helpers. This module centralises the Cargo lockfile operations shared by release -workflows. It discovers lockfiles that belong to the source workspace, -regenerates them after manifest rewrites, and validates that Cargo can read -them under ``--locked`` before expensive publish pre-flight commands run. +workflows. It discovers lockfiles that belong to the source workspace and +validates that Cargo can read them under ``--locked`` before expensive +publish pre-flight commands run. Discovery is intentionally conservative. :func:`discover_tracked_lockfiles` queries the git index for tracked ``Cargo.lock`` files, then narrows the result to paths that are not under a ``target`` directory and have an adjacent ``Cargo.toml`` manifest. -``lading bump`` calls :func:`discover_tracked_lockfiles` followed by -:func:`refresh_lockfile` after it updates manifest versions. ``lading publish`` -uses :func:`discover_tracked_lockfiles` and -:func:`validate_lockfile_freshness` before the cargo check/test pre-flight, so -stale lockfiles fail early with an actionable repair command. +Call graph: ``lading publish`` uses :func:`discover_tracked_lockfiles` and +:func:`validate_lockfile_freshness` before the cargo check/test pre-flight, +so stale lockfiles fail early with an actionable repair command. +``lading bump`` regenerates lockfiles via +:func:`lading.commands.bump_lockfiles.regenerate_lockfiles` instead, which +runs ``cargo update --workspace``: bump wants existing pinned versions +refreshed in place after manifest rewrites, whereas validation here uses +``cargo metadata --locked`` purely as a read-only freshness probe. -Typical local or CI usage follows the same sequence: +Typical publish-side usage: ```python lockfiles = discover_tracked_lockfiles(workspace_root, runner) for lockfile_path in lockfiles: - refresh_lockfile(lockfile_path.parent / "Cargo.toml", runner) validate_lockfile_freshness(lockfile_path.parent / "Cargo.toml", runner) ``` """ @@ -37,7 +39,7 @@ from lading.exceptions import LadingError from lading.utils import metrics -from lading.utils.process import append_detail, command_detail, with_detail +from lading.utils.process import append_detail, command_detail if typ.TYPE_CHECKING: from lading.runtime import CommandRunner @@ -47,16 +49,10 @@ # Metric names (issue #91); documented in docs/developers-guide.md. DISCOVERED_LOCKFILES_METRIC = "lockfile.discovered" -REFRESH_METRIC = "lockfile.refresh" -REFRESH_DURATION_METRIC = "lockfile.refresh.duration" VALIDATE_METRIC = "lockfile.validate" VALIDATE_DURATION_METRIC = "lockfile.validate.duration" -class LockfileRefreshError(LadingError): - """Raised when Cargo cannot regenerate a lockfile.""" - - class LockfileDiscoveryError(LadingError): """Raised when git cannot list tracked lockfiles.""" @@ -174,49 +170,6 @@ def discover_tracked_lockfiles( return lockfiles -def refresh_lockfile( - manifest_path: Path, - runner: CommandRunner, -) -> Path: - """Regenerate the lockfile for ``manifest_path`` and return its path. - - Parameters - ---------- - manifest_path - Path to the ``Cargo.toml`` manifest to generate a lockfile for. - runner - Callable used to run external commands. It receives a command sequence - and returns ``(exit_code, stdout, stderr)``. - - Returns - ------- - Path - Path to the generated ``Cargo.lock`` in ``manifest_path.parent``. - - Raises - ------ - LockfileRefreshError - Raised when ``cargo generate-lockfile`` returns a non-zero exit code. - The error message includes Cargo's stderr, or stdout when stderr is - empty, so callers can report why regeneration failed. - """ - lockfile_path = manifest_path.parent / "Cargo.lock" - LOGGER.info("Refreshing %s", lockfile_path) - started_at = time.perf_counter() - exit_code, stdout, stderr = runner( - ("cargo", "generate-lockfile", "--manifest-path", str(manifest_path)), - cwd=manifest_path.parent, - ) - metrics.observe_duration(REFRESH_DURATION_METRIC, time.perf_counter() - started_at) - if exit_code != 0: - metrics.increment_counter(REFRESH_METRIC, outcome="failure") - message = with_detail(f"Failed to refresh {lockfile_path}", stdout, stderr) - raise LockfileRefreshError(message) - metrics.increment_counter(REFRESH_METRIC, outcome="success") - LOGGER.info("Refreshed %s", lockfile_path) - return lockfile_path - - def validate_lockfile_freshness( manifest_path: Path, runner: CommandRunner, diff --git a/lading/exceptions.py b/lading/exceptions.py index 0ab1aad3..09d98f9c 100644 --- a/lading/exceptions.py +++ b/lading/exceptions.py @@ -8,7 +8,7 @@ Examples include `ConfigurationError`, `PublishPreflightError`, `CargoMetadataError`, `WorkspaceModelError`, `LockfileDiscoveryError`, and -`LockfileRefreshError`. Feature-specific subclasses should continue to inherit +`LockfileRegenerationError`. Feature-specific subclasses should continue to inherit from their local root exception, preserving precise handling within each component while keeping a consistent package-wide exception hierarchy. """ diff --git a/tests/unit/__snapshots__/test_command_failure_messages.ambr b/tests/unit/__snapshots__/test_command_failure_messages.ambr index b962eaf1..c8e9bdf3 100644 --- a/tests/unit/__snapshots__/test_command_failure_messages.ambr +++ b/tests/unit/__snapshots__/test_command_failure_messages.ambr @@ -8,9 +8,6 @@ # name: test_cargo_metadata_invocation_message[stdout_fallback] 'out' # --- -# name: test_lockfile_refresh_failure_message - 'Failed to refresh /ws/crates/alpha/Cargo.lock: error: registry offline' -# --- # name: test_lockfile_regeneration_failure_message 'Cargo lockfile regeneration failed for /ws/Cargo.toml with exit code 101: error: dependency conflict' # --- diff --git a/tests/unit/test_command_failure_messages.py b/tests/unit/test_command_failure_messages.py index 2089960e..91aa6a0c 100644 --- a/tests/unit/test_command_failure_messages.py +++ b/tests/unit/test_command_failure_messages.py @@ -24,7 +24,7 @@ import pytest -from lading.commands import bump_lockfiles, lockfile +from lading.commands import bump_lockfiles from lading.workspace import metadata if typ.TYPE_CHECKING: @@ -51,17 +51,6 @@ def runner( return runner -def test_lockfile_refresh_failure_message(snapshot: SnapshotAssertion) -> None: - """``refresh_lockfile`` renders the canonical detail suffix.""" - manifest = Path("/ws/crates/alpha/Cargo.toml") - with pytest.raises(lockfile.LockfileRefreshError) as excinfo: - lockfile.refresh_lockfile( - manifest, _failing_runner("", "error: registry offline\n") - ) - - assert snapshot == str(excinfo.value) - - def test_lockfile_regeneration_failure_message(snapshot: SnapshotAssertion) -> None: """``regenerate_lockfiles`` renders the canonical detail suffix.""" with pytest.raises(bump_lockfiles.LockfileRegenerationError) as excinfo: diff --git a/tests/unit/test_lockfile.py b/tests/unit/test_lockfile.py index 49ae4199..ec2cfc77 100644 --- a/tests/unit/test_lockfile.py +++ b/tests/unit/test_lockfile.py @@ -175,49 +175,6 @@ def runner( lockfile.discover_tracked_lockfiles(tmp_path, runner) -def test_refresh_lockfile_returns_lockfile_path(tmp_path: Path) -> None: - """Successful lockfile refresh returns the expected Cargo.lock path.""" - manifest = tmp_path / "Cargo.toml" - - def runner( - command: cabc.Sequence[str], - *, - cwd: Path | None = None, - env: cabc.Mapping[str, str] | None = None, - ) -> tuple[int, str, str]: - assert command == ( - "cargo", - "generate-lockfile", - "--manifest-path", - str(manifest), - ) - assert cwd == manifest.parent - return 0, "", "" - - expected = tmp_path / "Cargo.lock" - result = lockfile.refresh_lockfile(manifest, runner) - assert result == expected, ( - "refresh helper returned unexpected lockfile path; " - f"expected {expected!r}, got {result!r}" - ) - - -def test_refresh_lockfile_raises_on_failure(tmp_path: Path) -> None: - """Refresh failures include cargo stderr in the raised error.""" - manifest = tmp_path / "Cargo.toml" - - def runner( - command: cabc.Sequence[str], - *, - cwd: Path | None = None, - env: cabc.Mapping[str, str] | None = None, - ) -> tuple[int, str, str]: - return 101, "", "failed to resolve" - - with pytest.raises(lockfile.LockfileRefreshError, match="failed to resolve"): - lockfile.refresh_lockfile(manifest, runner) - - def _validate_lockfile_freshness_for_result( tmp_path: Path, exit_code: int, stderr: str ) -> lockfile.LockfileFreshness: @@ -380,28 +337,6 @@ def test_discovery_records_lockfile_count(tmp_path: Path) -> None: assert metrics.counter_value(lockfile.DISCOVERED_LOCKFILES_METRIC) == 1 -@pytest.mark.usefixtures("_metrics_registry") -def test_refresh_records_success_outcome_and_duration(tmp_path: Path) -> None: - """A successful refresh counts a success and observes a duration.""" - lockfile.refresh_lockfile(tmp_path / "Cargo.toml", _static_runner(0, "", "")) - - assert metrics.counter_value(lockfile.REFRESH_METRIC, outcome="success") == 1 - assert metrics.counter_value(lockfile.REFRESH_METRIC, outcome="failure") == 0 - assert metrics.duration_stats(lockfile.REFRESH_DURATION_METRIC).count == 1 - - -@pytest.mark.usefixtures("_metrics_registry") -def test_refresh_records_failure_outcome(tmp_path: Path) -> None: - """A failed refresh counts a failure and still observes a duration.""" - with pytest.raises(lockfile.LockfileRefreshError): - lockfile.refresh_lockfile( - tmp_path / "Cargo.toml", _static_runner(101, "", "boom") - ) - - assert metrics.counter_value(lockfile.REFRESH_METRIC, outcome="failure") == 1 - assert metrics.duration_stats(lockfile.REFRESH_DURATION_METRIC).count == 1 - - @pytest.mark.usefixtures("_metrics_registry") @pytest.mark.parametrize( ("exit_code", "stderr", "expected_state"),