From e1ef2142031e2310349daf98415d1098f0fa2670 Mon Sep 17 00:00:00 2001 From: Anton Maklakov Date: Tue, 16 Jan 2024 12:56:49 +0700 Subject: [PATCH] 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 | 3 ++ tools/idf_tools.py | 60 ++++++++++++--------- tools/test_idf_tools/test_idf_tools.py | 75 ++++++++++++++++++++++++-- 3 files changed, 109 insertions(+), 29 deletions(-) diff --git a/.gitlab/ci/host-test.yml b/.gitlab/ci/host-test.yml index 8f78b88a38..edca658bd4 100644 --- a/.gitlab/ci/host-test.yml +++ b/.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 diff --git a/tools/idf_tools.py b/tools/idf_tools.py index 4dd75b04b7..4c0cfe8b68 100755 --- a/tools/idf_tools.py +++ b/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: - warn(f'directory for tool {self.name} version {version} is present, but tool was not found') + except ToolNotFoundError as e: + 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 ' diff --git a/tools/test_idf_tools/test_idf_tools.py b/tools/test_idf_tools/test_idf_tools.py index dcfb753523..09f2d19553 100755 --- a/tools/test_idf_tools/test_idf_tools.py +++ b/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 -# TestUsage class serves as a parent for classes TestUsageUnix and TestUsageWin -class TestUsage(unittest.TestCase): +# TestUsageBase takes care of general test setup to use the idf_tools commands +class TestUsageBase(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()