Merge branch 'keep-failed-build-in-test-mode' into 'master'

`fdroid build --test` keeps unsigned APKs in tmp/ that fail to reproduce

See merge request fdroid/fdroidserver!904

.gitlab-ci.yml

@@ -5,6 +5,7 @@ variables:
   GIT_DEPTH: 1
 
 
+# Run the whole test suite in an environment that is like the buildserver guest VM.
 ci-images-base run-tests:
   image: registry.gitlab.com/fdroid/ci-images-base
   script:
@@ -67,6 +68,11 @@ metadata_v0:
     - apt-get update
     - apt-get dist-upgrade
 
+
+# Since F-Droid uses Debian as its default platform, from production
+# servers to CI to contributor machines, it is important to know when
+# changes in Debian break our stuff.  This tests against the latest
+# dependencies as they are included in Debian.
 debian_testing:
   image: debian:testing
   <<: *apt-template
@@ -88,6 +94,7 @@ debian_testing:
     - cd tests
     - ./run-tests
 
+
 # Test using latest LTS set up with the PPA, including Recommends.
 ubuntu_lts_ppa:
   image: ubuntu:latest
@@ -106,6 +113,7 @@ ubuntu_lts_ppa:
     - cd tests
     - ./run-tests
 
+
 # Test using Ubuntu/bionic LTS (supported til 2022) with all depends
 # from pypi.  The venv is used to isolate the dist tarball generation
 # environment from the clean install environment.
@@ -131,6 +139,7 @@ ubuntu_bionic_pip:
     - cd fdroidserver-*
     - fdroid=`which fdroid` ./tests/run-tests
 
+
 # test install process on a bleeding edge distro with pip
 arch_pip_install:
   image: archlinux/base
@@ -144,6 +153,8 @@ arch_pip_install:
     - fdroid update --help
 
 
+# The gradlew-fdroid tests are isolated from the rest of the test
+# suite, so they run as their own job.
 gradlew-fdroid:
   image: debian:bullseye
   <<: *apt-template
@@ -157,6 +168,7 @@ gradlew-fdroid:
     - ./tests/test-gradlew-fdroid
 
 
+# Run all the various linters and static analysis tools.
 lint_format_safety_bandit_checks:
   image: alpine:3.10 # cannot upgrade until bandit supports Python 3.8
   variables:
@@ -188,6 +200,7 @@ lint_format_safety_bandit_checks:
     - pybabel compile --domain=fdroidserver --directory locale 2>&1 | (grep -F "error:" && exit 1) || true
     - exit $EXITVALUE
 
+
 lint_mypy:
   image: python:3.9-buster
   script:
@@ -196,6 +209,7 @@ lint_mypy:
     # exclude vendored file
     - mypy --exclude fdroidserver/apksigcopier.py
 
+
 fedora_latest:
   image: fedora:latest
   only:
@@ -246,6 +260,7 @@ fedora_latest:
     - su testuser --login --command
         "cd `pwd`; export ANDROID_HOME=$ANDROID_HOME; fdroid=~testuser/.local/bin/fdroid ./run-tests"
 
+
 gradle:
   image: debian:bullseye
   <<: *apt-template
@@ -272,6 +287,8 @@ gradle:
       fi
     - ./tests/gradle-release-checksums.py
 
+
+# Run an actual build in a simple, faked version of the buildserver guest VM.
 fdroid build:
   image: registry.gitlab.com/fdroid/ci-images-client
   only:
@@ -318,6 +335,10 @@ fdroid build:
     - fdroid build --verbose --on-server --no-tarball --latest org.fdroid.fdroid
 
 
+# test the plugin API and specifically the fetchsrclibs plugin, which
+# is used by the `fdroid build` job.  This uses a fixed commit from
+# fdroiddata because that one is known to work, and this is a CI job,
+# so it should be isolated from the normal churn of fdroiddata.
 plugin_fetchsrclibs:
   image: debian:buster
   <<: *apt-template

fdroidserver/build.py

@@ -1126,7 +1126,7 @@ def main():
                         url = url.replace('%v', build.versionName)
                         url = url.replace('%c', str(build.versionCode))
                         logging.info("...retrieving " + url)
-                        of = re.sub(r'.apk$', '.binary.apk', common.get_release_filename(app, build))
+                        of = re.sub(r'\.apk$', '.binary.apk', common.get_release_filename(app, build))
                         of = os.path.join(binaries_dir, of)
                         try:
                             net.download_file(url, local_filename=of)
@@ -1146,8 +1146,12 @@ def main():
                             compare_result = \
                                 common.verify_apks(of, unsigned_apk, tmpdir)
                             if compare_result:
-                                logging.debug('removing %s', unsigned_apk)
-                                os.remove(unsigned_apk)
+                                if options.test:
+                                    logging.warning(_('Keeping failed build "{apkfilename}"')
+                                                    .format(apkfilename=unsigned_apk))
+                                else:
+                                    logging.debug('removing %s', unsigned_apk)
+                                    os.remove(unsigned_apk)
                                 logging.debug('removing %s', of)
                                 os.remove(of)
                                 compare_result = compare_result.split('\n')

tests/build.TestCase

@@ -11,6 +11,7 @@ import sys
 import tempfile
 import textwrap
 import unittest
+import yaml
 from unittest import mock
 
 localmodule = os.path.realpath(
@@ -40,6 +41,11 @@ class BuildTest(unittest.TestCase):
         fdroidserver.common.config = None
         fdroidserver.build.config = None
 
+    def create_fake_android_home(self, d):
+        os.makedirs(os.path.join(d, 'build-tools'), exist_ok=True)
+        os.makedirs(os.path.join(d, 'platform-tools'), exist_ok=True)
+        os.makedirs(os.path.join(d, 'tools'), exist_ok=True)
+
     def test_get_apk_metadata(self):
         config = dict()
         fdroidserver.common.fill_config_defaults(config)
@@ -206,6 +212,102 @@ class BuildTest(unittest.TestCase):
         count = fdroidserver.scanner.scan_source("build", build)
         self.assertEqual(0, count, "Shouldn't error on jar from extlib")
 
+    def test_failed_verifies_are_not_in_unsigned(self):
+
+        class FakeProcess:
+            output = 'fake output'
+            returncode = 0
+
+            def __init__(self, args, **kwargs):
+                print('FakeFDroidPopen', args, kwargs)
+
+        testdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir)
+        os.chdir(testdir)
+        sdk_path = os.path.join(testdir, 'android-sdk')
+        self.create_fake_android_home(sdk_path)
+        with open('config.yml', 'w') as fp:
+            yaml.dump({'sdk_path': sdk_path}, fp)
+        os.chmod('config.yml', 0o600)
+        fdroidserver.common.build = fdroidserver.common.read_config()
+
+        os.mkdir('metadata')
+        appid = 'info.guardianproject.checkey'
+        metadata_file = os.path.join('metadata', appid + '.yml')
+        shutil.copy(os.path.join(self.basedir, metadata_file),
+                    'metadata')
+        with open(metadata_file) as fp:
+            app = fdroidserver.metadata.App(yaml.safe_load(fp))
+        app['RepoType'] = 'git'
+        app['Binaries'] = 'https://example.com/fdroid/repo/info.guardianproject.checkey_%v.apk'
+        build = fdroidserver.metadata.Build({
+            'versionCode': 123,
+            'versionName': '1.2.3',
+            'commit': '1.2.3',
+            'disable': False,
+        })
+        app['Builds'] = [build]
+        fdroidserver.metadata.write_metadata(metadata_file, app)
+
+        os.makedirs(os.path.join('unsigned', 'binaries'))
+        production_result = os.path.join('unsigned', '%s_%d.apk' % (appid, build['versionCode']))
+        production_compare_file = os.path.join('unsigned', 'binaries',
+                                               '%s_%d.binary.apk' % (appid, build['versionCode']))
+        os.makedirs(os.path.join('tmp', 'binaries'))
+        test_result = os.path.join('tmp', '%s_%d.apk' % (appid, build['versionCode']))
+        test_compare_file = os.path.join(
+            'tmp', 'binaries', '%s_%d.binary.apk' % (appid, build['versionCode'])
+        )
+        with mock.patch(
+            'fdroidserver.common.force_exit', lambda *args: None
+        ) as a, mock.patch(
+            'fdroidserver.common.get_android_tools_version_log', lambda s: 'fake'
+        ) as b, mock.patch(
+            'fdroidserver.common.FDroidPopen', FakeProcess
+        ) as c, mock.patch(
+            'fdroidserver.build.FDroidPopen', FakeProcess
+        ) as d, mock.patch(
+            'fdroidserver.build.trybuild', lambda *args: True
+        ) as e, mock.patch(
+            'fdroidserver.net.download_file', lambda *args, **kwargs: None
+        ) as f:
+            a, b, c, d, e, f  # silence linters' "unused" warnings
+
+            with mock.patch('sys.argv', ['fdroid build', appid]):
+                # successful comparison
+                open(production_result, 'w').close()
+                open(production_compare_file, 'w').close()
+                with mock.patch('fdroidserver.common.verify_apks', lambda *args: None):
+                    fdroidserver.build.main()
+                self.assertTrue(os.path.exists(production_result))
+                self.assertTrue(os.path.exists(production_compare_file))
+                # failed comparison
+                open(production_result, 'w').close()
+                open(production_compare_file, 'w').close()
+                with mock.patch('fdroidserver.common.verify_apks', lambda *args: 'failed'):
+                    fdroidserver.build.main()
+                self.assertFalse(os.path.exists(production_result))
+                self.assertFalse(os.path.exists(production_compare_file))
+
+            with mock.patch('sys.argv', ['fdroid build', '--test', appid]):
+                # successful comparison
+                open(test_result, 'w').close()
+                open(test_compare_file, 'w').close()
+                with mock.patch('fdroidserver.common.verify_apks', lambda *args: None):
+                    fdroidserver.build.main()
+                self.assertTrue(os.path.exists(test_result))
+                self.assertTrue(os.path.exists(test_compare_file))
+                self.assertFalse(os.path.exists(production_result))
+                self.assertFalse(os.path.exists(production_compare_file))
+                # failed comparison
+                open(test_result, 'w').close()
+                open(test_compare_file, 'w').close()
+                with mock.patch('fdroidserver.common.verify_apks', lambda *args: 'failed'):
+                    fdroidserver.build.main()
+                self.assertTrue(os.path.exists(test_result))
+                self.assertFalse(os.path.exists(test_compare_file))
+                self.assertFalse(os.path.exists(production_result))
+                self.assertFalse(os.path.exists(production_compare_file))
+
 
 if __name__ == "__main__":
     os.chdir(os.path.dirname(__file__))

tests/common.TestCase

@@ -1129,6 +1129,7 @@ class CommonTest(unittest.TestCase):
                          fdroidserver.common.parse_androidmanifests(paths, app))
 
     def test_get_all_gradle_and_manifests(self):
+        """Test whether the function works with relative and absolute paths"""
         a = fdroidserver.common.get_all_gradle_and_manifests(os.path.join('source-files', 'cn.wildfirechat.chat'))
         paths = [
             os.path.join('source-files', 'cn.wildfirechat.chat', 'avenginekit', 'build.gradle'),
@@ -1145,6 +1146,11 @@ class CommonTest(unittest.TestCase):
         ]
         self.assertEqual(sorted(paths), sorted(a))
 
+        abspath = os.path.join(self.basedir, 'source-files', 'realm')
+        p = fdroidserver.common.get_all_gradle_and_manifests(abspath)
+        self.assertEqual(1, len(p))
+        self.assertTrue(p[0].startswith(abspath))
+
     def test_get_gradle_subdir(self):
         subdirs = {
             'cn.wildfirechat.chat': 'chat',