file_utils: get_tool_path to always return an absolute path

Change get_tool_path to always return an absolute path and fail if
the required command cannot be found.

The unit tests have been adapted to not be path dependent and tests
for retrieving environment dependent paths have been added.

LP: #1785320

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

snapcraft/file_utils.py

@@ -33,6 +33,7 @@
     RequiredPathDoesNotExist,
     SnapcraftEnvironmentError,
     SnapcraftCopyFileNotFoundError,
+    ToolMissingError,
 )
 
 if sys.version_info < (3, 6):
@@ -340,19 +341,20 @@ def get_tool_path(command_name):
     :return: Path to command
     :rtype: str
     """
-    path = command_name
-
     if common.is_snap():
-        path = _command_path_in_root(os.getenv("SNAP"), command_name)
+        command_path = _command_path_in_root(os.getenv("SNAP"), command_name)
     elif common.is_docker_instance():
-        path = _command_path_in_root(
+        command_path = _command_path_in_root(
             os.path.join(os.sep, "snap", "snapcraft", "current"), command_name
         )
-
-    if path:
-        return path
     else:
-        return command_name
+        command_path = shutil.which(command_name)
+
+    # shutil.which will return None if it cannot find command_name
+    if command_path is None:
+        raise ToolMissingError(command_name=command_name)
+
+    return command_path
 
 
 def _command_path_in_root(root, command_name):

snapcraft/internal/errors.py

@@ -295,6 +295,17 @@ def __init__(self, message):
         super().__init__(message=message)
 
 
+class ToolMissingError(SnapcraftError):
+
+    fmt = (
+        "A tool snapcraft depends on could not be found: {command_name!r}.\n"
+        "Ensure the tool is installed and available, and try again."
+    )
+
+    def __init__(self, *, command_name: str) -> None:
+        super().__init__(command_name=command_name)
+
+
 class RequiredCommandFailure(SnapcraftError):
 
     fmt = "{command!r} failed."

snapcraft/internal/lifecycle/_packer.py

@@ -22,7 +22,7 @@
 from progressbar import AnimatedMarker, ProgressBar
 
 from snapcraft import file_utils
-from snapcraft.internal import common, repo
+from snapcraft.internal import common
 from snapcraft.internal.indicators import is_dumb_terminal
 
 
@@ -44,9 +44,6 @@ def _snap_data_from_dir(directory):
 def pack(directory, output=None):
     mksquashfs_path = file_utils.get_tool_path("mksquashfs")
 
-    # Check for our prerequesite external command early
-    repo.check_for_command(mksquashfs_path)
-
     snap = _snap_data_from_dir(directory)
     output_snap_name = output or common.format_snap_name(snap)
 

tests/unit/__init__.py

@@ -136,6 +136,13 @@ def setUp(self):
         self.cpu_count.return_value = 2
         self.addCleanup(patcher.stop)
 
+        # 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
+        )
+        patcher.start()
+        self.addCleanup(patcher.stop)
+
         patcher = mock.patch(
             "snapcraft.internal.indicators.ProgressBar", new=SilentProgressBar
         )

tests/unit/commands/test_push.py

@@ -14,7 +14,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 import os
-import subprocess
 from unittest import mock
 
 from testtools.matchers import Contains, Equals, FileExists, Not
@@ -27,7 +26,6 @@
     StoreUploadError,
 )
 import tests
-from tests import fixture_setup
 from . import CommandBaseTestCase
 
 
@@ -79,64 +77,6 @@ def test_push_a_snap(self):
         )
         mock_upload.assert_called_once_with("basic", self.snap_file)
 
-    def test_push_a_snap_running_from_snap(self):
-        self.useFixture(fixture_setup.FakeSnapcraftIsASnap())
-
-        mock_tracker = mock.Mock(storeapi._status_tracker.StatusTracker)
-        mock_tracker.track.return_value = {
-            "code": "ready_to_release",
-            "processed": True,
-            "can_release": True,
-            "url": "/fake/url",
-            "revision": 9,
-        }
-        patcher = mock.patch.object(storeapi.StoreClient, "upload")
-        mock_upload = patcher.start()
-        self.addCleanup(patcher.stop)
-        mock_upload.return_value = mock_tracker
-
-        original_exists = os.path.exists
-
-        def _fake_exists(path):
-            if path == "/snap/snapcraft/current/usr/bin/unsquashfs":
-                return True
-            else:
-                return original_exists(path)
-
-        actual_unsquashfs_path = None
-        original_check_output = subprocess.check_output
-
-        # Push requires unsquashfs to work, but we're faking it out to use one
-        # that doesn't exist. So we record what path it WOULD use, and then
-        # pass it through to the real one so the rest of the function works.
-        def _fake_check_output(*args, **kwargs):
-            nonlocal actual_unsquashfs_path
-            if "unsquashfs" in args[0][0]:
-                actual_unsquashfs_path = args[0][0]
-                args[0][0] = "unsquashfs"
-
-            return original_check_output(*args, **kwargs)
-
-        # Upload
-        with mock.patch("subprocess.check_output", side_effect=_fake_check_output):
-            with mock.patch("snapcraft.storeapi._status_tracker.StatusTracker"):
-                with mock.patch("os.path.exists", side_effect=_fake_exists):
-                    result = self.run_command(["push", self.snap_file])
-
-        self.assertThat(result.exit_code, Equals(0))
-
-        self.assertRegexpMatches(
-            self.fake_logger.output,
-            ".*push '.*test-snap.snap' to the store\.\n"
-            "Revision 9 of 'basic' created\.",
-        )
-        mock_upload.assert_called_once_with("basic", self.snap_file)
-
-        unsquashfs_path = os.path.join(
-            "/snap", "snapcraft", "current", "usr", "bin", "unsquashfs"
-        )
-        self.assertThat(actual_unsquashfs_path, Equals(unsquashfs_path))
-
     def test_push_without_login_must_raise_exception(self):
         raised = self.assertRaises(
             storeapi.errors.InvalidCredentialsError,

tests/unit/commands/test_snap.py

@@ -100,70 +100,6 @@ def test_snap_defaults(self):
             stdout=subprocess.PIPE,
         )
 
-    @mock.patch("snapcraft.internal.repo.check_for_command")
-    @mock.patch("snapcraft.internal.lifecycle._packer._run_mksquashfs")
-    def test_mksquashfs_from_snap_used_if_using_snap(
-        self, mock_run_mksquashfs, mock_check_command
-    ):
-        self.useFixture(fixture_setup.FakeSnapcraftIsASnap())
-
-        self.make_snapcraft_yaml()
-
-        original_exists = os.path.exists
-
-        def _fake_exists(path):
-            if path == "/snap/snapcraft/current/usr/bin/mksquashfs":
-                return True
-            else:
-                return original_exists(path)
-
-        with mock.patch("os.path.exists", side_effect=_fake_exists):
-            self.run_command(["snap"])
-
-        mksquashfs_path = os.path.join(
-            "/snap", "snapcraft", "current", "usr", "bin", "mksquashfs"
-        )
-
-        mock_run_mksquashfs.assert_called_once_with(
-            mksquashfs_path,
-            directory=self.prime_dir,
-            snap_name="snap-test",
-            snap_type="app",
-            output_snap_name="snap-test_1.0_amd64.snap",
-        )
-
-    @mock.patch("snapcraft.internal.common.is_docker_instance")
-    @mock.patch("snapcraft.internal.repo.check_for_command")
-    @mock.patch("snapcraft.internal.lifecycle._packer._run_mksquashfs")
-    def test_mksquashfs_from_snap_used_if_docker(
-        self, mock_run_mksquashfs, mock_check_command, mock_is_docker
-    ):
-        mock_is_docker.return_value = True
-        self.make_snapcraft_yaml()
-
-        original_exists = os.path.exists
-
-        def _fake_exists(path):
-            if path == "/snap/snapcraft/current/usr/bin/mksquashfs":
-                return True
-            else:
-                return original_exists(path)
-
-        with mock.patch("os.path.exists", side_effect=_fake_exists):
-            self.run_command(["snap"])
-
-        mksquashfs_path = os.path.join(
-            "/snap", "snapcraft", "current", "usr", "bin", "mksquashfs"
-        )
-
-        mock_run_mksquashfs.assert_called_once_with(
-            mksquashfs_path,
-            directory=self.prime_dir,
-            snap_name="snap-test",
-            snap_type="app",
-            output_snap_name="snap-test_1.0_amd64.snap",
-        )
-
     def test_snap_fails_with_bad_type(self):
         self.make_snapcraft_yaml(snap_type="bad-type")
 

tests/unit/lifecycle/test_lifecycle.py

@@ -1362,13 +1362,3 @@ def create_core_snap(self, deb_arch):
             print("description: description", file=f)
 
         return lifecycle.pack(directory=core_path)
-
-
-class SnapErrorsTestCase(LifecycleTestBase):
-    def test_mksquashfs_missing(self):
-        with mock.patch("shutil.which") as which_mock:
-            which_mock.return_value = None
-            raised = self.assertRaises(
-                errors.MissingCommandError, lifecycle.pack, mock.Mock()
-            )
-        self.assertThat(str(raised), Contains("mksquashfs"))

tests/unit/test_elf.py

@@ -368,34 +368,6 @@ def test_patch_does_nothing_if_no_interpreter(self):
         elf_patcher = elf.Patcher(dynamic_linker="/lib/fake-ld", root_path="/fake")
         elf_patcher.patch(elf_file=elf_file)
 
-    def test_tools_from_snap_used_if_using_snap(self):
-        self.useFixture(fixture_setup.FakeSnapcraftIsASnap())
-
-        real_exists = os.path.exists
-
-        def _fake_exists(path):
-            if path == "/snap/snapcraft/current/bin/patchelf":
-                return True
-            elif path == "/snap/snapcraft/current/usr/bin/strip":
-                return True
-            else:
-                return real_exists(path)
-
-        with mock.patch("os.path.exists", side_effect=_fake_exists):
-            # The base_path does not matter here as there are not files to
-            # be crawled for.
-            elf_patcher = elf.Patcher(dynamic_linker="/lib/fake-ld", root_path="/fake")
-
-        expected_patchelf = os.path.join(
-            "/snap", "snapcraft", "current", "bin", "patchelf"
-        )
-        self.assertThat(elf_patcher._patchelf_cmd, Equals(expected_patchelf))
-
-        expected_strip = os.path.join(
-            "/snap", "snapcraft", "current", "usr", "bin", "strip"
-        )
-        self.assertThat(elf_patcher._strip_cmd, Equals(expected_strip))
-
 
 class TestPatcherErrors(TestElfBase):
     def test_patch_fails_raises_patcherror_exception(self):
@@ -441,100 +413,6 @@ def test_patch_fails_with_old_version(self):
                 ]
             )
 
-    def test_patch_uses_snapped_strip(self):
-        self.useFixture(fixture_setup.FakeSnapcraftIsASnap())
-        self.fake_elf = fixture_setup.FakeElf(
-            root_path=self.path, patchelf_version="0.8"
-        )
-        self.useFixture(self.fake_elf)
-
-        elf_file = self.fake_elf["fake_elf-bad-patchelf"]
-
-        real_check_call = subprocess.check_call
-        real_check_output = subprocess.check_output
-        real_exists = os.path.exists
-
-        def _fake_check_call(*args, **kwargs):
-            if "patchelf" in args[0][0]:
-                self.assertThat(
-                    args[0][0], Equals("/snap/snapcraft/current/bin/patchelf")
-                )
-                args[0][0] = "patchelf"
-            elif "strip" in args[0][0]:
-                self.assertThat(
-                    args[0][0], Equals("/snap/snapcraft/current/usr/bin/strip")
-                )
-                args[0][0] = "strip"
-            real_check_call(*args, **kwargs)
-
-        def _fake_check_output(*args, **kwargs):
-            if "patchelf" in args[0][0]:
-                self.assertThat(
-                    args[0][0], Equals("/snap/snapcraft/current/bin/patchelf")
-                )
-                args[0][0] = "patchelf"
-            elif "strip" in args[0][0]:
-                self.assertThat(
-                    args[0][0], Equals("/snap/snapcraft/current/usr/bin/strip")
-                )
-                args[0][0] = "strip"
-            return real_check_output(*args, **kwargs)
-
-        def _fake_exists(path):
-            if path == "/snap/snapcraft/current/bin/patchelf":
-                return True
-            elif path == "/snap/snapcraft/current/usr/bin/strip":
-                return True
-            else:
-                return real_exists(path)
-
-        with mock.patch("subprocess.check_call") as mock_check_call:
-            with mock.patch("subprocess.check_output") as mock_check_output:
-                with mock.patch("os.path.exists", side_effect=_fake_exists):
-                    mock_check_call.side_effect = _fake_check_call
-                    mock_check_output.side_effect = _fake_check_output
-
-                    # The base_path does not matter here as there are not files
-                    # for which to crawl.
-                    elf_patcher = elf.Patcher(
-                        dynamic_linker="/lib/fake-ld", root_path="/fake"
-                    )
-                    self.assertRaises(
-                        errors.PatcherNewerPatchelfError,
-                        elf_patcher.patch,
-                        elf_file=elf_file,
-                    )
-
-                    # Test that .note.go.buildid is stripped off
-                    mock_check_call.assert_has_calls(
-                        [
-                            mock.call(
-                                [
-                                    "patchelf",
-                                    "--set-interpreter",
-                                    "/lib/fake-ld",
-                                    mock.ANY,
-                                ]
-                            ),
-                            mock.call(
-                                [
-                                    "strip",
-                                    "--remove-section",
-                                    ".note.go.buildid",
-                                    mock.ANY,
-                                ]
-                            ),
-                            mock.call(
-                                [
-                                    "patchelf",
-                                    "--set-interpreter",
-                                    "/lib/fake-ld",
-                                    mock.ANY,
-                                ]
-                            ),
-                        ]
-                    )
-
 
 class TestSonameCache(unit.TestCase):
     def setUp(self):