file utils: rename get_tool_path() to get_snap_tool_path() (#3231)

Scope get_tool_path() helper to finding tools that are expected to be
found in the snap. As before, if not running as a snap, use shutil.which().

- Rename get_tool_path() -> get_snap_tool_path().

- Remove special casing for "snap" command from get_snap_tool_path().

- Rename _command_path_in_root() to _find_command_path_in_root()
  and return None rather than empty string on error.

Signed-off-by: Chris Patterson <chris.patterson@canonical.com>
Co-authored-by: Sergio Schvezov <sergio.schvezov@canonical.com>

snapcraft/_store.py

@@ -32,7 +32,7 @@
 from snapcraft.cli import echo
 from tabulate import tabulate
 
-from snapcraft.file_utils import calculate_sha3_384, get_tool_path
+from snapcraft.file_utils import calculate_sha3_384, get_snap_tool_path
 from snapcraft import storeapi, yaml_utils
 from snapcraft.internal import cache, deltas, repo
 from snapcraft.internal.errors import SnapDataExtractionError, ToolMissingError
@@ -53,7 +53,7 @@
 
 def _get_data_from_snap_file(snap_path):
     with tempfile.TemporaryDirectory() as temp_dir:
-        unsquashfs_path = get_tool_path("unsquashfs")
+        unsquashfs_path = get_snap_tool_path("unsquashfs")
         try:
             output = subprocess.check_output(
                 [
@@ -80,7 +80,7 @@ def _get_data_from_snap_file(snap_path):
 def _get_icon_from_snap_file(snap_path):
     icon_file = None
     with tempfile.TemporaryDirectory() as temp_dir:
-        unsquashfs_path = get_tool_path("unsquashfs")
+        unsquashfs_path = get_snap_tool_path("unsquashfs")
         try:
             output = subprocess.check_output(
                 [

snapcraft/file_utils.py

@@ -335,39 +335,32 @@ def calculate_hash(path: str, *, algorithm: str) -> str:
     return hasher.hexdigest()
 
 
-def get_tool_path(command_name: str) -> str:
-    """Return the path to the given command
+def get_snap_tool_path(command_name: str) -> str:
+    """Return the path command found in the snap.
 
-    Return a path to command_name, if Snapcraft is running out of the snap
-    or in legacy mode (snap or sources), it ensures it is using the one in
-    the snap, not the host.
-    If a path cannot be resolved, ToolMissingError is raised.
+    If snapcraft is not running as a snap, shutil.which() is used
+    to resolve the command using PATH.
 
-    : param str command_name: the name of the command to resolve a path for.
-    :raises ToolMissingError: if command_name cannot be resolved to a path.
+    :param command_name: the name of the command to resolve a path for.
+    :raises ToolMissingError: if command_name was not found.
     :return: Path to command
-    :rtype: str
     """
-    command_path: Optional[str] = None
-
-    if common.is_snap() and command_name != "snap":
+    if common.is_snap():
         snap_path = os.getenv("SNAP")
         if snap_path is None:
             raise RuntimeError("SNAP not defined, but SNAP_NAME is?")
 
-        command_path = _command_path_in_root(snap_path, command_name)
+        command_path = _find_command_path_in_root(snap_path, command_name)
     else:
         command_path = shutil.which(command_name)
 
-    # shutil.which will return None if it cannot find command_name but
-    # _command_path_in_root will return an empty string.
-    if not command_path:
+    if command_path is None:
         raise ToolMissingError(command_name=command_name)
 
     return command_path
 
 
-def _command_path_in_root(root, command_name: str) -> str:
+def _find_command_path_in_root(root, command_name: str) -> Optional[str]:
     for bin_directory in (
         os.path.join("usr", "local", "sbin"),
         os.path.join("usr", "local", "bin"),
@@ -380,7 +373,7 @@ def _command_path_in_root(root, command_name: str) -> str:
         if os.path.exists(path):
             return path
 
-    return ""
+    return None
 
 
 def get_linker_version_from_file(linker_file: str) -> str:

snapcraft/internal/cache/_snap.py

@@ -41,7 +41,7 @@ def _setup_snap_cache_root(self):
 
     def _get_snap_deb_arch(self, snap_filename):
         with tempfile.TemporaryDirectory() as temp_dir:
-            unsquashfs_path = file_utils.get_tool_path("unsquashfs")
+            unsquashfs_path = file_utils.get_snap_tool_path("unsquashfs")
             output = subprocess.check_output(
                 [
                     unsquashfs_path,

snapcraft/internal/deltas/_deltas.py

@@ -56,7 +56,7 @@ def __init__(
         self.target_path = target_path
         self.delta_format = delta_format
         self.delta_file_extname = delta_file_extname
-        self.delta_tool_path = file_utils.get_tool_path(delta_tool)
+        self.delta_tool_path = file_utils.get_snap_tool_path(delta_tool)
 
         # some pre-checks
         self._check_properties()

snapcraft/internal/elf.py

@@ -486,9 +486,9 @@ def __init__(
         if preferred_patchelf_path:
             self._patchelf_cmd = preferred_patchelf_path
         else:
-            self._patchelf_cmd = file_utils.get_tool_path("patchelf")
+            self._patchelf_cmd = file_utils.get_snap_tool_path("patchelf")
 
-        self._strip_cmd = file_utils.get_tool_path("strip")
+        self._strip_cmd = file_utils.get_snap_tool_path("strip")
 
     def patch(self, *, elf_file: ElfFile) -> None:
         """Patch elf_file with the Patcher instance configuration.

snapcraft/internal/mangling.py

@@ -64,7 +64,7 @@ def clear_execstack(*, elf_files: FrozenSet[elf.ElfFile]) -> None:
     param elf.ElfFile elf_files: the full list of elf files to analyze
                                  and clear the execstack if present.
     """
-    execstack_path = file_utils.get_tool_path("execstack")
+    execstack_path = file_utils.get_snap_tool_path("execstack")
     elf_files_with_execstack = [e for e in elf_files if e.execstack_set]
 
     if elf_files_with_execstack:

snapcraft/plugins/v1/kernel.py

@@ -273,7 +273,7 @@ def _unpack_generic_initrd(self):
         os.makedirs(initrd_unpacked_path)
 
         with tempfile.TemporaryDirectory() as temp_dir:
-            unsquashfs_path = snapcraft.file_utils.get_tool_path("unsquashfs")
+            unsquashfs_path = snapcraft.file_utils.get_snap_tool_path("unsquashfs")
             subprocess.check_call(
                 [unsquashfs_path, self.os_snap, os.path.dirname(initrd_path)],
                 cwd=temp_dir,

tests/unit/__init__.py

@@ -159,7 +159,7 @@ def setUp(self):
 
         # We do not want the paths to affect every test we have.
         patcher = mock.patch(
-            "snapcraft.file_utils.get_tool_path", side_effect=lambda x: x
+            "snapcraft.file_utils.get_snap_tool_path", side_effect=lambda x: x
         )
         patcher.start()
         self.addCleanup(patcher.stop)

tests/unit/deltas/test_deltas.py

@@ -44,7 +44,7 @@ def setUp(self):
 
         self.useFixture(
             fixtures.MockPatch(
-                "snapcraft.file_utils.get_tool_path",
+                "snapcraft.file_utils.get_snap_tool_path",
                 side_effect=lambda x: os.path.join("/usr", "bin", x),
             )
         )

tests/unit/pluginhandler/test_clean.py

@@ -311,7 +311,7 @@ class TestCleanPrime:
     ]
 
     def test_clean_prime(self, monkeypatch, tmp_work_path, fileset):
-        monkeypatch.setattr(file_utils, "get_tool_path", lambda x: x)
+        monkeypatch.setattr(file_utils, "get_snap_tool_path", lambda x: x)
 
         handler = load_part("test_part", part_properties={"prime": fileset})
         handler.makedirs()

tests/unit/pluginhandler/test_patcher.py

@@ -137,7 +137,7 @@ def test_no_patcher_called(self, mock_partpatcher, snap_type, snap_name):
 
 
 def test_patcher_called(monkeypatch, mock_partpatcher):
-    monkeypatch.setattr(file_utils, "get_tool_path", lambda x: x)
+    monkeypatch.setattr(file_utils, "get_snap_tool_path", lambda x: x)
 
     handler = load_part(
         "test-part",

tests/unit/test_file_utils.py

@@ -312,13 +312,13 @@ def test_get_tool_from_host_path(self, monkeypatch, tool_path, fake_exists):
         fake_exists.paths = [abs_tool_path]
         monkeypatch.setattr(shutil, "which", lambda x: abs_tool_path.as_posix())
 
-        assert file_utils.get_tool_path("tool-command") == abs_tool_path.as_posix()
+        assert file_utils.get_snap_tool_path("tool-command") == abs_tool_path.as_posix()
 
     def test_get_tool_from_snapcraft_snap_path(self, in_snap, tool_path, fake_exists):
         abs_tool_path = pathlib.Path("/snap/snapcraft/current") / tool_path
         fake_exists.paths = [abs_tool_path]
 
-        assert file_utils.get_tool_path("tool-command") == abs_tool_path.as_posix()
+        assert file_utils.get_snap_tool_path("tool-command") == abs_tool_path.as_posix()
 
     def test_get_tool_from_docker_snap_path(
         self, monkeypatch, in_snap, tool_path, fake_exists
@@ -327,22 +327,22 @@ def test_get_tool_from_docker_snap_path(
         fake_exists.paths = [abs_tool_path]
         monkeypatch.setattr(common, "is_process_container", lambda: True)
 
-        assert file_utils.get_tool_path("tool-command") == abs_tool_path.as_posix()
+        assert file_utils.get_snap_tool_path("tool-command") == abs_tool_path.as_posix()
 
 
 class GetToolPathErrorsTest(testtools.TestCase):
-    def test_get_tool_path_fails(self):
+    def test_get_snap_tool_path_fails(self):
         self.assertRaises(
             file_utils.ToolMissingError,
-            file_utils.get_tool_path,
+            file_utils.get_snap_tool_path,
             "non-existent-tool-command",
         )
 
-    def test_get_tool_path_in_container_fails_root(self):
+    def test_get_snap_tool_path_in_container_fails_root(self):
         self.useFixture(fixture_setup.FakeSnapcraftIsASnap())
 
         self.assertRaises(
             file_utils.ToolMissingError,
-            file_utils.get_tool_path,
+            file_utils.get_snap_tool_path,
             "non-existent-tool-command",
         )