diff --git a/lading/commands/bump_lockfiles.py b/lading/commands/bump_lockfiles.py index 70e07c8f..fcc095ad 100644 --- a/lading/commands/bump_lockfiles.py +++ b/lading/commands/bump_lockfiles.py @@ -9,6 +9,7 @@ import collections.abc as cabc import logging +import shlex import time from pathlib import Path @@ -60,37 +61,94 @@ def regenerate_lockfiles( ------ LockfileRegenerationError If any configured manifest path is invalid (outside the workspace or - not named ``Cargo.toml``), or if ``cargo update --workspace`` exits - non-zero for any manifest. + not named ``Cargo.toml``), or — after every manifest has been + attempted — if ``cargo update --workspace`` failed for one or more + manifests. The aggregated message lists each failed manifest with a + repair command. Notes ----- - **Partial-update semantics:** regeneration is not atomic. If ``cargo`` - fails for the *n*-th manifest, the first *n - 1* lockfiles have already - been updated on disk and will not be rolled back. Callers that require - atomic guarantees must implement their own rollback (see issue ``#84``). + **Partial-update semantics:** regeneration is not atomic and successful + updates are not rolled back when a later manifest fails. Every manifest + is attempted (issue #84), so a single cargo failure does not leave + unrelated lockfiles silently stale; the aggregated error tells the + operator exactly which lockfiles still need repair and how. """ command_runner = subprocess_runner if runner is None else runner manifests = _resolve_manifest_paths(workspace_root, lockfile_manifests) started_at = time.perf_counter() _LOGGER.info("Regenerating %d Cargo lockfile(s)", len(manifests)) + lockfiles, failures = _collect_lockfile_results( + workspace_root, manifests, command_runner + ) + _raise_if_failures(failures) + _LOGGER.info( + "Regenerated %d Cargo lockfile(s) in %.3fs", + len(lockfiles), + time.perf_counter() - started_at, + ) + return tuple(lockfiles) + + +def _collect_lockfile_results( + workspace_root: Path, + manifests: cabc.Sequence[Path], + runner: CommandRunner, +) -> tuple[list[Path], list[tuple[Path, LockfileRegenerationError]]]: + """Attempt every manifest and return successful lockfiles and failures.""" lockfiles: list[Path] = [] + failures: list[tuple[Path, LockfileRegenerationError]] = [] for manifest in manifests: manifest_started_at = time.perf_counter() _LOGGER.info("Regenerating Cargo lockfile for %s", manifest) - _run_workspace_lockfile_update(workspace_root, manifest, command_runner) + try: + _run_workspace_lockfile_update(workspace_root, manifest, runner) + except LockfileRegenerationError as exc: + # Keep going: attempting the remaining manifests gives the + # operator one aggregated repair list instead of a re-run per + # failure (issue #84). + _LOGGER.exception("Cargo lockfile regeneration failed") + failures.append((manifest, exc)) + continue _LOGGER.info( "Regenerated Cargo lockfile for %s in %.3fs", manifest, time.perf_counter() - manifest_started_at, ) lockfiles.append(manifest.parent / "Cargo.lock") - _LOGGER.info( - "Regenerated %d Cargo lockfile(s) in %.3fs", - len(lockfiles), - time.perf_counter() - started_at, + return lockfiles, failures + + +def _raise_if_failures( + failures: list[tuple[Path, LockfileRegenerationError]], +) -> None: + """Raise an aggregated error when one or more manifest updates failed.""" + if not failures: + return + # Chain from the first underlying failure so diagnostics (for + # example a missing cargo executable) survive the aggregation. + primary = failures[0][1] + cause = primary.__cause__ if primary.__cause__ is not None else primary + raise LockfileRegenerationError( + _build_aggregate_failure_message(failures) + ) from cause + + +def _build_aggregate_failure_message( + +) -> str: + """Return the aggregated operator-facing regeneration failure message.""" + header = ( + f"Cargo lockfile regeneration failed for {len(failures)} manifest(s). " + "Manifests already carry the new version, so the workspace is " + "inconsistent until each lockfile below is repaired:" ) - return tuple(lockfiles) + lines = [header] + for manifest, error in failures: + lines.append(f"- {error}") + quoted_manifest = shlex.quote(str(manifest)) + lines.append(f" cargo update --workspace --manifest-path {quoted_manifest}") + return "\n".join(lines) def _resolve_manifest_paths( diff --git a/tests/unit/__snapshots__/test_bump_lockfiles.ambr b/tests/unit/__snapshots__/test_bump_lockfiles.ambr new file mode 100644 index 00000000..3b9f4960 --- /dev/null +++ b/tests/unit/__snapshots__/test_bump_lockfiles.ambr @@ -0,0 +1,10 @@ +# serializer version: 1 +# name: test_regenerate_lockfiles_aggregates_multiple_failures + ''' + Cargo lockfile regeneration failed for 2 manifest(s). Manifests already carry the new version, so the workspace is inconsistent until each lockfile below is repaired: + - Cargo lockfile regeneration failed for /a/Cargo.toml with exit code 101: error: dependency conflict + cargo update --workspace --manifest-path /a/Cargo.toml + - Cargo lockfile regeneration failed for /b/Cargo.toml with exit code 101: error: dependency conflict + cargo update --workspace --manifest-path /b/Cargo.toml + ''' +# --- diff --git a/tests/unit/test_bump_lockfiles.py b/tests/unit/test_bump_lockfiles.py index 4345b043..9c253e09 100644 --- a/tests/unit/test_bump_lockfiles.py +++ b/tests/unit/test_bump_lockfiles.py @@ -6,13 +6,14 @@ import dataclasses as dc import pathlib import typing as typ +from pathlib import Path import pytest from lading.commands import bump_lockfiles if typ.TYPE_CHECKING: - from pathlib import Path + from syrupy.assertion import SnapshotAssertion @dc.dataclass(frozen=True, slots=True) @@ -267,3 +268,87 @@ def failing_runner( ) assert isinstance(exc_info.value.__cause__, OSError) + + +# --------------------------------------------------------------------------- +# Aggregated failure handling (issue #84) +# --------------------------------------------------------------------------- + + +def _selective_failure_runner( + failing_manifests: set[Path], +) -> tuple[cabc.Callable[..., tuple[int, str, str]], list[Path]]: + """Return a runner failing for ``failing_manifests`` and its call log.""" + attempted: list[Path] = [] + + def runner( + command: cabc.Sequence[str], + *, + cwd: Path | None = None, + ) -> tuple[int, str, str]: + del cwd + manifest = Path(command[-1]) + attempted.append(manifest) + if manifest in failing_manifests: + return 101, "", "error: dependency conflict" + return 0, "", "" + + return runner, attempted + + +def test_regenerate_lockfiles_attempts_all_manifests_after_failure( + tmp_path: Path, +) -> None: + """A mid-list cargo failure does not skip the remaining manifests.""" + for name in ("a", "b"): + (tmp_path / name).mkdir() + (tmp_path / name / "Cargo.toml").write_text("", encoding="utf-8") + (tmp_path / "Cargo.toml").write_text("", encoding="utf-8") + failing = (tmp_path / "a" / "Cargo.toml").resolve() + runner, attempted = _selective_failure_runner({failing}) + + with pytest.raises(bump_lockfiles.LockfileRegenerationError) as excinfo: + bump_lockfiles.regenerate_lockfiles( + tmp_path, + ("a/Cargo.toml", "b/Cargo.toml"), + runner=runner, + ) + + assert attempted == [ + (tmp_path / "Cargo.toml").resolve(), + failing, + (tmp_path / "b" / "Cargo.toml").resolve(), + ] + message = str(excinfo.value) + assert "failed for 1 manifest(s)" in message + assert str(failing) in message + assert f"cargo update --workspace --manifest-path {failing}" in message + assert str((tmp_path / "b" / "Cargo.toml").resolve()) not in message + + +def test_regenerate_lockfiles_aggregates_multiple_failures( + tmp_path: Path, snapshot: SnapshotAssertion +) -> None: + """Every failed manifest is listed once with its repair command.""" + for name in ("a", "b"): + (tmp_path / name).mkdir() + (tmp_path / name / "Cargo.toml").write_text("", encoding="utf-8") + (tmp_path / "Cargo.toml").write_text("", encoding="utf-8") + failing = { + (tmp_path / "a" / "Cargo.toml").resolve(), + (tmp_path / "b" / "Cargo.toml").resolve(), + } + runner, _ = _selective_failure_runner(failing) + + with pytest.raises(bump_lockfiles.LockfileRegenerationError) as excinfo: + bump_lockfiles.regenerate_lockfiles( + tmp_path, + ("a/Cargo.toml", "b/Cargo.toml"), + runner=runner, + ) + + message = str(excinfo.value) + assert "failed for 2 manifest(s)" in message + for manifest in failing: + assert message.count(f"--manifest-path {manifest}") == 1 + assert snapshot == message.replace(str(tmp_path), "")