From 76f9a08963244f42289b3fbf39e72335614ad866 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Thu, 28 Nov 2024 21:28:30 +1100 Subject: [PATCH] Fix failure to reinstall an extension uninstalling the extension When an extension could could be removed but it's directory could be renamed, the install operation would fail anyway. Resolve by completing the installation when the directory can be moved. Report as part of #129884. --- scripts/addons_core/bl_pkg/cli/blender_ext.py | 13 +++- .../bl_pkg/tests/test_cli_blender.py | 73 +++++++++++++++++-- 2 files changed, 78 insertions(+), 8 deletions(-) diff --git a/scripts/addons_core/bl_pkg/cli/blender_ext.py b/scripts/addons_core/bl_pkg/cli/blender_ext.py index 1690240767f..a9abf1b1c74 100755 --- a/scripts/addons_core/bl_pkg/cli/blender_ext.py +++ b/scripts/addons_core/bl_pkg/cli/blender_ext.py @@ -4065,8 +4065,17 @@ class subcmd_client: filepath_local_pkg, temp_prefix_and_suffix=temp_prefix_and_suffix, )) is not None: - msglog.error("Failed to remove existing directory for \"{:s}\": {:s}".format(manifest.id, error)) - return False + if os.path.isdir(filepath_local_pkg): + msglog.error("Failed to remove or relocate existing directory for \"{:s}\": {:s}".format( + manifest.id, + error, + )) + return False + + msglog.status("Relocated directory that could not be removed \"{:s}\": {:s}".format( + manifest.id, + error, + )) is_reinstall = True diff --git a/scripts/addons_core/bl_pkg/tests/test_cli_blender.py b/scripts/addons_core/bl_pkg/tests/test_cli_blender.py index d4a257968ee..c02c6b67f42 100644 --- a/scripts/addons_core/bl_pkg/tests/test_cli_blender.py +++ b/scripts/addons_core/bl_pkg/tests/test_cli_blender.py @@ -379,6 +379,17 @@ user_dirs: tuple[str, ...] = ( ) +# Recursively remove with a fallback, needed for tests that create read-only files. +def _rmtree_with_chmod(dir_remove: str) -> None: + try: + shutil.rmtree(dir_remove) + except PermissionError: + if sys.platform != "win32": + for root, _dirs, _files in os.walk(dir_remove): + os.chmod(root, 0o777) + shutil.rmtree(dir_remove) + + class TestWithTempBlenderUser_MixIn(unittest.TestCase): @staticmethod @@ -392,11 +403,11 @@ class TestWithTempBlenderUser_MixIn(unittest.TestCase): @staticmethod def _repo_dirs_destroy() -> None: for dirname in user_dirs: - shutil.rmtree(os.path.join(TEMP_DIR_BLENDER_USER, dirname)) + _rmtree_with_chmod(os.path.join(TEMP_DIR_BLENDER_USER, dirname)) if os.path.exists(TEMP_DIR_REMOTE): - shutil.rmtree(TEMP_DIR_REMOTE) + _rmtree_with_chmod(TEMP_DIR_REMOTE) if os.path.exists(TEMP_DIR_LOCAL): - shutil.rmtree(TEMP_DIR_LOCAL) + _rmtree_with_chmod(TEMP_DIR_LOCAL) def setUp(self) -> None: self._repo_dirs_create() @@ -404,12 +415,12 @@ class TestWithTempBlenderUser_MixIn(unittest.TestCase): def tearDown(self) -> None: self._repo_dirs_destroy() - def repo_add(self, *, repo_id: str, repo_name: str) -> None: + def repo_add(self, *, repo_id: str, repo_name: str, use_remote: bool = True) -> None: stdout = run_blender_extensions_no_errors(( "repo-add", "--name", repo_name, "--directory", TEMP_DIR_LOCAL, - "--url", TEMP_DIR_REMOTE_AS_URL, + *(("--url", TEMP_DIR_REMOTE_AS_URL) if use_remote else ()), # A bit odd, this argument avoids running so many commands to setup a test. "--clear-all", repo_id, @@ -429,7 +440,7 @@ class TestWithTempBlenderUser_MixIn(unittest.TestCase): blender_version_max: str | None = None, python_script: str | None = None, file_contents: dict[str, bytes] | None = None, - ) -> None: + ) -> str: if pkg_filename is None: pkg_filename = pkg_idname pkg_output_filepath = os.path.join(TEMP_DIR_REMOTE, pkg_filename + PKG_EXT) @@ -459,6 +470,7 @@ class TestWithTempBlenderUser_MixIn(unittest.TestCase): "created: \"{:s}\", {:d}\n" ).format(pkg_filename, PKG_EXT, pkg_output_filepath, os.path.getsize(pkg_output_filepath)), ) + return pkg_output_filepath class TestSimple(TestWithTempBlenderUser_MixIn, unittest.TestCase): @@ -637,6 +649,55 @@ class TestSimple(TestWithTempBlenderUser_MixIn, unittest.TestCase): print(TEMP_DIR_BLENDER_USER) pause_until_keyboard_interrupt() + def test_install_files(self) -> None: + + repo_id = "test_repo_module_name" + repo_name = "MyTestRepo" + + self.repo_add(repo_id=repo_id, repo_name=repo_name, use_remote=False) + + wheel_module_name = "my_custom_wheel" + + # Create a package contents. + pkg_idname = "my_test_pkg" + pkg_filepath = self.build_package( + pkg_idname=pkg_idname, + wheel_params=( + WheelModuleParams( + module_name=wheel_module_name, + module_version="1.0.1", + ), + ), + ) + + stdout = run_blender_extensions_no_errors(("install-file", pkg_filepath, "--repo=" + repo_id)) + self.assertEqual( + [line for line in stdout.split("\n") if line.startswith("STATUS ")], + ["STATUS Installed \"my_test_pkg\""], + ) + + stdout = run_blender_extensions_no_errors(("install-file", pkg_filepath, "--repo=" + repo_id)) + self.assertEqual( + [line for line in stdout.split("\n") if line.startswith("STATUS ")], + ["STATUS Reinstalled \"my_test_pkg\""], + ) + + # Permissions are system specific, run this last. + if sys.platform != "win32": + pkg_dir_local = os.path.join(TEMP_DIR_LOCAL, pkg_idname) + + contents_to_filesystem({"readonly_dir/readonly_file": b"\n"}, pkg_dir_local) + os.chmod(os.path.join(pkg_dir_local, "readonly_dir", "readonly_file"), 0o555) + os.chmod(os.path.join(pkg_dir_local, "readonly_dir"), 0o555) + + stdout = run_blender_extensions_no_errors(("install-file", pkg_filepath, "--repo=" + repo_id)) + + status = [line for line in stdout.split("\n") if line.startswith("STATUS ")] + + # Ignore the actual permission exception as this is system dependent. + self.assertRegex(status[0], "^STATUS Relocated directory that could not be removed \"my_test_pkg\": .*") + self.assertEqual(status[1], "STATUS Reinstalled \"my_test_pkg\"") + class TestPlatform(TestWithTempBlenderUser_MixIn, unittest.TestCase): def test_platform_filter(self) -> None: