Repository: miurahr/aqtinstall
Pull request #415 (2021-10-01 00:59:58)

Fix usage of `disable_socket`
In an earlier PR, I added calls to `disable_socket()` from `pytest_socket` where I thought they were needed to prevent some tests from accessing the network, in case they weren't monkeypatched properly. Today, I discovered that `disable_socket()` disables sockets globally for all tests, which means that the tests that use remote data cannot run if they are executed after another test calls `disable_socket()`.

This change calls `disable_socket()` once from `conftest.py`, so that no tests are allowed to use network data unless they are marked as OK to use the network, with `@pytest.mark.enable_socket`. See example of usage in `tests/test_connection.py`.

See [pytest_socket docs](https://github.com/miketheman/pytest-socket).

ddalcino: Test failed due to checksum error while downloading an archive. This is unrelated to the changed code.

Patch:
diff --git a/aqt/exceptions.py b/aqt/exceptions.py
index ae4a8c1..8c46d25 100644
--- a/aqt/exceptions.py
+++ b/aqt/exceptions.py
@@ -43,6 +43,10 @@ class ArchiveDownloadError(AqtException):
     pass
 
 
+class ArchiveChecksumError(ArchiveDownloadError):
+    pass
+
+
 class ArchiveConnectionError(AqtException):
     pass
 
diff --git a/aqt/helper.py b/aqt/helper.py
index 2bc6407..2e3b74f 100644
--- a/aqt/helper.py
+++ b/aqt/helper.py
@@ -28,13 +28,14 @@ import sys
 import xml.etree.ElementTree as ElementTree
 from logging import getLogger
 from logging.handlers import QueueListener
-from typing import Callable, Dict, List
+from pathlib import Path
+from typing import Callable, Dict, List, Tuple
 from urllib.parse import urlparse
 
 import requests
 import requests.adapters
 
-from aqt.exceptions import ArchiveConnectionError, ArchiveDownloadError, ArchiveListError
+from aqt.exceptions import ArchiveChecksumError, ArchiveConnectionError, ArchiveDownloadError, ArchiveListError
 
 
 def _get_meta(url: str):
@@ -49,7 +50,9 @@ def _check_content_type(ct: str) -> bool:
 def getUrl(url: str, timeout) -> str:
     logger = getLogger("aqt.helper")
     with requests.Session() as session:
-        retries = requests.adapters.Retry(total=Settings.max_retries, backoff_factor=Settings.backoff_factor)
+        retries = requests.adapters.Retry(
+            total=Settings.max_retries_on_connection_error, backoff_factor=Settings.backoff_factor
+        )
         adapter = requests.adapters.HTTPAdapter(max_retries=retries)
         session.mount("http://", adapter)
         session.mount("https://", adapter)
@@ -76,10 +79,13 @@ def getUrl(url: str, timeout) -> str:
     return result
 
 
-def downloadBinaryFile(url: str, out: str, hash_algo: str, exp: str, timeout):
+def downloadBinaryFile(url: str, out: str, hash_algo: str, exp: bytes, timeout):
     logger = getLogger("aqt.helper")
+    filename = Path(url).name
     with requests.Session() as session:
-        retries = requests.adapters.Retry(total=Settings.max_retries, backoff_factor=Settings.backoff_factor)
+        retries = requests.adapters.Retry(
+            total=Settings.max_retries_on_connection_error, backoff_factor=Settings.backoff_factor
+        )
         adapter = requests.adapters.HTTPAdapter(max_retries=retries)
         session.mount("http://", adapter)
         session.mount("https://", adapter)
@@ -103,13 +109,31 @@ def downloadBinaryFile(url: str, out: str, hash_algo: str, exp: str, timeout):
                         hash.update(chunk)
                     fd.flush()
             except Exception as e:
-                raise ArchiveDownloadError(f"Download error: {e}") from e
+                raise ArchiveDownloadError(f"Download of {filename} has error: {e}") from e
             if exp is not None and hash.digest() != exp:
-                raise ArchiveDownloadError(
-                    f"Download file is corrupted! Detect checksum error.\nExpected {exp}, Actual {hash.digest()}"
+                raise ArchiveChecksumError(
+                    f"Downloaded file {filename} is corrupted! Detect checksum error.\n"
+                    f"Expect {exp.hex()}: {url}\n"
+                    f"Actual {hash.digest().hex()}: {out}"
                 )
 
 
+def retry_on_errors(action: Callable[[], any], acceptable_errors: Tuple, num_retries: int, name: str):
+    logger = getLogger("aqt.helper")
+    for i in range(num_retries):
+        try:
+            retry_msg = f": attempt #{1 + i}" if i > 0 else ""
+            logger.info(f"{name}{retry_msg}...")
+            ret = action()
+            if i > 0:
+                logger.info(f"Success on attempt #{1 + i}: {name}")
+            return ret
+        except acceptable_errors as e:
+            if i < num_retries - 1:
+                continue  # just try again
+            raise e from e
+
+
 def altlink(url: str, alt: str):
     """
     Blacklisting redirected(alt) location based on Settings.blacklist configuration.
@@ -313,8 +337,17 @@ class SettingsClass:
 
     @property
     def max_retries(self):
+        """Deprecated: please use `max_retries_on_connection_error` and `max_retries_on_checksum_error` instead!"""
         return self.config.getfloat("requests", "max_retries", fallback=5)
 
+    @property
+    def max_retries_on_connection_error(self):
+        return self.config.getfloat("requests", "max_retries_on_connection_error", fallback=self.max_retries)
+
+    @property
+    def max_retries_on_checksum_error(self):
+        return self.config.getint("requests", "max_retries_on_checksum_error", fallback=int(self.max_retries))
+
     @property
     def backoff_factor(self):
         return self.config.getfloat("requests", "retry_backoff", fallback=0.1)
diff --git a/aqt/installer.py b/aqt/installer.py
index 04c15a0..37dce63 100644
--- a/aqt/installer.py
+++ b/aqt/installer.py
@@ -39,6 +39,7 @@ import aqt
 from aqt.archives import QtArchives, QtPackage, SrcDocExamplesArchives, ToolArchives
 from aqt.exceptions import (
     AqtException,
+    ArchiveChecksumError,
     ArchiveConnectionError,
     ArchiveDownloadError,
     ArchiveExtractionError,
@@ -47,7 +48,7 @@ from aqt.exceptions import (
     CliKeyboardInterrupt,
     OutOfMemory,
 )
-from aqt.helper import MyQueueListener, Settings, downloadBinaryFile, getUrl, setup_logging
+from aqt.helper import MyQueueListener, Settings, downloadBinaryFile, getUrl, retry_on_errors, setup_logging
 from aqt.metadata import ArchiveId, MetadataFactory, QtRepoProperty, SimpleSpec, Version, show_list, suggested_follow_up
 from aqt.updater import Updater
 
@@ -64,6 +65,8 @@ class Cli:
 
     __slot__ = ["parser", "combinations", "logger"]
 
+    UNHANDLED_EXCEPTION_CODE = 254
+
     def __init__(self):
         parser = argparse.ArgumentParser(
             prog="aqt",
@@ -112,7 +115,7 @@ class Cli:
                 "Please file a bug report at https://github.com/miurahr/aqtinstall/issues.\n"
                 "Please remember to include a copy of this program's output in your report."
             )
-            return 1
+            return Cli.UNHANDLED_EXCEPTION_CODE
 
     def _check_tools_arg_combination(self, os_name, tool_name, arch):
         for c in Settings.tools_combinations:
@@ -895,14 +898,18 @@ def installer(
         logger.removeHandler(handler)
     logger.addHandler(qh)
     #
-    logger.info("Downloading {}...".format(name))
     logger.debug("Download URL: {}".format(url))
     if response_timeout is None:
         timeout = (Settings.connection_timeout, Settings.response_timeout)
     else:
         timeout = (Settings.connection_timeout, response_timeout)
     hash = binascii.unhexlify(getUrl(hashurl, timeout))
-    downloadBinaryFile(url, archive, "sha1", hash, timeout)
+    retry_on_errors(
+        action=lambda: downloadBinaryFile(url, archive, "sha1", hash, timeout),
+        acceptable_errors=(ArchiveChecksumError,),
+        num_retries=Settings.max_retries_on_checksum_error,
+        name=f"Downloading {name}",
+    )
     if command is None:
         with py7zr.SevenZipFile(archive, "r") as szf:
             szf.extractall(path=base_dir)
diff --git a/aqt/settings.ini b/aqt/settings.ini
index b8cd797..0faf319 100644
--- a/aqt/settings.ini
+++ b/aqt/settings.ini
@@ -9,8 +9,9 @@ print_stacktrace_on_error: False
 [requests]
 connection_timeout: 3.5
 response_timeout: 30
-max_retries: 5
+max_retries_on_connection_error: 5
 retry_backoff: 0.1
+max_retries_on_checksum_error: 5
 
 [mirrors]
 blacklist:
diff --git a/docs/configuration.rst b/docs/configuration.rst
index 46bd93e..e683324 100644
--- a/docs/configuration.rst
+++ b/docs/configuration.rst
@@ -17,10 +17,16 @@ A file is like as follows:
 
     [aqt]
     concurrency: 4
-    connection_timeout: 3.5
-    response_timeout: 30
     baseurl: https://download.qt.io
     7zcmd: 7z
+    print_stacktrace_on_error: False
+
+    [requests]
+    connection_timeout: 3.5
+    response_timeout: 30
+    max_retries_on_connection_error: 5
+    retry_backoff: 0.1
+    max_retries_on_checksum_error: 5
 
     [mirrors]
     blacklist:
@@ -33,24 +39,20 @@ A file is like as follows:
         http://ftp1.nluug.nl/languages/qt
         https://mirrors.dotsrc.org/qtproject
 
+    [kde_patches]
+    patches:
+        0001-toolchain.prf-Use-vswhere-to-obtain-VS-installation-.patch
+
 
 Settings
 --------
 
-An ``[aqt]`` section is a configuration for basic behavior.
+The ``[aqt]`` section configures basic behavior.
 
 concurrency:
     ``concurrency`` is a setting how many download concurrently starts.
     It should be a integer value.
 
-connection_timeout:
-    ``connection_timeout`` is a timeout in second for connection.
-    It is passed to ``requests`` library.
-
-response_timeout:
-    ``response_timeout`` is a timeout in second how much time waiting for response.
-    It is passed to ``requests`` library.
-
 baseurl:
     ``baseurl`` is a URL of Qt download site.
     When you have your own Qt download site repository, you can set it here.
@@ -62,8 +64,43 @@ baseurl:
     ``py7zr`` library.
     When ``--external`` option specified, a value is override with option's one.
 
+print_stacktrace_on_error:
+    ``print_stacktrace_on_error`` is either ``True`` or ``False``.
+    The ``True`` setting causes a stack trace to be printed to stderr any time
+    an error occurs that will end the program.
+    The ``False`` setting will hide the stack trace, unless an unhandled
+    exception occurs.
+
+
+The ``[requests]`` section controls the way that ``aqt`` makes network requests.
+
+connection_timeout:
+    ``connection_timeout`` is a timeout in second for connection.
+    It is passed to ``requests`` library.
+
+response_timeout:
+    ``response_timeout`` is a timeout in second how much time waiting for response.
+    It is passed to ``requests`` library.
+
+max_retries:
+    Deprecated; please do not use this setting. It has been replaced by the
+
+max_retries_on_connection_error:
+    ``max_retries_on_connection_error`` is an integer that controls how many times
+    ``aqt`` will try to reconnect to the server in the case of a connection error.
+
+retry_backoff:
+    ``retry_backoff`` is a floating point number that controls how long ``aqt``
+    will sleep between failed connection attempts.
+    Setting this value too low will hammer the server, and may result
+    in no successful connections at all.
+
+max_retries_on_checksum_error:
+    This setting controls how many times ``aqt`` will attempt to download a file,
+    in the case of a checksum error.
+
 
-A ``[mirrors]`` section is a configuration for mirror handling.
+The ``[mirrors]`` section is a configuration for mirror handling.
 
 blacklist:
     It is a list of URL where is a problematic mirror site.


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index e69de29..b4e630a 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -0,0 +1,5 @@
+from pytest_socket import disable_socket
+
+
+def pytest_runtest_setup():
+    disable_socket()
diff --git a/tests/test_cli.py b/tests/test_cli.py
index 09427ca..233458f 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -294,7 +294,7 @@ def test_cli_unexpected_error(monkeypatch, capsys):
 
     cli = Cli()
     cli._setup_settings()
-    assert 1 == cli.run(["install-qt", "mac", "ios", "6.2.0"])
+    assert Cli.UNHANDLED_EXCEPTION_CODE == cli.run(["install-qt", "mac", "ios", "6.2.0"])
     out, err = capsys.readouterr()
     assert err.startswith("Some unexpected error")
     assert err.rstrip().endswith(
diff --git a/tests/test_connection.py b/tests/test_connection.py
index 8d29d13..43049ac 100644
--- a/tests/test_connection.py
+++ b/tests/test_connection.py
@@ -6,6 +6,7 @@ import pytest
 import aqt
 
 
+@pytest.mark.enable_socket
 @pytest.mark.remote_data
 def test_cli_unknown_version(capsys):
     wrong_version = "5.16.0"
diff --git a/tests/test_helper.py b/tests/test_helper.py
index b1b1a1c..fe7666e 100644
--- a/tests/test_helper.py
+++ b/tests/test_helper.py
@@ -6,21 +6,14 @@ from urllib.parse import urlparse
 
 import pytest
 import requests
-from pytest_socket import disable_socket
 from requests.models import Response
 
 from aqt import helper
-from aqt.exceptions import ArchiveConnectionError, ArchiveDownloadError
-from aqt.helper import getUrl
+from aqt.exceptions import ArchiveChecksumError, ArchiveConnectionError, ArchiveDownloadError
+from aqt.helper import getUrl, retry_on_errors
 from aqt.metadata import Version
 
 
-@pytest.fixture(autouse=True)
-def disable_sockets():
-    # This blocks all network connections, causing test failure if we used monkeypatch wrong
-    disable_socket()
-
-
 def test_helper_altlink(monkeypatch):
     class Message:
         headers = {"content-type": "text/plain", "length": 300}
@@ -129,17 +122,24 @@ def test_helper_downloadBinary_wrong_checksum(tmp_path, monkeypatch):
 
     actual_hash = binascii.unhexlify("1d41a93e4a585bb01e4518d4af431933")
     wrong_hash = binascii.unhexlify("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
-    expected_err = f"Download file is corrupted! Detect checksum error.\nExpected {wrong_hash}, Actual {actual_hash}"
-    out = tmp_path.joinpath("text.xml")
-    with pytest.raises(ArchiveDownloadError) as e:
-        helper.downloadBinaryFile("http://example.com/test.xml", out, "md5", wrong_hash, 60)
-    assert e.type == ArchiveDownloadError
+    out = tmp_path.joinpath("test.xml")
+    url = "http://example.com/test.xml"
+    expected_err = (
+        f"Downloaded file test.xml is corrupted! Detect checksum error."
+        f"\nExpect {wrong_hash.hex()}: {url}"
+        f"\nActual {actual_hash.hex()}: {out}"
+    )
+    with pytest.raises(ArchiveChecksumError) as e:
+        helper.downloadBinaryFile(url, str(out), "md5", wrong_hash, 60)
+    assert e.type == ArchiveChecksumError
     assert format(e.value) == expected_err
 
 
 def test_helper_downloadBinary_response_error_undefined(tmp_path, monkeypatch):
+    contained_error_msg = "This chunk of downloaded content contains an error."
+
     def iter_broken_content(*args, **kwargs):
-        raise RuntimeError("This chunk of downloaded content contains an error.")
+        raise RuntimeError(contained_error_msg)
 
     def mock_requests_get(*args, **kwargs):
         response = Response()
@@ -154,7 +154,33 @@ def test_helper_downloadBinary_response_error_undefined(tmp_path, monkeypatch):
     with pytest.raises(ArchiveDownloadError) as e:
         helper.downloadBinaryFile("http://example.com/test.xml", out, "md5", expected, 60)
     assert e.type == ArchiveDownloadError
-    assert format(e.value) == "Download error: This chunk of downloaded content contains an error."
+    assert format(e.value) == f"Download of test.xml has error: {contained_error_msg}"
+
+
+@pytest.mark.parametrize(
+    "num_attempts_before_success, num_retries_allowed",
+    (
+        (2, 5),
+        (5, 5),
+        (5, 2),
+    ),
+)
+def test_helper_retry_on_error(num_attempts_before_success, num_retries_allowed):
+    enclosed = {"call_count": 0}
+
+    def action():
+        enclosed["call_count"] += 1
+        more_attempts_needed = num_attempts_before_success - enclosed["call_count"]
+        if more_attempts_needed <= 0:
+            return True
+        raise RuntimeError(f"Must retry {more_attempts_needed} more times before success")
+
+    if num_attempts_before_success > num_retries_allowed:
+        with pytest.raises(RuntimeError) as e:
+            retry_on_errors(action, (RuntimeError,), num_retries_allowed, "do something")
+        assert e.type == RuntimeError
+    else:
+        assert retry_on_errors(action, (RuntimeError,), num_retries_allowed, "do something")
 
 
 @pytest.mark.parametrize(
diff --git a/tests/test_install.py b/tests/test_install.py
index a237457..9bd5eec 100644
--- a/tests/test_install.py
+++ b/tests/test_install.py
@@ -12,7 +12,6 @@ from typing import Any, Callable, Dict, Iterable, List, Optional, Tuple
 
 import py7zr
 import pytest
-from pytest_socket import disable_socket
 
 from aqt.archives import QtPackage
 from aqt.exceptions import ArchiveDownloadError, ArchiveExtractionError
@@ -155,10 +154,7 @@ def make_mock_geturl_download_archive(
 
 
 @pytest.fixture(autouse=True)
-def disable_sockets_and_multiprocessing(monkeypatch):
-    # This blocks all network connections, causing test failure if we used monkeypatch wrong
-    disable_socket()
-
+def disable_multiprocessing(monkeypatch):
     # This blocks all multiprocessing, which would otherwise spawn processes that are not monkeypatched
     monkeypatch.setattr(
         "aqt.installer.multiprocessing.get_context",
@@ -538,41 +534,45 @@ def test_install_nonexistent_archives(monkeypatch, capsys, cmd, xml_file: Option
 
 
 @pytest.mark.parametrize(
-    "make_exception, expect_end_msg, settings_file",
+    "make_exception, settings_file, expect_end_msg, expect_return",
     (
         (
             "RuntimeError()",
+            "../aqt/settings.ini",
             "===========================PLEASE FILE A BUG REPORT===========================\n"
             "You have discovered a bug in aqt.\n"
             "Please file a bug report at https://github.com/miurahr/aqtinstall/issues.\n"
             "Please remember to include a copy of this program's output in your report.",
-            "../aqt/settings.ini",
+            Cli.UNHANDLED_EXCEPTION_CODE,
         ),
         (
             "KeyboardInterrupt()",
-            "Caught KeyboardInterrupt, terminating installer workers\n" "Installer halted by keyboard interrupt.",
             "../aqt/settings.ini",
+            "Caught KeyboardInterrupt, terminating installer workers\nInstaller halted by keyboard interrupt.",
+            1,
         ),
         (
             "MemoryError()",
+            "../aqt/settings.ini",
             "Caught MemoryError, terminating installer workers\n"
             "Out of memory when downloading and extracting archives in parallel.\n"
             "==============================Suggested follow-up:==============================\n"
             "* Please reduce your 'concurrency' setting (see "
             "https://aqtinstall.readthedocs.io/en/stable/configuration.html#configuration)",
-            "../aqt/settings.ini",
+            1,
         ),
         (
             "MemoryError()",
+            "data/settings_no_concurrency.ini",
             "Caught MemoryError, terminating installer workers\n"
             "Out of memory when downloading and extracting archives.\n"
             "==============================Suggested follow-up:==============================\n"
             "* Please free up more memory.",
-            "data/settings_no_concurrency.ini",
+            1,
         ),
     ),
 )
-def test_install_pool_exception(monkeypatch, capsys, make_exception, expect_end_msg, settings_file):
+def test_install_pool_exception(monkeypatch, capsys, make_exception, settings_file, expect_end_msg, expect_return):
     def mock_installer_func(*args):
         raise eval(make_exception)
 
@@ -588,7 +588,7 @@ def test_install_pool_exception(monkeypatch, capsys, make_exception, expect_end_
 
     Settings.load_settings(str(Path(__file__).parent / settings_file))
     cli = Cli()
-    assert cli.run(cmd) == 1
+    assert expect_return == cli.run(cmd)
     out, err = capsys.readouterr()
     assert err.rstrip().endswith(expect_end_msg)
 
diff --git a/tests/test_updater.py b/tests/test_updater.py
index 0899fbc..6b67c45 100644
--- a/tests/test_updater.py
+++ b/tests/test_updater.py
@@ -2,7 +2,6 @@ import re
 from tempfile import TemporaryDirectory
 
 import pytest
-from pytest_socket import disable_socket
 
 from aqt.archives import TargetConfig
 from aqt.exceptions import UpdaterError
@@ -10,12 +9,6 @@ from aqt.helper import Settings
 from aqt.updater import Updater
 
 
-@pytest.fixture(autouse=True)
-def disable_sockets():
-    # This blocks all network connections, causing test failure if we used monkeypatch wrong
-    disable_socket()
-
-
 @pytest.fixture(autouse=True)
 def setup_settings():
     Settings.load_settings()