Improve MakefileInstaller: only run 'make install' if an install target exists

- Added parsing of Makefile to detect real 'install' target
- Added safe fallback when Makefile cannot be read
- Added informative skip message when install target is missing
- Updated unit tests to cover: install present, install missing, file read
- Removed circular self-import causing test import failures

Conversation reference: https://chatgpt.com/share/6936a2bf-6aa4-800f-a1bc-7c8eac66bc18

pkgmgr/installers/makefile.py

@@ -2,13 +2,15 @@
 # -*- coding: utf-8 -*-
 
 """
-Installer that triggers `make install` if a Makefile is present.
+Installer that triggers `make install` if a Makefile is present and
+the Makefile actually defines an 'install' target.
 
 This is useful for repositories that expose a standard Makefile-based
 installation step.
 """
 
 import os
+import re
 
 from pkgmgr.context import RepoContext
 from pkgmgr.installers.base import BaseInstaller
@@ -16,7 +18,7 @@ from pkgmgr.run_command import run_command
 
 
 class MakefileInstaller(BaseInstaller):
-    """Run `make install` if a Makefile exists in the repository."""
+    """Run `make install` if a Makefile with an 'install' target exists."""
 
     # Logical layer name, used by capability matchers.
     layer = "makefile"
@@ -24,15 +26,68 @@ class MakefileInstaller(BaseInstaller):
     MAKEFILE_NAME = "Makefile"
 
     def supports(self, ctx: RepoContext) -> bool:
+        """Return True if a Makefile exists in the repository directory."""
         makefile_path = os.path.join(ctx.repo_dir, self.MAKEFILE_NAME)
         return os.path.exists(makefile_path)
 
+    def _has_install_target(self, makefile_path: str) -> bool:
+        """
+        Check whether the Makefile defines an 'install' target.
+
+        We treat the presence of a real install target as either:
+          - a line starting with 'install:' (optionally preceded by whitespace), or
+          - a .PHONY line that lists 'install' as one of the targets.
+        """
+        try:
+            with open(makefile_path, "r", encoding="utf-8", errors="ignore") as f:
+                content = f.read()
+        except OSError:
+            # If we cannot read the Makefile for some reason, assume no target.
+            return False
+
+        # install: ...
+        if re.search(r"^\s*install\s*:", content, flags=re.MULTILINE):
+            return True
+
+        # .PHONY: ... install ...
+        if re.search(r"^\s*\.PHONY\s*:\s*.*\binstall\b", content, flags=re.MULTILINE):
+            return True
+
+        return False
+
     def run(self, ctx: RepoContext) -> None:
         """
-        Execute `make install` in the repository directory.
+        Execute `make install` in the repository directory, but only if an
+        'install' target is actually defined in the Makefile.
 
         Any failure in `make install` is treated as a fatal error and will
         propagate as SystemExit from run_command().
         """
+        makefile_path = os.path.join(ctx.repo_dir, self.MAKEFILE_NAME)
+
+        if not os.path.exists(makefile_path):
+            # Should normally not happen if supports() was checked before,
+            # but keep this guard for robustness.
+            if not ctx.quiet:
+                print(
+                    f"[pkgmgr] Makefile '{makefile_path}' not found, "
+                    "skipping make install."
+                )
+            return
+
+        if not self._has_install_target(makefile_path):
+            if not ctx.quiet:
+                print(
+                    "[pkgmgr] Skipping Makefile install: no 'install' target "
+                    f"found in {makefile_path}."
+                )
+            return
+
+        if not ctx.quiet:
+            print(
+                f"[pkgmgr] Running 'make install' in {ctx.repo_dir} "
+                "(install target detected in Makefile)."
+            )
+
         cmd = "make install"
         run_command(cmd, cwd=ctx.repo_dir, preview=ctx.preview)

tests/unit/pkgmgr/installers/test_makefile_installer.py

@@ -2,7 +2,7 @@
 
 import os
 import unittest
-from unittest.mock import patch
+from unittest.mock import patch, mock_open
 
 from pkgmgr.context import RepoContext
 from pkgmgr.installers.makefile import MakefileInstaller
@@ -36,9 +36,25 @@ class TestMakefileInstaller(unittest.TestCase):
         self.assertFalse(self.installer.supports(self.ctx))
 
     @patch("pkgmgr.installers.makefile.run_command")
+    @patch(
+        "builtins.open",
+        new_callable=mock_open,
+        read_data="install:\n\t@echo 'installing'\n",
+    )
     @patch("os.path.exists", return_value=True)
-    def test_run_executes_make_install(self, mock_exists, mock_run_command):
+    def test_run_executes_make_install_when_target_present(
+        self, mock_exists, mock_file, mock_run_command
+    ):
         self.installer.run(self.ctx)
+
+        # Ensure we checked the Makefile and then called make install.
+        mock_file.assert_called_once_with(
+            os.path.join(self.ctx.repo_dir, "Makefile"),
+            "r",
+            encoding="utf-8",
+            errors="ignore",
+        )
+
         cmd = mock_run_command.call_args[0][0]
         self.assertEqual(cmd, "make install")
         self.assertEqual(
@@ -46,6 +62,27 @@ class TestMakefileInstaller(unittest.TestCase):
             self.ctx.repo_dir,
         )
 
+    @patch("pkgmgr.installers.makefile.run_command")
+    @patch(
+        "builtins.open",
+        new_callable=mock_open,
+        read_data="all:\n\t@echo 'nothing here'\n",
+    )
+    @patch("os.path.exists", return_value=True)
+    def test_run_skips_when_no_install_target(
+        self, mock_exists, mock_file, mock_run_command
+    ):
+        self.installer.run(self.ctx)
+
+        # We should have read the Makefile, but not called run_command().
+        mock_file.assert_called_once_with(
+            os.path.join(self.ctx.repo_dir, "Makefile"),
+            "r",
+            encoding="utf-8",
+            errors="ignore",
+        )
+        mock_run_command.assert_not_called()
+
 
 if __name__ == "__main__":
     unittest.main()