fix(install): install a repository once, by the first hook that supports it
The pipeline used to skip a second installer only when it advertised capabilities the first had already provided, so a hook with a different capability still ran on the very same project. file-dedupe was installed through pip and then again through `make install`, whose first line upgrades pip; PEP 668 refuses that, and the run went red on a repository that was already installed correctly. One hook now installs a repository and the pipeline stops there. The order of INSTALLERS is the preference: `make install` leads, because it is the hook a repository writes for itself, while the others infer an installation from a manifest. A hook that fails installed nothing, so the rule is one *successful* installation rather than one attempt: _run_installer returns a bool instead of re-raising, and the caller moves on to the next hook. To keep that fallback from turning a broken repository into a silent pass, a repository whose hooks all fail now exits with the list of what was tried. The capability bookkeeping the old skip rule needed is gone from the pipeline; discover_capabilities remains on BaseInstaller and is now unused there. Verified: 14 targeted unit and integration tests, and a full `pkgmgr update --all` sweep in the arch image with 0 installation failures. Delta measured at the consumer: repositories left without a CLI entry point, 16 before and 16 after, diff identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -38,13 +38,14 @@ from pkgmgr.core.repository.verify import verify_repository
|
|||||||
Repository = dict[str, Any]
|
Repository = dict[str, Any]
|
||||||
|
|
||||||
INSTALLERS = [
|
INSTALLERS = [
|
||||||
|
MakefileInstaller(),
|
||||||
ArchPkgbuildInstaller(),
|
ArchPkgbuildInstaller(),
|
||||||
DebianControlInstaller(),
|
DebianControlInstaller(),
|
||||||
RpmSpecInstaller(),
|
RpmSpecInstaller(),
|
||||||
NixFlakeInstaller(),
|
NixFlakeInstaller(),
|
||||||
PythonInstaller(),
|
PythonInstaller(),
|
||||||
MakefileInstaller(),
|
|
||||||
]
|
]
|
||||||
|
"""Preference order: the pipeline runs the first supported hook and stops."""
|
||||||
|
|
||||||
|
|
||||||
def _ensure_repo_dir(
|
def _ensure_repo_dir(
|
||||||
|
|||||||
@@ -81,13 +81,15 @@ class InstallationPipeline:
|
|||||||
else:
|
else:
|
||||||
repo.pop("command", None)
|
repo.pop("command", None)
|
||||||
|
|
||||||
provided_capabilities: set[str] = set()
|
attempted: list[str] = []
|
||||||
|
|
||||||
for installer in self._installers:
|
for installer in self._installers:
|
||||||
layer_name = getattr(installer, "layer", None)
|
layer_name = getattr(installer, "layer", None)
|
||||||
|
|
||||||
if layer_name is None:
|
if layer_name is None:
|
||||||
self._run_installer(installer, ctx, identifier, repo_dir, quiet)
|
if self._run_installer(installer, ctx, identifier, repo_dir, quiet):
|
||||||
|
return
|
||||||
|
attempted.append(installer.__class__.__name__)
|
||||||
continue
|
continue
|
||||||
|
|
||||||
try:
|
try:
|
||||||
@@ -120,15 +122,6 @@ class InstallationPipeline:
|
|||||||
if not installer.supports(ctx):
|
if not installer.supports(ctx):
|
||||||
continue
|
continue
|
||||||
|
|
||||||
caps = installer.discover_capabilities(ctx)
|
|
||||||
if caps and caps.issubset(provided_capabilities):
|
|
||||||
if not quiet:
|
|
||||||
print(
|
|
||||||
f"Skipping installer {installer.__class__.__name__} "
|
|
||||||
f"for {identifier} – capabilities {caps} already provided."
|
|
||||||
)
|
|
||||||
continue
|
|
||||||
|
|
||||||
if not quiet:
|
if not quiet:
|
||||||
if (
|
if (
|
||||||
ctx.force_update
|
ctx.force_update
|
||||||
@@ -142,13 +135,17 @@ class InstallationPipeline:
|
|||||||
else:
|
else:
|
||||||
print(
|
print(
|
||||||
f"[pkgmgr] Running installer {installer.__class__.__name__} "
|
f"[pkgmgr] Running installer {installer.__class__.__name__} "
|
||||||
f"for {identifier} in '{repo_dir}' "
|
f"for {identifier} in '{repo_dir}'..."
|
||||||
f"(new capabilities: {caps or set()})..."
|
|
||||||
)
|
)
|
||||||
|
|
||||||
self._run_installer(installer, ctx, identifier, repo_dir, quiet)
|
if not self._run_installer(installer, ctx, identifier, repo_dir, quiet):
|
||||||
|
attempted.append(installer.__class__.__name__)
|
||||||
provided_capabilities.update(caps)
|
if not quiet:
|
||||||
|
print(
|
||||||
|
f"[pkgmgr] {installer.__class__.__name__} installed nothing "
|
||||||
|
f"for {identifier}; falling back to the next hook."
|
||||||
|
)
|
||||||
|
continue
|
||||||
|
|
||||||
new_state = resolver.resolve()
|
new_state = resolver.resolve()
|
||||||
if new_state.command:
|
if new_state.command:
|
||||||
@@ -165,6 +162,13 @@ class InstallationPipeline:
|
|||||||
repo.pop("command", None)
|
repo.pop("command", None)
|
||||||
|
|
||||||
state = new_state
|
state = new_state
|
||||||
|
return
|
||||||
|
|
||||||
|
if attempted:
|
||||||
|
raise SystemExit(
|
||||||
|
f"every installation hook failed for {identifier}: "
|
||||||
|
f"{', '.join(attempted)}"
|
||||||
|
)
|
||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _run_installer(
|
def _run_installer(
|
||||||
@@ -173,9 +177,11 @@ class InstallationPipeline:
|
|||||||
identifier: str,
|
identifier: str,
|
||||||
repo_dir: str,
|
repo_dir: str,
|
||||||
quiet: bool,
|
quiet: bool,
|
||||||
) -> None:
|
) -> bool:
|
||||||
|
"""Run one hook. Returns whether it installed anything."""
|
||||||
try:
|
try:
|
||||||
installer.run(ctx)
|
installer.run(ctx)
|
||||||
|
return True
|
||||||
except SystemExit as exc:
|
except SystemExit as exc:
|
||||||
exit_code = exc.code if isinstance(exc.code, int) else str(exc.code)
|
exit_code = exc.code if isinstance(exc.code, int) else str(exc.code)
|
||||||
print(
|
print(
|
||||||
@@ -193,4 +199,4 @@ class InstallationPipeline:
|
|||||||
f" pkgmgr install {identifier} "
|
f" pkgmgr install {identifier} "
|
||||||
"--clone-mode shallow --no-verification"
|
"--clone-mode shallow --no-verification"
|
||||||
)
|
)
|
||||||
raise
|
return False
|
||||||
|
|||||||
@@ -1,9 +1,5 @@
|
|||||||
"""
|
"""
|
||||||
Integration tests for recursive capability resolution and installer shadowing.
|
Integration tests for installer selection across repository layouts.
|
||||||
|
|
||||||
These tests verify that, given different repository layouts (Makefile, pyproject,
|
|
||||||
flake.nix, PKGBUILD), only the expected installers are executed based on the
|
|
||||||
capabilities provided by higher layers.
|
|
||||||
|
|
||||||
Layer order (strongest → weakest):
|
Layer order (strongest → weakest):
|
||||||
|
|
||||||
@@ -129,10 +125,8 @@ class TestRecursiveCapabilitiesIntegration(unittest.TestCase):
|
|||||||
"With only a Makefile, the MakefileInstaller should run exactly once.",
|
"With only a Makefile, the MakefileInstaller should run exactly once.",
|
||||||
)
|
)
|
||||||
|
|
||||||
def test_python_and_makefile_both_run_when_caps_disjoint(self) -> None:
|
def test_only_the_first_supported_hook_runs(self) -> None:
|
||||||
"""
|
"""A repository with two usable hooks is installed by the first one."""
|
||||||
If Python and Makefile have disjoint capabilities, both installers run.
|
|
||||||
"""
|
|
||||||
repo_dir = self._new_repo()
|
repo_dir = self._new_repo()
|
||||||
|
|
||||||
# pyproject.toml without any explicit "make install" hint
|
# pyproject.toml without any explicit "make install" hint
|
||||||
@@ -153,9 +147,8 @@ class TestRecursiveCapabilitiesIntegration(unittest.TestCase):
|
|||||||
|
|
||||||
self.assertEqual(
|
self.assertEqual(
|
||||||
called,
|
called,
|
||||||
["python", "makefile"],
|
["python"],
|
||||||
"PythonInstaller and MakefileInstaller should both run when their "
|
"Only the first supported hook may install the repository.",
|
||||||
"capabilities are disjoint.",
|
|
||||||
)
|
)
|
||||||
|
|
||||||
def test_python_shadows_makefile_when_pyproject_mentions_make_install(self) -> None:
|
def test_python_shadows_makefile_when_pyproject_mentions_make_install(self) -> None:
|
||||||
|
|||||||
@@ -21,11 +21,13 @@ class DummyInstaller(BaseInstaller):
|
|||||||
layer: str | None = None,
|
layer: str | None = None,
|
||||||
supports_result: bool = True,
|
supports_result: bool = True,
|
||||||
capabilities: set[str] | None = None,
|
capabilities: set[str] | None = None,
|
||||||
|
fails: bool = False,
|
||||||
) -> None:
|
) -> None:
|
||||||
self._name = name
|
self._name = name
|
||||||
self.layer = layer # type: ignore[assignment]
|
self.layer = layer # type: ignore[assignment]
|
||||||
self._supports_result = supports_result
|
self._supports_result = supports_result
|
||||||
self._capabilities = capabilities or set()
|
self._capabilities = capabilities or set()
|
||||||
|
self._fails = fails
|
||||||
self.ran = False
|
self.ran = False
|
||||||
|
|
||||||
def supports(self, ctx: RepoContext) -> bool: # type: ignore[override]
|
def supports(self, ctx: RepoContext) -> bool: # type: ignore[override]
|
||||||
@@ -33,6 +35,8 @@ class DummyInstaller(BaseInstaller):
|
|||||||
|
|
||||||
def run(self, ctx: RepoContext) -> None: # type: ignore[override]
|
def run(self, ctx: RepoContext) -> None: # type: ignore[override]
|
||||||
self.ran = True
|
self.ran = True
|
||||||
|
if self._fails:
|
||||||
|
raise SystemExit(2)
|
||||||
|
|
||||||
def discover_capabilities(self, ctx: RepoContext) -> set[str]: # type: ignore[override]
|
def discover_capabilities(self, ctx: RepoContext) -> set[str]: # type: ignore[override]
|
||||||
return set(self._capabilities)
|
return set(self._capabilities)
|
||||||
@@ -117,15 +121,12 @@ class TestInstallationPipeline(unittest.TestCase):
|
|||||||
|
|
||||||
@patch("pkgmgr.actions.install.pipeline.create_ink")
|
@patch("pkgmgr.actions.install.pipeline.create_ink")
|
||||||
@patch("pkgmgr.actions.install.pipeline.resolve_command_for_repo")
|
@patch("pkgmgr.actions.install.pipeline.resolve_command_for_repo")
|
||||||
def test_capabilities_prevent_duplicate_installers(
|
def test_only_one_installation_hook_runs(
|
||||||
self,
|
self,
|
||||||
mock_resolve_command_for_repo: MagicMock,
|
mock_resolve_command_for_repo: MagicMock,
|
||||||
mock_create_ink: MagicMock,
|
mock_create_ink: MagicMock,
|
||||||
) -> None:
|
) -> None:
|
||||||
"""
|
"""A repository is installed once, by the first hook that supports it."""
|
||||||
If one installer has already provided a set of capabilities,
|
|
||||||
a second installer advertising the same capabilities should be skipped.
|
|
||||||
"""
|
|
||||||
mock_resolve_command_for_repo.return_value = None # no CLI initially
|
mock_resolve_command_for_repo.return_value = None # no CLI initially
|
||||||
|
|
||||||
ctx = _minimal_context()
|
ctx = _minimal_context()
|
||||||
@@ -148,9 +149,103 @@ class TestInstallationPipeline(unittest.TestCase):
|
|||||||
self.assertTrue(first.ran, "First installer should run.")
|
self.assertTrue(first.ran, "First installer should run.")
|
||||||
self.assertFalse(
|
self.assertFalse(
|
||||||
second.ran,
|
second.ran,
|
||||||
"Second installer with identical capabilities must be skipped.",
|
"A second installation hook must not run after the first one did.",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
@patch("pkgmgr.actions.install.pipeline.create_ink")
|
||||||
|
@patch("pkgmgr.actions.install.pipeline.resolve_command_for_repo")
|
||||||
|
def test_a_differing_second_hook_is_not_run_either(
|
||||||
|
self,
|
||||||
|
mock_resolve_command_for_repo: MagicMock,
|
||||||
|
mock_create_ink: MagicMock,
|
||||||
|
) -> None:
|
||||||
|
"""The rule is one hook, not one hook per capability."""
|
||||||
|
mock_resolve_command_for_repo.return_value = None
|
||||||
|
|
||||||
|
ctx = _minimal_context()
|
||||||
|
first = DummyInstaller(
|
||||||
|
"python-installer",
|
||||||
|
layer=CliLayer.PYTHON.value,
|
||||||
|
supports_result=True,
|
||||||
|
capabilities={"python-runtime"},
|
||||||
|
)
|
||||||
|
second = DummyInstaller(
|
||||||
|
"makefile-installer",
|
||||||
|
layer=CliLayer.MAKEFILE.value,
|
||||||
|
supports_result=True,
|
||||||
|
capabilities={"make-install"},
|
||||||
|
)
|
||||||
|
|
||||||
|
pipeline = InstallationPipeline([first, second])
|
||||||
|
pipeline.run(ctx)
|
||||||
|
|
||||||
|
self.assertTrue(first.ran, "The first supported hook should run.")
|
||||||
|
self.assertFalse(
|
||||||
|
second.ran,
|
||||||
|
"A hook advertising a different capability must not run either: "
|
||||||
|
"the project is already installed.",
|
||||||
|
)
|
||||||
|
|
||||||
|
@patch("pkgmgr.actions.install.pipeline.create_ink")
|
||||||
|
@patch("pkgmgr.actions.install.pipeline.resolve_command_for_repo")
|
||||||
|
def test_a_failed_hook_falls_back_to_the_next(
|
||||||
|
self,
|
||||||
|
mock_resolve_command_for_repo: MagicMock,
|
||||||
|
mock_create_ink: MagicMock,
|
||||||
|
) -> None:
|
||||||
|
"""A hook that fails installed nothing, so the next one gets its turn."""
|
||||||
|
mock_resolve_command_for_repo.return_value = None
|
||||||
|
|
||||||
|
ctx = _minimal_context()
|
||||||
|
broken = DummyInstaller(
|
||||||
|
"makefile-installer",
|
||||||
|
layer=CliLayer.MAKEFILE.value,
|
||||||
|
supports_result=True,
|
||||||
|
fails=True,
|
||||||
|
)
|
||||||
|
working = DummyInstaller(
|
||||||
|
"python-installer",
|
||||||
|
layer=CliLayer.PYTHON.value,
|
||||||
|
supports_result=True,
|
||||||
|
)
|
||||||
|
|
||||||
|
pipeline = InstallationPipeline([broken, working])
|
||||||
|
pipeline.run(ctx)
|
||||||
|
|
||||||
|
self.assertTrue(broken.ran, "The preferred hook should be tried first.")
|
||||||
|
self.assertTrue(
|
||||||
|
working.ran,
|
||||||
|
"The next hook must run when the preferred one installed nothing.",
|
||||||
|
)
|
||||||
|
|
||||||
|
@patch("pkgmgr.actions.install.pipeline.create_ink")
|
||||||
|
@patch("pkgmgr.actions.install.pipeline.resolve_command_for_repo")
|
||||||
|
def test_a_repository_whose_hooks_all_fail_is_reported(
|
||||||
|
self,
|
||||||
|
mock_resolve_command_for_repo: MagicMock,
|
||||||
|
mock_create_ink: MagicMock,
|
||||||
|
) -> None:
|
||||||
|
"""Falling back must not turn a broken repository into a silent pass."""
|
||||||
|
mock_resolve_command_for_repo.return_value = None
|
||||||
|
|
||||||
|
ctx = _minimal_context()
|
||||||
|
first = DummyInstaller(
|
||||||
|
"makefile-installer",
|
||||||
|
layer=CliLayer.MAKEFILE.value,
|
||||||
|
supports_result=True,
|
||||||
|
fails=True,
|
||||||
|
)
|
||||||
|
second = DummyInstaller(
|
||||||
|
"python-installer",
|
||||||
|
layer=CliLayer.PYTHON.value,
|
||||||
|
supports_result=True,
|
||||||
|
fails=True,
|
||||||
|
)
|
||||||
|
|
||||||
|
pipeline = InstallationPipeline([first, second])
|
||||||
|
with self.assertRaises(SystemExit):
|
||||||
|
pipeline.run(ctx)
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|||||||
Reference in New Issue
Block a user