From 696c8d4941745a190034111b6bba54142db014a5 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Tue, 14 Oct 2025 15:16:32 +0100 Subject: [PATCH 1/5] Avoid recreating unchanged global aliases, and use hard links where possible. Fixes #184 --- src/manage/commands.py | 4 +++ src/manage/install_command.py | 60 +++++++++++++++++++++++++++++++++-- tests/test_install_command.py | 20 +++++++----- 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/src/manage/commands.py b/src/manage/commands.py index ec57c09..f17722a 100644 --- a/src/manage/commands.py +++ b/src/manage/commands.py @@ -351,6 +351,10 @@ class BaseCommand: show_help = False def __init__(self, args, root=None): + # Storage for command-specific data per-command execution. + # All data should use a unique key. + self.scratch = {} + cmd_args = { k: v for k, v in [*CLI_SCHEMA.items(), *CLI_SCHEMA.get(self.CMD, {}).items()] diff --git a/src/manage/install_command.py b/src/manage/install_command.py index eaff783..fd3d706 100644 --- a/src/manage/install_command.py +++ b/src/manage/install_command.py @@ -225,11 +225,12 @@ def _if_exists(launcher, plat): def _write_alias(cmd, install, alias, target): p = (cmd.global_dir / alias["name"]) + target = Path(target) ensure_tree(p) - unlink(p) launcher = cmd.launcher_exe if alias.get("windowed"): launcher = cmd.launcherw_exe or launcher + plat = install["tag"].rpartition("-")[-1] if plat: LOGGER.debug("Checking for launcher for platform -%s", plat) @@ -247,8 +248,61 @@ def _write_alias(cmd, install, alias, target): else: LOGGER.debug("Skipping %s alias because the launcher template was not found.", alias["name"]) return - p.write_bytes(launcher.read_bytes()) - p.with_name(p.name + ".__target__").write_text(str(target), encoding="utf-8") + + launcher_remap = cmd.scratch.setdefault("install_command._write_alias.launcher_remap", {}) + LOGGER.debug("remap %s", launcher_remap) + try: + launcher = launcher_remap[str(launcher)] + except KeyError: + pass + + try: + launcher_bytes = launcher.read_bytes() + except OSError: + warnings_shown = cmd.scratch.setdefault("install_command._write_alias.warnings_shown", set()) + if str(launcher) not in warnings_shown: + LOGGER.warn("Failed to read launcher template at %s.", launcher) + warnings_shown.add(str(launcher)) + LOGGER.debug(exc_info=True) + return + + existing_bytes = b'' + try: + with open(p, 'rb') as f: + existing_bytes = f.read(len(launcher_bytes) + 1) + except FileNotFoundError: + pass + except OSError: + LOGGER.debug("Failed to read existing alias launcher.") + + if existing_bytes != launcher_bytes: + # First try and create a hard link + unlink(p) + try: + os.link(launcher, p) + LOGGER.debug("Created %s as hard link to %s", p.name, launcher.name) + except OSError as ex: + if ex.winerror != 17: + # Report errors other than cross-drive links + LOGGER.debug("Failed to create hard link for command.", exc_info=True) + try: + p.write_bytes(launcher_bytes) + LOGGER.debug("Created %s as copy of %s", p.name, launcher.name) + launcher_remap[str(launcher)] = p + except OSError: + LOGGER.error("Failed to create global command %s.", alias["name"]) + LOGGER.debug(exc_info=True) + + p_target = p.with_name(p.name + ".__target__") + try: + if target.match(p_target.read_text(encoding="utf-8")): + return + except FileNotFoundError: + pass + except (OSError, UnicodeDecodeError): + LOGGER.debug("Failed to read existing target path.", exc_info=True) + + p_target.write_text(str(target), encoding="utf-8") def _create_shortcut_pep514(cmd, install, shortcut): diff --git a/tests/test_install_command.py b/tests/test_install_command.py index 2e0f445..6509cc1 100644 --- a/tests/test_install_command.py +++ b/tests/test_install_command.py @@ -22,6 +22,7 @@ class Cmd: default_platform = "-64" def __init__(self, platform=None): + self.scratch = {} if platform: self.default_platform = platform @@ -81,19 +82,19 @@ def check_warm64(self, cmd, tag, name): def test_write_alias_tag_with_platform(alias_checker): - alias_checker.check_32(alias_checker.Cmd, "1.0-32", "testA") - alias_checker.check_w32(alias_checker.Cmd, "1.0-32", "testB") - alias_checker.check_64(alias_checker.Cmd, "1.0-64", "testC") - alias_checker.check_w64(alias_checker.Cmd, "1.0-64", "testD") - alias_checker.check_arm64(alias_checker.Cmd, "1.0-arm64", "testE") - alias_checker.check_warm64(alias_checker.Cmd, "1.0-arm64", "testF") + alias_checker.check_32(alias_checker.Cmd(), "1.0-32", "testA") + alias_checker.check_w32(alias_checker.Cmd(), "1.0-32", "testB") + alias_checker.check_64(alias_checker.Cmd(), "1.0-64", "testC") + alias_checker.check_w64(alias_checker.Cmd(), "1.0-64", "testD") + alias_checker.check_arm64(alias_checker.Cmd(), "1.0-arm64", "testE") + alias_checker.check_warm64(alias_checker.Cmd(), "1.0-arm64", "testF") def test_write_alias_default_platform(alias_checker): alias_checker.check_32(alias_checker.Cmd("-32"), "1.0", "testA") alias_checker.check_w32(alias_checker.Cmd("-32"), "1.0", "testB") - alias_checker.check_64(alias_checker.Cmd, "1.0", "testC") - alias_checker.check_w64(alias_checker.Cmd, "1.0", "testD") + alias_checker.check_64(alias_checker.Cmd(), "1.0", "testC") + alias_checker.check_w64(alias_checker.Cmd(), "1.0", "testD") alias_checker.check_arm64(alias_checker.Cmd("-arm64"), "1.0", "testE") alias_checker.check_warm64(alias_checker.Cmd("-arm64"), "1.0", "testF") @@ -110,6 +111,7 @@ def test_write_alias_default(alias_checker, monkeypatch, tmp_path, default): class Cmd: global_dir = Path(tmp_path) / "bin" launcher_exe = None + scratch = {} def get_installs(self): return [ { @@ -146,6 +148,7 @@ def write_alias(*a): def test_print_cli_shortcuts(patched_installs, assert_log, monkeypatch, tmp_path): class Cmd: + scratch = {} global_dir = Path(tmp_path) def get_installs(self): return installs.get_installs(None) @@ -163,6 +166,7 @@ def get_installs(self): def test_print_path_warning(patched_installs, assert_log, tmp_path): class Cmd: + scratch = {} global_dir = Path(tmp_path) def get_installs(self): return installs.get_installs(None) From 2a78d33bb25ec420e897ff8decd4e231ec84a17d Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Tue, 14 Oct 2025 19:20:24 +0100 Subject: [PATCH 2/5] Improved coverage and handling of link fallbacks --- src/manage/install_command.py | 39 +++++++++++++++----------- tests/test_install_command.py | 53 +++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 16 deletions(-) diff --git a/src/manage/install_command.py b/src/manage/install_command.py index fd3d706..f2984f7 100644 --- a/src/manage/install_command.py +++ b/src/manage/install_command.py @@ -249,13 +249,6 @@ def _write_alias(cmd, install, alias, target): LOGGER.debug("Skipping %s alias because the launcher template was not found.", alias["name"]) return - launcher_remap = cmd.scratch.setdefault("install_command._write_alias.launcher_remap", {}) - LOGGER.debug("remap %s", launcher_remap) - try: - launcher = launcher_remap[str(launcher)] - except KeyError: - pass - try: launcher_bytes = launcher.read_bytes() except OSError: @@ -263,7 +256,7 @@ def _write_alias(cmd, install, alias, target): if str(launcher) not in warnings_shown: LOGGER.warn("Failed to read launcher template at %s.", launcher) warnings_shown.add(str(launcher)) - LOGGER.debug(exc_info=True) + LOGGER.debug("Failed to read %s", launcher, exc_info=True) return existing_bytes = b'' @@ -275,7 +268,13 @@ def _write_alias(cmd, install, alias, target): except OSError: LOGGER.debug("Failed to read existing alias launcher.") - if existing_bytes != launcher_bytes: + launcher_remap = cmd.scratch.setdefault("install_command._write_alias.launcher_remap", {}) + + if existing_bytes == launcher_bytes: + # Valid existing launcher, so save its path in case we need it later + # for a hard link. + launcher_remap.setdefault(launcher.name, p) + else: # First try and create a hard link unlink(p) try: @@ -285,13 +284,21 @@ def _write_alias(cmd, install, alias, target): if ex.winerror != 17: # Report errors other than cross-drive links LOGGER.debug("Failed to create hard link for command.", exc_info=True) - try: - p.write_bytes(launcher_bytes) - LOGGER.debug("Created %s as copy of %s", p.name, launcher.name) - launcher_remap[str(launcher)] = p - except OSError: - LOGGER.error("Failed to create global command %s.", alias["name"]) - LOGGER.debug(exc_info=True) + launcher2 = launcher_remap.get(launcher.name) + if launcher2: + try: + os.link(launcher, p) + except OSError: + LOGGER.debug("Failed to create hard link from fallback launcher") + launcher2 = None + if not launcher2: + try: + p.write_bytes(launcher_bytes) + LOGGER.debug("Created %s as copy of %s", p.name, launcher.name) + launcher_remap[launcher.name] = p + except OSError: + LOGGER.error("Failed to create global command %s.", alias["name"]) + LOGGER.debug(exc_info=True) p_target = p.with_name(p.name + ".__target__") try: diff --git a/tests/test_install_command.py b/tests/test_install_command.py index 6509cc1..e391d22 100644 --- a/tests/test_install_command.py +++ b/tests/test_install_command.py @@ -104,6 +104,59 @@ def test_write_alias_fallback_platform(alias_checker): alias_checker.check_w64(alias_checker.Cmd("-spam"), "1.0", "testB") +def test_write_alias_launcher_missing(fake_config, assert_log, tmp_path): + fake_config.launcher_exe = tmp_path / "non-existent.exe" + fake_config.default_platform = '-32' + fake_config.global_dir = tmp_path / "bin" + IC._write_alias( + fake_config, + {"tag": "test"}, + {"name": "test.exe"}, + tmp_path / "target.exe", + ) + assert_log( + "Checking for launcher.*", + "Checking for launcher.*", + "Checking for launcher.*", + "Create %s linking to %s", + "Skipping %s alias because the launcher template was not found.", + assert_log.end_of_log(), + ) + + +def test_write_alias_launcher_unreadable(fake_config, assert_log, tmp_path): + class FakeLauncherPath: + stem = "test" + suffix = ".exe" + parent = tmp_path + + @staticmethod + def is_file(): + return True + + @staticmethod + def read_bytes(): + raise OSError("no reading for the test") + + fake_config.scratch = {} + fake_config.launcher_exe = FakeLauncherPath + fake_config.default_platform = '-32' + fake_config.global_dir = tmp_path / "bin" + IC._write_alias( + fake_config, + {"tag": "test"}, + {"name": "test.exe"}, + tmp_path / "target.exe", + ) + assert_log( + "Checking for launcher.*", + "Create %s linking to %s", + "Failed to read launcher template at %s.", + "Failed to read %s", + assert_log.end_of_log(), + ) + + @pytest.mark.parametrize("default", [1, 0]) def test_write_alias_default(alias_checker, monkeypatch, tmp_path, default): prefix = Path(tmp_path) / "runtime" From c339db154dfbcae20d969af99e7c71e11f2dfa57 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Wed, 15 Oct 2025 00:34:16 +0100 Subject: [PATCH 3/5] Replace os.link with parameter for easier testing --- src/manage/install_command.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/manage/install_command.py b/src/manage/install_command.py index f2984f7..99291d8 100644 --- a/src/manage/install_command.py +++ b/src/manage/install_command.py @@ -223,7 +223,7 @@ def _if_exists(launcher, plat): return launcher -def _write_alias(cmd, install, alias, target): +def _write_alias(cmd, install, alias, target, _link=os.link): p = (cmd.global_dir / alias["name"]) target = Path(target) ensure_tree(p) @@ -278,7 +278,7 @@ def _write_alias(cmd, install, alias, target): # First try and create a hard link unlink(p) try: - os.link(launcher, p) + _link(launcher, p) LOGGER.debug("Created %s as hard link to %s", p.name, launcher.name) except OSError as ex: if ex.winerror != 17: @@ -287,7 +287,7 @@ def _write_alias(cmd, install, alias, target): launcher2 = launcher_remap.get(launcher.name) if launcher2: try: - os.link(launcher, p) + _link(launcher2, p) except OSError: LOGGER.debug("Failed to create hard link from fallback launcher") launcher2 = None From 2c8b20c44202b4b1f68eafb29ea559c0c2b68417 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 17 Oct 2025 14:20:11 +0100 Subject: [PATCH 4/5] Improved test coverage --- src/manage/install_command.py | 7 ++++ tests/test_install_command.py | 63 ++++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/manage/install_command.py b/src/manage/install_command.py index 99291d8..19eb6c5 100644 --- a/src/manage/install_command.py +++ b/src/manage/install_command.py @@ -288,8 +288,15 @@ def _write_alias(cmd, install, alias, target, _link=os.link): if launcher2: try: _link(launcher2, p) + LOGGER.debug("Created %s as hard link to %s", p.name, launcher2.name) + except FileNotFoundError: + raise except OSError: LOGGER.debug("Failed to create hard link from fallback launcher") + try: + launcher_bytes = launcher2.read_bytes() + except OSError: + LOGGER.debug("Failed to read fallback launcher", exc_info=True) launcher2 = None if not launcher2: try: diff --git a/tests/test_install_command.py b/tests/test_install_command.py index e391d22..ec48de2 100644 --- a/tests/test_install_command.py +++ b/tests/test_install_command.py @@ -151,12 +151,73 @@ def read_bytes(): assert_log( "Checking for launcher.*", "Create %s linking to %s", - "Failed to read launcher template at %s.", + "Failed to read launcher template at %s\\.", "Failed to read %s", assert_log.end_of_log(), ) +def test_write_alias_launcher_unlinkable(fake_config, assert_log, tmp_path): + def fake_link(x, y): + raise OSError("Error for testing") + + fake_config.scratch = {} + fake_config.launcher_exe = tmp_path / "launcher.txt" + fake_config.launcher_exe.write_bytes(b'Arbitrary contents') + fake_config.default_platform = '-32' + fake_config.global_dir = tmp_path / "bin" + IC._write_alias( + fake_config, + {"tag": "test"}, + {"name": "test.exe"}, + tmp_path / "target.exe", + _link=fake_link + ) + assert_log( + "Checking for launcher.*", + "Create %s linking to %s", + "Failed to create hard link.+", + "Created %s as copy of %s", + assert_log.end_of_log(), + ) + + +def test_write_alias_launcher_unlinkable_remap(fake_config, assert_log, tmp_path): + # This is for the fairly expected case of the PyManager install being on one + # drive, but the global commands directory being on another. In this + # situation, we can't hard link directly into the app files, and will need + # to copy. But we only need to copy once, so if a launcher_remap has been + # set (in the current process), then we have an available copy already and + # can link to that. + + def fake_link(x, y): + if x.match("launcher.txt"): + raise OSError(17, "Error for testing") + + fake_config.scratch = { + "install_command._write_alias.launcher_remap": {"launcher.txt": tmp_path / "actual_launcher.txt"}, + } + fake_config.launcher_exe = tmp_path / "launcher.txt" + fake_config.launcher_exe.write_bytes(b'Arbitrary contents') + (tmp_path / "actual_launcher.txt").write_bytes(b'Arbitrary contents') + fake_config.default_platform = '-32' + fake_config.global_dir = tmp_path / "bin" + IC._write_alias( + fake_config, + {"tag": "test"}, + {"name": "test.exe"}, + tmp_path / "target.exe", + _link=fake_link + ) + assert_log( + "Checking for launcher.*", + "Create %s linking to %s", + "Failed to create hard link.+", + ("Created %s as hard link to %s", ("test.exe", "actual_launcher.txt")), + assert_log.end_of_log(), + ) + + @pytest.mark.parametrize("default", [1, 0]) def test_write_alias_default(alias_checker, monkeypatch, tmp_path, default): prefix = Path(tmp_path) / "runtime" From d6e73ecd38e717aeb43717a7697f795397feaa2f Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 17 Oct 2025 14:47:26 +0100 Subject: [PATCH 5/5] Don't reread fallback launchers --- src/manage/install_command.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/manage/install_command.py b/src/manage/install_command.py index 19eb6c5..ac67e61 100644 --- a/src/manage/install_command.py +++ b/src/manage/install_command.py @@ -292,11 +292,7 @@ def _write_alias(cmd, install, alias, target, _link=os.link): except FileNotFoundError: raise except OSError: - LOGGER.debug("Failed to create hard link from fallback launcher") - try: - launcher_bytes = launcher2.read_bytes() - except OSError: - LOGGER.debug("Failed to read fallback launcher", exc_info=True) + LOGGER.debug("Failed to create hard link to fallback launcher") launcher2 = None if not launcher2: try: