Merge pull request #705 from ddalcino/topic/ddalcino/cli/warn-ext7zr

Log a warning when aqtinstall falls back to an external 7z extraction tool

aqt/installer.py

@@ -233,21 +233,27 @@ def _warning_on_bad_combination(self, combo_message: str) -> str:
             "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 @@ def run_install_qt(self, args: InstallArgParser):
             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 @@ def _run_src_doc_examples(self, flavor, args, cmd_name: Optional[str] = None):
         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 @@ def run_install_tool(self, args: InstallToolArgParser):
         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)

tests/test_cli.py

@@ -7,6 +7,7 @@
 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 _mocked_run(*args):
     )
 
 
-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(