Merge branch 'fix-icon-check' into 'master'

fix: fix existing icon detection for repo and archive

See merge request fdroid/fdroidserver!1425

fdroidserver/common.py

@@ -710,6 +710,14 @@ def read_config():
         if not config['archive_url'].endswith('/archive'):
             raise FDroidException(_('archive_url needs to end with /archive'))
 
+    repo_icon = config.get('repo_icon', '')
+    if '/' in repo_icon:
+        raise FDroidException(
+            _('repo_icon should be a filename ({name}), not a path!').format(
+                name=os.path.basename(repo_icon)
+            )
+        )
+
     confignames_to_delete = set()
     for configname, dictvalue in config.items():
         if configname == 'java_paths':

fdroidserver/index.py

@@ -102,11 +102,10 @@ def make(apps, apks, repodir, archive):
     if common.config['repo_maxage'] != 0:
         repodict['maxage'] = common.config['repo_maxage']
 
+    repo_icon = common.config.get('repo_icon', common.default_config['repo_icon'])
     if archive:
         repodict['name'] = common.config['archive_name']
-        repodict['icon'] = common.config.get(
-            'archive_icon', common.default_config['repo_icon']
-        )
+        repodict['icon'] = repo_icon
         repodict['description'] = common.config['archive_description']
         archive_url = common.config.get(
             'archive_url', common.config['repo_url'][:-4] + 'archive'
@@ -117,9 +116,7 @@ def make(apps, apks, repodir, archive):
         repo_section = os.path.basename(urllib.parse.urlparse(archive_url).path)
     else:
         repodict['name'] = common.config['repo_name']
-        repodict['icon'] = common.config.get(
-            'repo_icon', common.default_config['repo_icon']
-        )
+        repodict['icon'] = repo_icon
         repodict['address'] = common.config['repo_url']
         if 'repo_web_base_url' in common.config:
             repodict["webBaseUrl"] = common.config['repo_web_base_url']
@@ -146,6 +143,7 @@ def make(apps, apks, repodir, archive):
     signer_fingerprints = load_publish_signer_fingerprints()
 
     make_v0(sortedapps, apks, repodir, repodict, requestsdict, signer_fingerprints)
+    copy_repo_icon(repodir)
     make_v1(sortedapps, apks, repodir, repodict, requestsdict, signer_fingerprints)
     make_v2(
         sortedapps, apks, repodir, repodict, requestsdict, signer_fingerprints, archive
@@ -1465,34 +1463,36 @@ def make_v0(apps, apks, repodir, repodict, requestsdict, signer_fingerprints):
             signindex.config = common.config
             signindex.sign_jar(signed, use_old_algs=True)
 
-    # Copy the repo icon into the repo directory...
+
+def copy_repo_icon(repodir):
     icon_dir = os.path.join(repodir, 'icons')
-    repo_icon = os.path.basename(
-        common.config.get('repo_icon', common.default_config['repo_icon'])
-    )
+    repo_icon = common.config.get('repo_icon', common.default_config['repo_icon'])
     icon_path = os.path.join(icon_dir, repo_icon)
     if not os.path.exists(icon_path):
-        logging.warning(
-            _(
-                'repo_icon "{repo_icon}" does not exist in "{icon_dir}", generating placeholder.'
-            ).format(repo_icon=repo_icon, icon_dir=icon_dir)
-        )
         os.makedirs(icon_dir, exist_ok=True)
-        try:
-            import qrcode
-
-            qrcode.make(common.config['repo_url']).save(icon_path)
-        except Exception as e:
-            if isinstance(e, ModuleNotFoundError):
-                logging.error(
-                    _(
-                        'The "qrcode" Python package is not installed (e.g. apt-get install python3-qrcode)!'
-                    )
-                )
-            exampleicon = os.path.join(
-                common.get_examples_dir(), common.default_config['repo_icon']
+        if os.path.exists(repo_icon):
+            shutil.copy(repo_icon, icon_path)
+        else:
+            logging.warning(
+                _(
+                    'repo_icon "{repo_icon}" does not exist in "{icon_dir}", generating placeholder.'
+                ).format(repo_icon=repo_icon, icon_dir=icon_dir)
             )
-            shutil.copy(exampleicon, icon_path)
+            try:
+                import qrcode
+
+                qrcode.make(common.config['repo_url']).save(icon_path)
+            except Exception as e:
+                if isinstance(e, ModuleNotFoundError):
+                    logging.error(
+                        _(
+                            'The "qrcode" Python package is not installed (e.g. apt-get install python3-qrcode)!'
+                        )
+                    )
+                exampleicon = os.path.join(
+                    common.get_examples_dir(), common.default_config['repo_icon']
+                )
+                shutil.copy(exampleicon, icon_path)
 
 
 def extract_pubkey():

fdroidserver/lint.py

@@ -1691,7 +1691,6 @@ CHECK_CONFIG_KEYS = (
     'apk_signing_key_block_list',
     'archive',
     'archive_description',
-    'archive_icon',
     'archive_name',
     'archive_older',
     'archive_url',

fdroidserver/update.py

@@ -2796,17 +2796,17 @@ def main():
         options.clean = True
 
     # check that icons exist now, rather than fail at the end of `fdroid update`
-    for k in ['repo_icon', 'archive_icon']:
-        if k in config:
-            icon_dir = os.path.join(k.split('_')[0], 'icons')
-            repo_icon = os.path.basename(common.config.get(k, ''))
-            icon_path = os.path.join(icon_dir, repo_icon)
-            if not os.path.exists(icon_path):
-                logging.warning(
-                    _(
-                        '{name} "{repo_icon}" does not exist in "{icon_dir}/"! Check "config.yml".'
-                    ).format(name=k, icon_dir=icon_dir, repo_icon=repo_icon)
-                )
+    icon_key = 'repo_icon'
+    if icon_key in config:
+        icon_dir = os.path.join(repodirs[0], 'icons')
+        repo_icon = os.path.basename(config.get(icon_key, ''))
+        icon_path = os.path.join(icon_dir, repo_icon)
+        if not os.path.exists(icon_path):
+            logging.warning(
+                _(
+                    '{name} "{repo_icon}" does not exist in "{icon_dir}/"! Check "config.yml".'
+                ).format(name=icon_key, icon_dir=icon_dir, repo_icon=repo_icon)
+            )
 
     # if the user asks to create a keystore, do it now, reusing whatever it can
     if options.create_key:

tests/test_common.py

@@ -2904,6 +2904,24 @@ class CommonTest(SetUpTearDownMixin, unittest.TestCase):
             fdroidserver.common.read_config()['serverwebroot'],
         )
 
+    def test_config_errors_ends_with_repo(self):
+        os.chdir(self.testdir)
+        fdroidserver.common.write_config_file("""repo_url: https://foo.com/fdroid""")
+        with self.assertRaises(FDroidException):
+            fdroidserver.common.read_config()
+
+    def test_config_errors_ends_with_archive(self):
+        os.chdir(self.testdir)
+        fdroidserver.common.write_config_file("""archive_url: https://foo.com/fdroid/repo""")
+        with self.assertRaises(FDroidException):
+            fdroidserver.common.read_config()
+
+    def test_config_errors_repo_icon(self):
+        os.chdir(self.testdir)
+        fdroidserver.common.write_config_file("""repo_icon: /path/to/icon.png""")
+        with self.assertRaises(FDroidException):
+            fdroidserver.common.read_config()
+
     @mock.patch.dict(os.environ, {'PATH': os.getenv('PATH')}, clear=True)
     def test_config_serverwebroot_list_of_dicts_env(self):
         os.chdir(self.testdir)

tests/test_index.py

@@ -414,6 +414,8 @@ class IndexTest(unittest.TestCase):
         requestsdict = {'install': [], 'uninstall': []}
         common.config['repo_pubkey'] = 'ffffffffffffffffffffffffffffffffff'
         index.make_v0({}, [], 'repo', repodict, requestsdict, {})
+        with self.assertLogs():
+            index.copy_repo_icon('repo')
         self.assertTrue(os.path.isdir(repo_icons_dir))
         self.assertTrue(
             os.path.exists(
@@ -422,6 +424,50 @@ class IndexTest(unittest.TestCase):
         )
         self.assertTrue(os.path.exists(os.path.join('repo', 'index.xml')))
 
+    def test_generate_repo_icon(self):
+        os.chdir(self.testdir)
+        os.mkdir('repo')
+        repo_icons_dir = os.path.join('repo', 'icons')
+        generated_path = os.path.join(
+            repo_icons_dir, common.default_config['repo_icon']
+        )
+        self.assertFalse(os.path.isdir(repo_icons_dir))
+        self.assertFalse(os.path.exists(generated_path))
+        with self.assertLogs():
+            index.copy_repo_icon('repo')
+        self.assertTrue(os.path.isdir(repo_icons_dir))
+        self.assertTrue(os.path.exists(generated_path))
+
+    def test_copy_repo_icon(self):
+        os.chdir(self.testdir)
+        os.mkdir('repo')
+        repo_icons_dir = Path('repo/icons')
+        self.assertFalse(os.path.isdir(repo_icons_dir))
+        common.config['repo_icon'] = 'test_icon.png'
+        test_value = 'test'
+        Path(common.config['repo_icon']).write_text(test_value)
+        copied_path = repo_icons_dir / os.path.basename(common.config['repo_icon'])
+        self.assertFalse(os.path.isdir(repo_icons_dir))
+        self.assertFalse(os.path.exists(copied_path))
+        index.copy_repo_icon('repo')
+        self.assertTrue(os.path.isdir(repo_icons_dir))
+        self.assertEqual(test_value, copied_path.read_text())
+
+    def test_copy_repo_icon_skipped_if_exists(self):
+        os.chdir(self.testdir)
+        test_value = 'test'
+        default_repo_icon = common.default_config['repo_icon']
+        repo_icon = Path('repo/icons') / default_repo_icon
+        repo_icon.parent.mkdir(parents=True)
+        repo_icon.write_text(test_value)
+        archive_icon = Path('archive/icons') / default_repo_icon
+        archive_icon.parent.mkdir(parents=True)
+        archive_icon.write_text(test_value)
+        Path(default_repo_icon).write_text('this should not be copied')
+        index.copy_repo_icon('repo')
+        self.assertEqual(test_value, repo_icon.read_text())
+        self.assertEqual(test_value, archive_icon.read_text())
+
     def test_make_v0(self):
         os.chdir(self.testdir)
         os.mkdir('metadata')
@@ -477,6 +523,8 @@ class IndexTest(unittest.TestCase):
         common.config['repo_pubkey'] = 'ffffffffffffffffffffffffffffffffff'
         common.config['make_current_version_link'] = True
         index.make_v0(apps, [apk], 'repo', repodict, requestsdict, {})
+        with self.assertLogs():
+            index.copy_repo_icon('repo')
         self.assertTrue(os.path.isdir(repo_icons_dir))
         self.assertTrue(
             os.path.exists(