From 0325fc0ef277800674e7e788e14922d1376cadd9 Mon Sep 17 00:00:00 2001 From: Ramesh Padmanabhaiah Date: Sun, 24 May 2026 19:41:12 -0700 Subject: [PATCH] Tighten artifact setup behavior --- cli/python/base_setup/engine.py | 39 ++++++++++--- cli/python/base_setup/tests/test_engine.py | 68 +++++++++++++++++++++- 2 files changed, 97 insertions(+), 10 deletions(-) diff --git a/cli/python/base_setup/engine.py b/cli/python/base_setup/engine.py index 75b7e0d..bfaa0bb 100644 --- a/cli/python/base_setup/engine.py +++ b/cli/python/base_setup/engine.py @@ -83,7 +83,13 @@ def reconcile_manifest( artifacts = merge_artifacts(default_manifest.artifacts, manifest.artifacts) definitions = resolve_artifact_definitions(artifacts) if not manifest.artifacts: - ctx.log.info("Project '%s' declares no artifacts.", manifest.project_name) + if artifacts: + ctx.log.info( + "Project '%s' declares no artifacts; installing Base default artifacts only.", + manifest.project_name, + ) + else: + ctx.log.info("Project '%s' has no artifacts to install.", manifest.project_name) for artifact, definition in zip(artifacts, definitions, strict=True): reconcile_artifact(ctx, definition, artifact.version, dry_run=dry_run) @@ -149,6 +155,13 @@ def reconcile_homebrew_artifact( version: str, dry_run: bool, ) -> None: + if version != "latest": + raise ArtifactError( + "Homebrew artifact " + f"'{definition.name}' specifies version '{version}', but Base only supports " + "Homebrew artifact version 'latest' right now." + ) + command = ["brew", "install", definition.package] if dry_run: dry_run_command(ctx, command) @@ -171,7 +184,7 @@ def reconcile_homebrew_artifact( definition.package, version, ) - run_command(command) + run_command(ctx, command) def reconcile_python_artifact( @@ -181,7 +194,7 @@ def reconcile_python_artifact( dry_run: bool, ) -> None: project = os.environ.get("BASE_PROJECT", "base") - venv_dir = Path.home() / ".base.d" / project / ".venv" + venv_dir = project_venv_dir(project) python_bin = venv_dir / "bin" / "python" requirement = f"{definition.package}=={version}" if version != "latest" else definition.package @@ -200,7 +213,14 @@ def reconcile_python_artifact( venv.create(venv_dir, with_pip=True) ctx.log.info("Installing Python artifact '%s' into project virtual environment.", definition.name) - run_command([str(python_bin), "-m", "pip", "install", requirement]) + run_command(ctx, [str(python_bin), "-m", "pip", "install", requirement]) + + +def project_venv_dir(project: str) -> Path: + override = os.environ.get("BASE_PROJECT_VENV_DIR") + if override: + return Path(override).expanduser() + return Path.home() / ".base.d" / project / ".venv" def python_artifact_installed(python_bin: Path, package: str, version: str) -> bool: @@ -229,10 +249,15 @@ def run_check(command: list[str]) -> bool: return subprocess.run(command, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False).returncode == 0 -def run_command(command: list[str]) -> None: - completed = subprocess.run(command, check=False) +def run_command(ctx: base_cli.Context, command: list[str]) -> None: + completed = subprocess.run(command, stderr=subprocess.PIPE, text=True, check=False) if completed.returncode: - raise ArtifactError(f"Command failed with exit {completed.returncode}: {format_command(command)}") + stderr = (completed.stderr or "").strip() + message = f"Command failed with exit {completed.returncode}: {format_command(command)}" + if stderr: + ctx.log.error("Command stderr: %s", stderr) + message = f"{message}\n{stderr}" + raise ArtifactError(message) def dry_run_command(ctx: base_cli.Context, command: list[str]) -> None: diff --git a/cli/python/base_setup/tests/test_engine.py b/cli/python/base_setup/tests/test_engine.py index 7e20758..abfc76e 100644 --- a/cli/python/base_setup/tests/test_engine.py +++ b/cli/python/base_setup/tests/test_engine.py @@ -9,6 +9,7 @@ from pathlib import Path from unittest import mock +from base_setup import engine from base_setup.engine import ArtifactError, main, merge_artifacts from base_setup.manifest import ArtifactRequest from base_setup.manifest import read_manifest @@ -25,6 +26,12 @@ def run_engine(args: list[str]) -> tuple[int, str, str]: return status, stdout.getvalue(), stderr.getvalue() +def fake_context() -> mock.Mock: + ctx = mock.Mock() + ctx.log = mock.Mock() + return ctx + + class ManifestTests(unittest.TestCase): def test_merge_artifacts_keeps_defaults_and_manifest_artifacts(self) -> None: merged = merge_artifacts( @@ -126,7 +133,7 @@ def test_known_homebrew_artifact_dry_run_does_not_require_brew(self) -> None: "artifacts:", " - type: tool", " name: terraform", - " version: \"1.8.5\"", + " version: latest", ] ), encoding="utf-8", @@ -137,6 +144,61 @@ def test_known_homebrew_artifact_dry_run_does_not_require_brew(self) -> None: self.assertEqual(status, 0) self.assertIn("[DRY-RUN] Would run: brew install terraform", stderr) + def test_homebrew_artifact_rejects_non_latest_version(self) -> None: + definition = get_artifact_definition("tool", "terraform") + self.assertIsNotNone(definition) + + with self.assertRaisesRegex(ArtifactError, "only supports Homebrew artifact version 'latest'"): + engine.reconcile_homebrew_artifact(fake_context(), definition, "1.8.5", dry_run=True) + + def test_homebrew_artifact_latest_invokes_brew_install(self) -> None: + definition = get_artifact_definition("tool", "terraform") + self.assertIsNotNone(definition) + ctx = fake_context() + + with mock.patch("base_setup.engine.command_exists", return_value=True), mock.patch( + "base_setup.engine.run_check", + return_value=False, + ), mock.patch("base_setup.engine.run_command") as run_command: + engine.reconcile_homebrew_artifact(ctx, definition, "latest", dry_run=False) + + run_command.assert_called_once_with(ctx, ["brew", "install", "terraform"]) + + def test_python_artifact_honors_project_venv_dir_override(self) -> None: + definition = get_artifact_definition("python-package", "requests") + self.assertIsNotNone(definition) + ctx = fake_context() + + with tempfile.TemporaryDirectory() as tmpdir: + venv_dir = Path(tmpdir) / "custom-venv" + with mock.patch.dict( + os.environ, + {"BASE_PROJECT": "demo", "BASE_PROJECT_VENV_DIR": str(venv_dir)}, + ), mock.patch("base_setup.engine.python_artifact_installed", return_value=False): + engine.reconcile_python_artifact(ctx, definition, "latest", dry_run=True) + + info_messages = [call.args[0] % call.args[1:] for call in ctx.log.info.call_args_list] + self.assertIn( + f"[DRY-RUN] Would create project virtual environment at '{venv_dir}'.", + info_messages, + ) + self.assertIn( + f"[DRY-RUN] Would run: {venv_dir}/bin/python -m pip install requests", + info_messages, + ) + + def test_run_command_includes_stderr_on_failure(self) -> None: + ctx = fake_context() + + with mock.patch( + "base_setup.engine.subprocess.run", + return_value=mock.Mock(returncode=17, stderr="installer exploded\n"), + ): + with self.assertRaisesRegex(ArtifactError, "installer exploded"): + engine.run_command(ctx, ["installer", "--bad"]) + + ctx.log.error.assert_called_once_with("Command stderr: %s", "installer exploded") + @unittest.skipUnless(importlib.util.find_spec("click"), "Click is not installed") def test_project_argument_validates_manifest_project_name(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: @@ -179,7 +241,7 @@ def test_empty_artifact_list_is_supported(self) -> None: self.assertEqual(manifest.artifacts, ()) @unittest.skipUnless(importlib.util.find_spec("click"), "Click is not installed") - def test_empty_artifact_list_logs_that_no_artifacts_are_declared(self) -> None: + def test_empty_artifact_list_logs_that_base_defaults_are_used(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: manifest_path = Path(tmpdir) / "base_manifest.yaml" manifest_path.write_text( @@ -197,7 +259,7 @@ def test_empty_artifact_list_logs_that_no_artifacts_are_declared(self) -> None: status, _stdout, stderr = run_engine(["--dry-run", "--manifest", str(manifest_path)]) self.assertEqual(status, 0) - self.assertIn("Project 'demo' declares no artifacts.", stderr) + self.assertIn("Project 'demo' declares no artifacts; installing Base default artifacts only.", stderr) if __name__ == "__main__":