Repository: miurahr/aqtinstall
Pull request #705 (2023-08-27 22:43:37)

Log a warning when aqtinstall falls back to an external 7z extraction tool
Inspired by https://github.com/miurahr/aqtinstall/issues/696#issuecomment-1651026317. That comment suggested a place to add a log message, but I was unable to log the message there without disregarding the user’s selected log settings, which are not available until the `Cli` object is built.

When `aqtinstall` fails to load the `py7zr` module, it falls back to whatever 7z extraction tool the user has specified in the `settings.ini` file, without notifying the user. This PR adds the following log message when that happens:

```
WARNING: The py7zr module failed to load. Falling back to '7z' for .7z extraction.
WARNING: You can use the  '--external | -E' flags to select your own extraction tool.
```

This PR also does a little refactoring of the `Cli._set_sevenzip` method to improve readability.

**Side effect**
There was an existing bug here, where the `install-qt` subcommand (but none of the other commands) would ignore the `7zcmd` parameter in the `settings.ini` file, and use the hardcoded `"7z"` instead. I'm pretty sure this was a bug; if not I can change it back.

https://github.com/miurahr/aqtinstall/blob/72da54f4b0156a1bbd0aea09c621992817f963f8/aqt/installer.py#L361-L363


Patch:
diff --git a/aqt/installer.py b/aqt/installer.py
index 2f9e791..0c39176 100644
--- a/aqt/installer.py
+++ b/aqt/installer.py
@@ -233,21 +233,27 @@ class Cli:
             "This may not install properly, but we will try our best."
         )
 
-    def _set_sevenzip(self, external):
+    def _set_sevenzip(self, external: Optional[str]) -> Optional[str]:
         sevenzip = external
+        fallback = Settings.zipcmd
         if sevenzip is None:
-            return None
-
+            if EXT7Z:
+                self.logger.warning(f"The py7zr module failed to load. Falling back to '{fallback}' for .7z extraction.")
+                self.logger.warning("You can use the  '--external | -E' flags to select your own extraction tool.")
+                sevenzip = fallback
+            else:
+                # Just use py7zr
+                return None
         try:
             subprocess.run(
                 [sevenzip, "--help"],
                 stdout=subprocess.DEVNULL,
                 stderr=subprocess.DEVNULL,
             )
+            return sevenzip
         except FileNotFoundError as e:
-            raise CliInputError("Specified 7zip command executable does not exist: {!r}".format(sevenzip)) from e
-
-        return sevenzip
+            qualifier = "Specified" if sevenzip == external else "Fallback"
+            raise CliInputError(f"{qualifier} 7zip command executable does not exist: '{sevenzip}'") from e
 
     @staticmethod
     def _set_arch(arch: Optional[str], os_name: str, target: str, qt_version_or_spec: str) -> str:
@@ -358,9 +364,6 @@ class Cli:
             timeout = (Settings.connection_timeout, Settings.response_timeout)
         modules = args.modules
         sevenzip = self._set_sevenzip(args.external)
-        if EXT7Z and sevenzip is None:
-            # override when py7zr is not exist
-            sevenzip = self._set_sevenzip("7z")
         if args.base is not None:
             if not self._check_mirror(args.base):
                 raise CliInputError(
@@ -475,9 +478,6 @@ class Cli:
         else:
             timeout = (Settings.connection_timeout, Settings.response_timeout)
         sevenzip = self._set_sevenzip(args.external)
-        if EXT7Z and sevenzip is None:
-            # override when py7zr is not exist
-            sevenzip = self._set_sevenzip(Settings.zipcmd)
         modules = getattr(args, "modules", None)  # `--modules` is invalid for `install-src`
         archives = args.archives
         all_extra = True if modules is not None and "all" in modules else False
@@ -549,9 +549,6 @@ class Cli:
         else:
             base_dir = output_dir
         sevenzip = self._set_sevenzip(args.external)
-        if EXT7Z and sevenzip is None:
-            # override when py7zr is not exist
-            sevenzip = self._set_sevenzip(Settings.zipcmd)
         version = getattr(args, "version", None)
         if version is not None:
             Cli._validate_version_str(version, allow_minus=True)


Test patch:
diff --git a/tests/test_cli.py b/tests/test_cli.py
index 91a568b..b8985e9 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -7,6 +7,7 @@ from typing import Dict, List, Optional
 import pytest
 
 from aqt.exceptions import CliInputError
+from aqt.helper import Settings
 from aqt.installer import Cli
 from aqt.metadata import MetadataFactory, SimpleSpec, Version
 
@@ -382,13 +383,63 @@ def test_cli_unexpected_error(monkeypatch, capsys):
     )
 
 
-def test_cli_set_7zip(monkeypatch):
+@pytest.mark.parametrize("external_tool_exists", (True, False))
+def test_set_7zip_checks_external_tool_when_specified(monkeypatch, capsys, external_tool_exists: bool):
     cli = Cli()
     cli._setup_settings()
-    with pytest.raises(CliInputError) as err:
-        cli._set_sevenzip("some_nonexistent_binary")
-    assert err.type == CliInputError
-    assert format(err.value) == "Specified 7zip command executable does not exist: 'some_nonexistent_binary'"
+    external = "my_7z_extractor"
+
+    def mock_subprocess_run(args, **kwargs):
+        assert args[0] == external
+        if not external_tool_exists:
+            raise FileNotFoundError()
+
+    monkeypatch.setattr("aqt.installer.subprocess.run", mock_subprocess_run)
+    monkeypatch.setattr("aqt.installer.EXT7Z", False)
+    if external_tool_exists:
+        assert external == cli._set_sevenzip(external)
+    else:
+        with pytest.raises(CliInputError) as err:
+            cli._set_sevenzip(external)
+        assert format(err.value) == format(f"Specified 7zip command executable does not exist: '{external}'")
+    assert capsys.readouterr()[1] == ""
+
+
+@pytest.mark.parametrize("fallback_exists", (True, False))
+def test_set_7zip_uses_fallback_when_py7zr_missing(monkeypatch, capsys, fallback_exists: bool):
+    cli = Cli()
+    cli._setup_settings()
+    external, fallback = None, Settings.zipcmd
+
+    def mock_subprocess_run(args, **kwargs):
+        assert args[0] == fallback
+        if not fallback_exists:
+            raise FileNotFoundError()
+
+    monkeypatch.setattr("aqt.installer.subprocess.run", mock_subprocess_run)
+    monkeypatch.setattr("aqt.installer.EXT7Z", True)
+    if fallback_exists:
+        assert fallback == cli._set_sevenzip(external)
+    else:
+        with pytest.raises(CliInputError) as err:
+            cli._set_sevenzip(external)
+        assert format(err.value) == format(f"Fallback 7zip command executable does not exist: '{fallback}'")
+    assert f"Falling back to '{fallback}'" in capsys.readouterr()[1]
+
+
+@pytest.mark.parametrize("fallback_exists", (True, False))
+def test_set_7zip_chooses_p7zr_when_ext_missing(monkeypatch, capsys, fallback_exists: bool):
+    cli = Cli()
+    cli._setup_settings()
+    external = None
+
+    def mock_subprocess_run(args, **kwargs):
+        assert False, "Should not try to run anything"
+
+    monkeypatch.setattr("aqt.installer.subprocess.run", mock_subprocess_run)
+    monkeypatch.setattr("aqt.installer.EXT7Z", False)
+    assert cli._set_sevenzip(external) is None
+    assert capsys.readouterr()[1] == ""
 
 
 @pytest.mark.parametrize(