Repository: miurahr/aqtinstall
Pull request #420 (2021-10-03 23:40:29)

Allow retry of download on checksum error
This allows downloads of archives to be retried when checksum errors occur, as discussed in https://github.com/miurahr/aqtinstall/issues/416#issuecomment-932472110. Even after I [turned off my VPN](https://github.com/miurahr/aqtinstall/issues/416#issuecomment-933031438), I am still getting so many checksum errors that it takes me several tries before I can successfully install Qt. If any of our users are having similar trouble, I'm sure that they are extremely frustrated with `aqt`, and I think that this change could be helpful for them.

I'm not certain that this is the best implementation of this feature; please see my notes below.


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..9c0e60b 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
 
@@ -895,14 +896,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/test_helper.py b/tests/test_helper.py
index b1b1a1c..158a760 100644
--- a/tests/test_helper.py
+++ b/tests/test_helper.py
@@ -10,8 +10,8 @@ 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
 
 
@@ -129,17 +129,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 +161,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(