feat(tools): stop installation if tool is invalid

install/check commands - stop on error
export/list commands - print a warning

.gitlab/ci/host-test.yml

@@ -132,11 +132,14 @@ test_cli_installer:
     entrypoint: [""]  # use system python3. no extra pip package installed
   script:
     # Tools must be downloaded for testing
+    # We could use "idf_tools.py download all", but we don't want to install clang because of its huge size
     - python3 ${IDF_PATH}/tools/idf_tools.py download required qemu-riscv32 qemu-xtensa
     - cd ${IDF_PATH}/tools/test_idf_tools
     - python3 -m pip install jsonschema
     - python3 ./test_idf_tools.py -v
     - python3 ./test_idf_tools_python_env.py
+    # It runs at the end because it modifies dependencies
+    - IDF_TEST_MAY_BREAK_DEPENDENCIES=1 python3 ./test_idf_tools.py -v TestSystemDependencies.test_commands_when_nodeps
 
 .test_efuse_table_on_host_template:
   extends: .host_test_template

tools/idf_tools.py

@@ -502,7 +502,7 @@ def report_progress(count: int, block_size: int, total_size: int) -> None:
 def mkdir_p(path: str) -> None:
     """
     Makes directory in given path.
-    Supresses error when directory is already created or path is a path to file.
+    Suppresses error when directory is already created or path is a path to file.
     """
     try:
         os.makedirs(path)
@@ -607,7 +607,7 @@ def urlretrieve_ctx(url: str,
 
 def download(url: str, destination: str) -> Union[None, Exception]:
     """
-    Download from given url and save into given destiantion.
+    Download from given url and save into given destination.
     """
     info(f'Downloading {url}')
     info(f'Destination: {destination}')
@@ -764,7 +764,7 @@ class IDFToolVersion(object):
             platform_name = Platforms.get(platform_name)
             if platform_name in self.downloads.keys():
                 return self.downloads[platform_name]
-        # exception can be ommited, as not detected platform is handled without err message
+        # exception can be omitted, as not detected platform is handled without err message
         except ValueError:
             pass
         if 'any' in self.downloads.keys():
@@ -927,11 +927,11 @@ class IDFTool(object):
 
         try:
             version_cmd_result = run_cmd_check_output(cmd, None, extra_paths)
-        except OSError:
+        except OSError as e:
             # tool is not on the path
-            raise ToolNotFoundError(f'Tool {self.name} not found')
+            raise ToolNotFoundError(f'Tool {self.name} not found with error: {e}')
         except subprocess.CalledProcessError as e:
-            raise ToolExecError(f'returned non-zero exit code ({e.returncode}) with error message:\n{e.stderr.decode("utf-8",errors="ignore")}')  # type: ignore
+            raise ToolExecError(f'non-zero exit code ({e.returncode}) with message: {e.stderr.decode("utf-8",errors="ignore")}')  # type: ignore
 
         in_str = version_cmd_result.decode('utf-8')
         match = re.search(self._current_options.version_regex, in_str)  # type: ignore
@@ -939,6 +939,19 @@ class IDFTool(object):
             return UNKNOWN_VERSION
         return re.sub(self._current_options.version_regex, self._current_options.version_regex_replace, match.group(0))  # type: ignore
 
+    def check_binary_valid(self, version: str) -> bool:
+        if not self.is_executable:
+            return True
+        try:
+            ver_str = self.get_version(self.get_export_paths(version))
+        except (ToolNotFoundError, ToolExecError) as e:
+            fatal(f'tool {self.name} version {version} is installed, but getting error: {e}')
+            return False
+        if ver_str != version:
+            # just print, state is still valid
+            warn(f'tool {self.name} version {version} is installed, but reporting version {ver_str}')
+        return True
+
     def check_version(self, executable_path: Optional[str]) -> bool:
         """
         Check if tool's version from executable path is in self.version dictionary.
@@ -982,7 +995,7 @@ class IDFTool(object):
 
     def get_recommended_version(self) -> Optional[str]:
         """
-        Get all reccomended versions of the tool. If more versions are recommended, highest version is returned.
+        Get all recommended versions of the tool. If more versions are recommended, highest version is returned.
         """
         recommended_versions = [k for k, v in self.versions.items()
                                 if v.status == IDFToolVersion.STATUS_RECOMMENDED
@@ -1021,7 +1034,7 @@ class IDFTool(object):
             # not in PATH
             pass
         except ToolExecError as e:
-            fatal(f'tool {self.name} found in path, but {e}')
+            fatal(f'tool {self.name} is found in PATH, but has failed: {e}')
             tool_error = True
         else:
             self.version_in_path = ver_str
@@ -1040,10 +1053,10 @@ class IDFTool(object):
                 continue
             try:
                 ver_str = self.get_version(self.get_export_paths(version))
-            except ToolNotFoundError:
+            except ToolNotFoundError as e:
-                warn(f'directory for tool {self.name} version {version} is present, but tool was not found')
+                warn(f'directory for tool {self.name} version {version} is present, but the tool has not been found: {e}')
             except ToolExecError as e:
-                fatal(f'tool {self.name} version {version} is installed, but {e}')
+                fatal(f'tool {self.name} version {version} is installed, but cannot be run: {e}')
                 tool_error = True
             else:
                 if ver_str != version:
@@ -1138,6 +1151,10 @@ class IDFTool(object):
         unpack(archive_path, dest_dir)
         if self._current_options.strip_container_dirs:  # type: ignore
             do_strip_container_dirs(dest_dir, self._current_options.strip_container_dirs)  # type: ignore
+        if not self.check_binary_valid(version):
+            fatal(f'Failed to check the tool while installed. Removing directory {dest_dir}')
+            shutil.rmtree(dest_dir)
+            raise SystemExit(1)
 
     @staticmethod
     def check_download_file(download_obj: IDFToolDownload, local_path: str) -> bool:
@@ -1691,7 +1708,7 @@ def get_python_env_path() -> Tuple[str, str, str, str]:
 
 def parse_tools_arg(tools_str: List[str]) -> List[str]:
     """
-    Base parsing "tools" argumets: all, required, etc.
+    Base parsing "tools" arguments: all, required, etc.
     """
     if not tools_str:
         return ['required']
@@ -1836,7 +1853,7 @@ def add_variables_to_deactivate_file(args: List[str], new_idf_vars:Dict[str, Any
 
 def deactivate_statement(args: List[str]) -> None:
     """
-    Deactivate statement is sequence of commands, that remove IDF global variables from enviroment,
+    Deactivate statement is sequence of commands, that remove IDF global variables from environment,
     so the environment gets to the state it was before calling export.{sh/fish} script.
     """
     env_state_obj = ENVState.get_env_state()
@@ -1916,7 +1933,6 @@ def list_default(args):  # type: ignore
     Prints currently installed versions of all tools compatible with current platform.
     """
     tools_info = load_tools_info()
-    tool_error = False
     for name, tool in tools_info.items():
         if tool.get_install_type() == IDFTool.INSTALL_NEVER:
             continue
@@ -1925,7 +1941,7 @@ def list_default(args):  # type: ignore
         try:
             tool.find_installed_versions()
         except ToolBinaryError:
-            tool_error = True
+            pass
         versions_for_platform = {k: v for k, v in tool.versions.items() if v.compatible_with_platform()}
         if not versions_for_platform:
             info(f'  (no versions compatible with platform {PYTHON_PLATFORM})')
@@ -1935,8 +1951,6 @@ def list_default(args):  # type: ignore
             version_obj = tool.versions[version]
             info('  - {} ({}{})'.format(version, version_obj.status,
                                         ', installed' if version in tool.versions_installed else ''))
-    if tool_error:
-        raise SystemExit(1)
 
 
 def list_outdated(args):  # type: ignore
@@ -2074,7 +2088,7 @@ def process_tool(
     try:
         tool.find_installed_versions()
     except ToolBinaryError:
-        tool_found = False
+        pass
     recommended_version_to_use = tool.get_preferred_installed_version()
 
     if not tool.is_executable and recommended_version_to_use:
@@ -2299,7 +2313,7 @@ def get_tools_spec_and_platform_info(selected_platform: str, targets: List[str],
 def action_download(args):  # type: ignore
     """
     Saves current IDF environment and for every tools in tools_spec, downloads the right archive for tools version and target platform, if possible.
-    If not, prints apropriate message to stderr  and raise SystemExit() expception.
+    If not, prints appropriate message to stderr and raise SystemExit() exception.
     """
     tools_spec = parse_tools_arg(args.tools)
 
@@ -2375,7 +2389,6 @@ def action_install(args):  # type: ignore
     tools_info = load_tools_info()
     tools_spec = expand_tools_arg(tools_spec, tools_info, targets)
     info(f'Installing tools: {", ".join(tools_spec)}')
-    tool_error = False
     for tool_spec in tools_spec:
         if '@' not in tool_spec:
             tool_name = tool_spec
@@ -2398,7 +2411,7 @@ def action_install(args):  # type: ignore
         try:
             tool_obj.find_installed_versions()
         except ToolBinaryError:
-            tool_error = True
+            pass
         tool_spec = f'{tool_name}@{tool_version}'
         if tool_version in tool_obj.versions_installed:
             info(f'Skipping {tool_spec} (already installed)')
@@ -2411,9 +2424,6 @@ def action_install(args):  # type: ignore
         tool_obj.download(tool_version)
         tool_obj.install(tool_version)
 
-    if tool_error:
-        raise SystemExit(1)
-
 
 def get_wheels_dir() -> Optional[str]:
     """
@@ -3133,7 +3143,7 @@ def main(argv: List[str]) -> None:
     install_python_env.add_argument('--extra-wheels-url', help='Additional URL with wheels', default=IDF_PIP_WHEELS_URL)
     install_python_env.add_argument('--no-index', help='Work offline without retrieving wheels index')
     install_python_env.add_argument('--features', default='core', help=('A comma separated list of desired features for installing. '
-                                                                        'It defaults to installing just the core funtionality.'))
+                                                                        'It defaults to installing just the core functionality.'))
     install_python_env.add_argument('--no-constraints', action='store_true', default=no_constraints_default,
                                     help=('Disable constraint settings. Use with care and only when you want to manage '
                                           'package versions by yourself. It can be set with the IDF_PYTHON_CHECK_CONSTRAINTS '

tools/test_idf_tools/test_idf_tools.py

@@ -9,10 +9,12 @@ import shutil
 import sys
 import tempfile
 import unittest
+from subprocess import check_call
+from subprocess import STDOUT
 from unittest.mock import patch
 
 try:
-    from contextlib import redirect_stdout
+    from contextlib import redirect_stdout, redirect_stderr
 except ImportError:
     import contextlib
 
@@ -23,6 +25,13 @@ except ImportError:
         yield
         sys.stdout = original
 
+    @contextlib.contextmanager  # type: ignore
+    def redirect_stderr(target):
+        original = sys.stderr
+        sys.stderr = target
+        yield
+        sys.stderr = original
+
 try:
     from cStringIO import StringIO
 except ImportError:
@@ -97,9 +106,8 @@ XTENSA_ELF_ARCHIVE_PATTERN = XTENSA_ELF + '-' \
     + (XTENSA_ELF_VERSION[len('esp-'):] if XTENSA_ELF_VERSION.startswith('esp-') else XTENSA_ELF_VERSION)
 
 
-# TestUsage takes care of general test setup and executes tests that behaves the same on both platforms
+# TestUsageBase takes care of general test setup to use the idf_tools commands
-# TestUsage class serves as a parent for classes TestUsageUnix and TestUsageWin
+class TestUsageBase(unittest.TestCase):
-class TestUsage(unittest.TestCase):
 
     @classmethod
     def setUpClass(cls):
@@ -159,6 +167,30 @@ class TestUsage(unittest.TestCase):
         output = output_stream.getvalue()
         return output
 
+    def run_idf_tools_with_error(self, action, assertError=False):
+        output_stream = StringIO()
+        error_stream = StringIO()
+        is_error_occured = False
+        try:
+            with redirect_stderr(error_stream), redirect_stdout(output_stream):
+                idf_tools.main(['--non-interactive'] + action)
+        except SystemExit:
+            is_error_occured = True
+            if assertError:
+                pass
+            else:
+                raise
+        if assertError:
+            self.assertTrue(is_error_occured, 'idf_tools should fail if assertError is set')
+        output = output_stream.getvalue()
+        error = error_stream.getvalue()
+        return output, error
+
+
+# TestUsage executes tests that behaves the same on both platforms
+# TestUsage class serves as a parent for classes TestUsageUnix and TestUsageWin
+class TestUsage(TestUsageBase):
+
     def test_tools_for_wildcards1(self):
         required_tools_installed = 2
         output = self.run_idf_tools_with_action(['install', '*gdb*'])
@@ -1071,5 +1103,40 @@ class TestArmDetection(unittest.TestCase):
                     self.assertEqual(idf_tools.Platforms.detect_linux_arm_platform(platform), exec_platform)
 
 
+# TestSystemDependencies tests behaviour when system dependencies had been broken, on linux
+@unittest.skipUnless(sys.platform == 'linux', reason='Tools for LINUX differ')
+@unittest.skipUnless(int(os.getenv('IDF_TEST_MAY_BREAK_DEPENDENCIES', 0)) == 1, reason='Do not run destructive tests by default')
+class TestSystemDependencies(TestUsageBase):
+
+    @classmethod
+    def tearDownClass(cls):
+        # We won't restore dependencies because `apt-get update` and `apt-get install` require network access and take a lot of time on the runners
+        print('\nINFO: System dependencies have been modified for TestSystemDependencies. Other tests may not work correctly')
+        super(TestSystemDependencies, cls).tearDownClass()
+
+    def test_commands_when_nodeps(self):
+        # Prerequisites
+        # (don't go to the setUpClass() because it's a part of this test case)
+
+        self.run_idf_tools_with_action(['install', 'required', 'qemu-xtensa'])
+        with open(os.devnull, 'w') as devnull:
+            check_call(['dpkg', '--purge', 'libsdl2-2.0-0'], stdout=devnull, stderr=STDOUT)
+
+        # Tests
+
+        _, err_output = self.run_idf_tools_with_error(['list'])
+        self.assertIn(f'ERROR: tool {QEMU_XTENSA} version {QEMU_XTENSA_VERSION} is installed, but cannot be run', err_output)
+
+        _, err_output = self.run_idf_tools_with_error(['export'])
+        self.assertIn(f'ERROR: tool {QEMU_XTENSA} version {QEMU_XTENSA_VERSION} is installed, but cannot be run', err_output)
+
+        _,err_output = self.run_idf_tools_with_error(['check'], assertError=True)
+        self.assertIn(f'ERROR: tool {QEMU_XTENSA} version {QEMU_XTENSA_VERSION} is installed, but cannot be run', err_output)
+
+        _,err_output = self.run_idf_tools_with_error(['install', 'qemu-xtensa'], assertError=True)
+        self.assertIn(f'ERROR: tool {QEMU_XTENSA} version {QEMU_XTENSA_VERSION} is installed, but getting error', err_output)
+        self.assertIn(f'ERROR: Failed to check the tool while installed', err_output)
+
+
 if __name__ == '__main__':
     unittest.main()