file_utils: get_tool_path to always return an absolute path (#2193)

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/cli/_errors.py

@@ -48,6 +48,7 @@
     Sorry, Snapcraft ran into an error when trying to running through its
     lifecycle that generated a trace that has been put in {!r}."""
 )
+_MSG_REPORTABLE_ERROR = "This is a problem in snapcraft; a trace has been put in {!r}."
 _MSG_SEND_TO_SENTRY_TRACEBACK_PROMPT = dedent(
     """\
     You can anonymously report this issue to the snapcraft developers.
@@ -77,7 +78,9 @@
 _ALWAYS_VALUES = ["always", "a"]
 
 
-def exception_handler(exception_type, exception, exception_traceback, *, debug=False):
+def exception_handler(
+    exception_type, exception, exception_traceback, *, debug=False
+) -> None:
     """Catch all Snapcraft exceptions unless debugging.
 
     This function is the global excepthook, properly handling uncaught
@@ -98,13 +101,16 @@ def exception_handler(exception_type, exception, exception_traceback, *, debug=F
     exc_info = (exception_type, exception, exception_traceback)
     exit_code = 1
     is_snapcraft_error = issubclass(exception_type, errors.SnapcraftError)
+    is_snapcraft_reportable_error = issubclass(
+        exception_type, errors.SnapcraftReportableError
+    )
     is_raven_setup = RavenClient is not None
     is_snapcraft_managed_host = (
         distutils.util.strtobool(os.getenv("SNAPCRAFT_MANAGED_HOST", "n")) == 1
     )
 
     if not is_snapcraft_error:
-        _handle_trace_output(exc_info, is_snapcraft_managed_host)
+        _handle_trace_output(exc_info, is_snapcraft_managed_host, _MSG_TRACEBACK_FILE)
         if not is_raven_setup:
             echo.warning(_MSG_RAVEN_MISSING)
         elif _is_send_to_sentry():
@@ -120,14 +126,23 @@ def exception_handler(exception_type, exception, exception_traceback, *, debug=F
         # of a double error print
         if exception_type != lxd_errors.ContainerSnapcraftCmdError:
             echo.error(str(exception))
+        if is_snapcraft_reportable_error and is_raven_setup:
+            _handle_trace_output(
+                exc_info, is_snapcraft_managed_host, _MSG_REPORTABLE_ERROR
+            )
+            if _is_send_to_sentry():
+                _submit_trace(exc_info)
+                click.echo(_MSG_SEND_TO_SENTRY_THANKS)
     else:
         click.echo("Unhandled error case")
         exit_code = -1
 
     sys.exit(exit_code)
 
 
-def _handle_trace_output(exc_info, is_snapcraft_managed_host: bool) -> None:
+def _handle_trace_output(
+    exc_info, is_snapcraft_managed_host: bool, trace_file_msg_tmpl: str
+) -> None:
     if is_snapcraft_managed_host:
         click.echo(_MSG_TRACEBACK_PRINT)
         traceback.print_exception(*exc_info)
@@ -139,7 +154,7 @@ def _handle_trace_output(exc_info, is_snapcraft_managed_host: bool) -> None:
             traceback.print_exception(
                 exc_info[0], exc_info[1], exc_info[2], file=trace_file
             )
-        click.echo(_MSG_TRACEBACK_FILE.format(trace_filepath))
+        click.echo(trace_file_msg_tmpl.format(trace_filepath))
 
 
 def _is_send_to_sentry() -> bool:

snapcraft/file_utils.py

@@ -33,6 +33,7 @@
     RequiredPathDoesNotExist,
     SnapcraftEnvironmentError,
     SnapcraftCopyFileNotFoundError,
+    ToolMissingError,
 )
 
 if sys.version_info < (3, 6):
@@ -330,29 +331,32 @@ def calculate_hash(path: str, *, algorithm: str) -> str:
     return hasher.hexdigest()
 
 
-def get_tool_path(command_name):
+def get_tool_path(command_name: str) -> str:
     """Return the path to the given command
 
-    By default this utilizes the PATH, but if Snapcraft is running out of the
-    snap or out of Docker, it ensures it's using the one in the snap, not the
-    host.
+    Return a path to command_name, if Snapcraft is running out of the snap or out
+    of Docker, it ensures it is using the one in the snap, not the host.
+    If a path cannot be resolved, ToolMissingError is raised.
 
+    : 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.
     :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

@@ -41,6 +41,18 @@ def get_exit_code(self):
         return 2
 
 
+class SnapcraftReportableError(SnapcraftError):
+    """Base class for all snapcraft exceptions that integrate with Sentry.
+
+    An exception class inheriting from this will get a prompt to report
+    to sentry if raised and is handled by the exception handler on program
+    exit.
+
+    :cvar fmt: A format string that daughter classes override
+
+    """
+
+
 class MissingStateCleanError(SnapcraftError):
     fmt = (
         "Failed to clean: "
@@ -295,6 +307,17 @@ def __init__(self, message):
         super().__init__(message=message)
 
 
+class ToolMissingError(SnapcraftReportableError):
+
+    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

@@ -26,7 +26,6 @@
 
 import fixtures
 from testtools.matchers import (
-    Contains,
     DirExists,
     Equals,
     FileContains,
@@ -1362,13 +1361,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"))