diff --git a/fdroidserver/common.py b/fdroidserver/common.py index 38922cde..da5e72b0 100644 --- a/fdroidserver/common.py +++ b/fdroidserver/common.py @@ -486,14 +486,7 @@ def read_config(opts=None): fill_config_defaults(config) if 'serverwebroot' in config: - if isinstance(config['serverwebroot'], str): - roots = [{'url': config['serverwebroot']}] - elif all(isinstance(item, str) for item in config['serverwebroot']): - roots = [{'url': i} for i in config['serverwebroot']] - elif all(isinstance(item, dict) for item in config['serverwebroot']): - roots = config['serverwebroot'] - else: - raise TypeError(_('only accepts strings, lists, and tuples')) + roots = parse_mirrors_config(config['serverwebroot']) rootlist = [] for d in roots: # since this is used with rsync, where trailing slashes have @@ -506,15 +499,7 @@ def read_config(opts=None): config['serverwebroot'] = rootlist if 'servergitmirrors' in config: - if isinstance(config['servergitmirrors'], str): - roots = [{"url": config['servergitmirrors']}] - elif all(isinstance(item, str) for item in config['servergitmirrors']): - roots = [{'url': i} for i in config['servergitmirrors']] - elif all(isinstance(item, dict) for item in config['servergitmirrors']): - roots = config['servergitmirrors'] - else: - raise TypeError(_('only accepts strings, lists, and tuples')) - config['servergitmirrors'] = roots + config['servergitmirrors'] = parse_mirrors_config(config['servergitmirrors']) limit = config['git_mirror_size_limit'] config['git_mirror_size_limit'] = parse_human_readable_size(limit) @@ -557,6 +542,18 @@ def read_config(opts=None): return config +def parse_mirrors_config(mirrors): + """Mirrors can be specified as a string, list of strings, or dictionary map.""" + if isinstance(mirrors, str): + return [{"url": mirrors}] + elif all(isinstance(item, str) for item in mirrors): + return [{'url': i} for i in mirrors] + elif all(isinstance(item, dict) for item in mirrors): + return mirrors + else: + raise TypeError(_('only accepts strings, lists, and tuples')) + + def file_entry(filename, hash_value=None): meta = {} meta["name"] = "/" + filename.split("/", 1)[1] diff --git a/fdroidserver/install.py b/fdroidserver/install.py index 79b0a924..9ef82da0 100644 --- a/fdroidserver/install.py +++ b/fdroidserver/install.py @@ -47,17 +47,31 @@ def main(): global options, config # Parse command line... - parser = ArgumentParser(usage="%(prog)s [options] [APPID[:VERCODE] [APPID[:VERCODE] ...]]") + parser = ArgumentParser( + usage="%(prog)s [options] [APPID[:VERCODE] [APPID[:VERCODE] ...]]" + ) common.setup_global_opts(parser) - parser.add_argument("appid", nargs='*', help=_("application ID with optional versionCode in the form APPID[:VERCODE]")) - parser.add_argument("-a", "--all", action="store_true", default=False, - help=_("Install all signed applications available")) + parser.add_argument( + "appid", + nargs='*', + help=_("application ID with optional versionCode in the form APPID[:VERCODE]"), + ) + parser.add_argument( + "-a", + "--all", + action="store_true", + default=False, + help=_("Install all signed applications available"), + ) options = parser.parse_args() common.set_console_logging(options.verbose) if not options.appid and not options.all: - parser.error(_("option %s: If you really want to install all the signed apps, use --all") % "all") + parser.error( + _("option %s: If you really want to install all the signed apps, use --all") + % "all" + ) config = common.read_config(options) @@ -67,14 +81,12 @@ def main(): sys.exit(0) if options.appid: - vercodes = common.read_pkg_args(options.appid, True) - common.get_metadata_files(vercodes) # only check appids + common.get_metadata_files(vercodes) # only check appids apks = {appid: None for appid in vercodes} # Get the signed APK with the highest vercode for apkfile in sorted(glob.glob(os.path.join(output_dir, '*.apk'))): - try: appid, vercode = common.publishednameinfo(apkfile) except FDroidException: @@ -90,9 +102,10 @@ def main(): raise FDroidException(_("No signed APK available for %s") % appid) else: - - apks = {common.publishednameinfo(apkfile)[0]: apkfile for apkfile in - sorted(glob.glob(os.path.join(output_dir, '*.apk')))} + apks = { + common.publishednameinfo(apkfile)[0]: apkfile + for apkfile in sorted(glob.glob(os.path.join(output_dir, '*.apk'))) + } for appid, apk in apks.items(): # Get device list each time to avoid device not found errors @@ -101,7 +114,11 @@ def main(): raise FDroidException(_("No attached devices found")) logging.info(_("Installing %s...") % apk) for dev in devs: - logging.info(_("Installing '{apkfilename}' on {dev}...").format(apkfilename=apk, dev=dev)) + logging.info( + _("Installing '{apkfilename}' on {dev}...").format( + apkfilename=apk, dev=dev + ) + ) p = SdkToolsPopen(['adb', "-s", dev, "install", apk]) fail = "" for line in p.output.splitlines(): @@ -111,11 +128,17 @@ def main(): continue if fail == "INSTALL_FAILED_ALREADY_EXISTS": - logging.warning(_('"{apkfilename}" is already installed on {dev}.') - .format(apkfilename=apk, dev=dev)) + logging.warning( + _('"{apkfilename}" is already installed on {dev}.').format( + apkfilename=apk, dev=dev + ) + ) else: - raise FDroidException(_("Failed to install '{apkfilename}' on {dev}: {error}") - .format(apkfilename=apk, dev=dev, error=fail)) + raise FDroidException( + _("Failed to install '{apkfilename}' on {dev}: {error}").format( + apkfilename=apk, dev=dev, error=fail + ) + ) logging.info('\n' + _('Finished')) diff --git a/fdroidserver/net.py b/fdroidserver/net.py index cf01395a..49d67f2c 100644 --- a/fdroidserver/net.py +++ b/fdroidserver/net.py @@ -29,12 +29,20 @@ HEADERS = {'User-Agent': 'F-Droid'} def download_file(url, local_filename=None, dldir='tmp', retries=3, backoff_factor=0.1): + """Try hard to download the file, including retrying on failures. + + This has two retry cycles, one inside of the requests session, the + other provided by this function. The requests retry logic applies + to failed DNS lookups, socket connections and connection timeouts, + never to requests where data has made it to the server. This + handles ChunkedEncodingError during transfer in its own retry + loop. This can result in more retries than are specified in the + retries parameter. + + """ filename = urllib.parse.urlparse(url).path.split('/')[-1] if local_filename is None: local_filename = os.path.join(dldir, filename) - # Retry applies to failed DNS lookups, socket connections and connection - # timeouts, never to requests where data has made it to the server; so we - # handle ChunkedEncodingError during transfer ourselves. for i in range(retries + 1): if retries: max_retries = Retry(total=retries - i, backoff_factor=backoff_factor) diff --git a/pyproject.toml b/pyproject.toml index 9694d7be..a4b7ddbe 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -41,7 +41,6 @@ force-exclude = '''( | fdroidserver/deploy\.py | fdroidserver/import_subcommand\.py | fdroidserver/index\.py - | fdroidserver/install\.py | fdroidserver/metadata\.py | fdroidserver/nightly\.py | fdroidserver/publish\.py @@ -53,7 +52,6 @@ force-exclude = '''( | tests/extra/manual-vmtools-test\.py | tests/gradle-release-checksums\.py | tests/openssl-version-check-test\.py - | tests/testcommon\.py | tests/valid-package-names/test\.py | tests/checkupdates\.TestCase | tests/common\.TestCase diff --git a/tests/common.TestCase b/tests/common.TestCase index 55202dcc..a92dc92a 100755 --- a/tests/common.TestCase +++ b/tests/common.TestCase @@ -12,6 +12,7 @@ import logging import optparse import os import re +import ruamel.yaml import shutil import subprocess import sys @@ -2892,6 +2893,27 @@ class CommonTest(unittest.TestCase): fdroidserver.common.read_config()['serverwebroot'], ) + def test_parse_mirrors_config_str(self): + s = 'foo@example.com:/var/www' + mirrors = ruamel.yaml.YAML(typ='safe').load("""'%s'""" % s) + self.assertEqual( + [{'url': s}], fdroidserver.common.parse_mirrors_config(mirrors) + ) + + def test_parse_mirrors_config_list(self): + s = 'foo@example.com:/var/www' + mirrors = ruamel.yaml.YAML(typ='safe').load("""- '%s'""" % s) + self.assertEqual( + [{'url': s}], fdroidserver.common.parse_mirrors_config(mirrors) + ) + + def test_parse_mirrors_config_dict(self): + s = 'foo@example.com:/var/www' + mirrors = ruamel.yaml.YAML(typ='safe').load("""- url: '%s'""" % s) + self.assertEqual( + [{'url': s}], fdroidserver.common.parse_mirrors_config(mirrors) + ) + if __name__ == "__main__": os.chdir(os.path.dirname(__file__)) diff --git a/tests/net.TestCase b/tests/net.TestCase index a53446bc..94b8ce84 100755 --- a/tests/net.TestCase +++ b/tests/net.TestCase @@ -4,8 +4,13 @@ import inspect import logging import optparse import os +import random +import requests +import socket import sys import tempfile +import threading +import time import unittest from unittest.mock import MagicMock, patch @@ -20,6 +25,57 @@ from fdroidserver import common, net from pathlib import Path +class RetryServer: + """A stupid simple HTTP server that can fail to connect""" + + def __init__(self, port=None, failures=3): + self.port = port + if self.port is None: + self.port = random.randint(1024, 65535) + self.failures = failures + self.stop_event = threading.Event() + threading.Thread(target=self.run_fake_server).start() + + def stop(self): + self.stop_event.set() + + def run_fake_server(self): + server_sock = socket.socket() + server_sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + server_sock.bind(('127.0.0.1', self.port)) + server_sock.listen(5) + server_sock.settimeout(5) + time.sleep(0.001) # wait for it to start + + while not self.stop_event.is_set(): + self.failures -= 1 + conn = None + try: + conn, address = server_sock.accept() + conn.settimeout(5) + except TimeoutError: + break + if self.failures > 0: + conn.close() + continue + conn.recv(8192) # request ignored + self.reply = b"""HTTP/1.1 200 OK + Date: Mon, 26 Feb 2024 09:00:14 GMT + Connection: close + Content-Type: text/html + + Hello World! + """ + self.reply = self.reply.replace(b' ', b'') # dedent + conn.sendall(self.reply) + conn.shutdown(socket.SHUT_RDWR) + conn.close() + + self.stop_event.wait(timeout=1) + server_sock.shutdown(socket.SHUT_RDWR) + server_sock.close() + + class NetTest(unittest.TestCase): basedir = Path(__file__).resolve().parent @@ -39,10 +95,10 @@ class NetTest(unittest.TestCase): return MagicMock() requests_get.side_effect = _get - f = net.download_file('https://f-droid.org/repo/index-v1.jar', retries=0) - self.assertTrue(requests_get.called) + f = net.download_file('https://f-droid.org/repo/entry.jar', retries=0) + requests_get.assert_called() self.assertTrue(os.path.exists(f)) - self.assertEqual('tmp/index-v1.jar', f) + self.assertEqual('tmp/entry.jar', f) f = net.download_file( 'https://d-05.example.com/custom/com.downloader.aegis-3175421.apk?_fn=QVBLUHVyZV92My4xNy41NF9hcGtwdXJlLmNvbS5hcGs&_p=Y29tLmFwa3B1cmUuYWVnb24&am=6avvTpfJ1dMl9-K6JYKzQw&arg=downloader%3A%2F%2Fcampaign%2F%3Futm_medium%3Ddownloader%26utm_source%3Daegis&at=1652080635&k=1f6e58465df3a441665e585719ab0b13627a117f&r=https%3A%2F%2Fdownloader.com%2Fdownloader-app.html%3Ficn%3Daegis%26ici%3Dimage_qr&uu=http%3A%2F%2F172.16.82.1%2Fcustom%2Fcom.downloader.aegis-3175421.apk%3Fk%3D3fb9c4ae0be578206f6a1c330736fac1627a117f', @@ -52,6 +108,20 @@ class NetTest(unittest.TestCase): self.assertTrue(os.path.exists(f)) self.assertEqual('tmp/com.downloader.aegis-3175421.apk', f) + def test_download_file_retries(self): + server = RetryServer() + f = net.download_file('http://localhost:%d/f.txt' % server.port) + # strip the HTTP headers and compare the reply + self.assertEqual(server.reply.split(b'\n\n')[1], Path(f).read_bytes()) + server.stop() + + def test_download_file_retries_not_forever(self): + """The retry logic should eventually exit with an error.""" + server = RetryServer(failures=5) + with self.assertRaises(requests.exceptions.ConnectionError): + net.download_file('http://localhost:%d/f.txt' % server.port) + server.stop() + if __name__ == "__main__": os.chdir(os.path.dirname(__file__)) diff --git a/tests/testcommon.py b/tests/testcommon.py index 61c1a904..fe3728ba 100644 --- a/tests/testcommon.py +++ b/tests/testcommon.py @@ -20,10 +20,8 @@ import sys import tempfile -class TmpCwd(): - """Context-manager for temporarily changing the current working - directory. - """ +class TmpCwd: + """Context-manager for temporarily changing the current working directory.""" def __init__(self, new_cwd): self.new_cwd = new_cwd @@ -36,9 +34,8 @@ class TmpCwd(): os.chdir(self.orig_cwd) -class TmpPyPath(): - """Context-manager for temporarily adding a direcory to python path - """ +class TmpPyPath: + """Context-manager for temporarily adding a directory to Python path.""" def __init__(self, additional_path): self.additional_path = additional_path @@ -51,14 +48,11 @@ class TmpPyPath(): def mock_open_to_str(mock): - """ - helper function for accessing all data written into a - unittest.mock.mock_open() instance as a string. - """ + """For accessing all data written into a unittest.mock.mock_open() instance as a string.""" - return "".join([ - x.args[0] for x in mock.mock_calls if str(x).startswith("call().write(") - ]) + return "".join( + [x.args[0] for x in mock.mock_calls if str(x).startswith("call().write(")] + ) def mkdtemp():