From 01c060704f4852d60a3889ccc273c43ed6e3354c Mon Sep 17 00:00:00 2001
From: Alexander Taylor <alexanderjohntaylor@gmail.com>
Date: Sun, 29 Sep 2019 15:15:49 +0100
Subject: [PATCH] Made p4a use per-arch dist build dirs (#1986)

* Made p4a use per-arch dist build dirs

* Fixed up check on existing folder when overwriting distributions

* Improved Distribution.get_distribution api

* Fixed test_archs.py

* Fixed test_bootstrap.py

* Fixed test_distribution.py

* Fixed recipes tests

* Fixed formatting

* Made sure whole build process accesses dist_dir correctly

* Fixed apk output name formatting by moving version to end of string

* Further test fixes following dist_dir changes

* Removed unnecessary variable
---
 pythonforandroid/bootstrap.py    | 16 +++---
 pythonforandroid/build.py        | 11 ++--
 pythonforandroid/distribution.py | 95 ++++++++++++++++++++++++--------
 pythonforandroid/python.py       |  7 ++-
 pythonforandroid/toolchain.py    | 16 ++++--
 tests/recipes/recipe_ctx.py      |  6 +-
 tests/test_archs.py              |  7 ++-
 tests/test_bootstrap.py          | 22 +++++---
 tests/test_distribution.py       | 20 +++++--
 9 files changed, 140 insertions(+), 60 deletions(-)

diff --git a/pythonforandroid/bootstrap.py b/pythonforandroid/bootstrap.py
index cd11c6e1a9..03925a3e01 100755
--- a/pythonforandroid/bootstrap.py
+++ b/pythonforandroid/bootstrap.py
@@ -8,10 +8,9 @@
 import shlex
 import shutil
 
-from pythonforandroid.logger import (warning, shprint, info, logger,
-                                     debug)
-from pythonforandroid.util import (current_directory, ensure_dir,
-                                   temp_directory)
+from pythonforandroid.logger import (shprint, info, logger, debug)
+from pythonforandroid.util import (
+    current_directory, ensure_dir, temp_directory, BuildInterruptingException)
 from pythonforandroid.recipe import Recipe
 
 
@@ -75,7 +74,6 @@ class Bootstrap(object):
     bootstrap_dir = None
 
     build_dir = None
-    dist_dir = None
     dist_name = None
     distribution = None
 
@@ -97,9 +95,9 @@ class Bootstrap(object):
     def dist_dir(self):
         '''The dist dir at which to place the finished distribution.'''
         if self.distribution is None:
-            warning('Tried to access {}.dist_dir, but {}.distribution '
-                    'is None'.format(self, self))
-            exit(1)
+            raise BuildInterruptingException(
+                'Internal error: tried to access {}.dist_dir, but {}.distribution '
+                'is None'.format(self, self))
         return self.distribution.dist_dir
 
     @property
@@ -158,7 +156,7 @@ def prepare_build_dir(self):
             with open('project.properties', 'w') as fileh:
                 fileh.write('target=android-{}'.format(self.ctx.android_api))
 
-    def prepare_dist_dir(self, name):
+    def prepare_dist_dir(self):
         ensure_dir(self.dist_dir)
 
     def run_distribute(self):
diff --git a/pythonforandroid/build.py b/pythonforandroid/build.py
index cc6cea1406..8f0ac40a82 100644
--- a/pythonforandroid/build.py
+++ b/pythonforandroid/build.py
@@ -92,8 +92,13 @@ class Context(object):
     # in which bootstraps are copied for building
     # and recipes are built
     build_dir = None
+
+    distribution = None
+    """The Distribution object representing the current build target location."""
+
     # the Android project folder where everything ends up
     dist_dir = None
+
     # where Android libs are cached after build
     # but before being placed in dists
     libs_dir = None
@@ -106,7 +111,6 @@ class Context(object):
 
     ndk_platform = None  # the ndk platform directory
 
-    dist_name = None  # should be deprecated in favour of self.dist.dist_name
     bootstrap = None
     bootstrap_build_dir = None
 
@@ -485,9 +489,8 @@ def prepare_bootstrap(self, bs):
         self.bootstrap.prepare_build_dir()
         self.bootstrap_build_dir = self.bootstrap.build_dir
 
-    def prepare_dist(self, name):
-        self.dist_name = name
-        self.bootstrap.prepare_dist_dir(self.dist_name)
+    def prepare_dist(self):
+        self.bootstrap.prepare_dist_dir()
 
     def get_site_packages_dir(self, arch=None):
         '''Returns the location of site-packages in the python-install build
diff --git a/pythonforandroid/distribution.py b/pythonforandroid/distribution.py
index f088ac01e6..379cd90852 100644
--- a/pythonforandroid/distribution.py
+++ b/pythonforandroid/distribution.py
@@ -24,7 +24,7 @@ class Distribution(object):
     ndk_api = None
 
     archs = []
-    '''The arch targets that the dist is built for.'''
+    '''The names of the arch targets that the dist is built for.'''
 
     recipes = []
 
@@ -42,12 +42,19 @@ def __repr__(self):
         return str(self)
 
     @classmethod
-    def get_distribution(cls, ctx, name=None, recipes=[],
-                         ndk_api=None,
-                         force_build=False,
-                         extra_dist_dirs=[],
-                         require_perfect_match=False,
-                         allow_replace_dist=True):
+    def get_distribution(
+            cls,
+            ctx,
+            *,
+            arch_name,  # required keyword argument: there is no sensible default
+            name=None,
+            recipes=[],
+            ndk_api=None,
+            force_build=False,
+            extra_dist_dirs=[],
+            require_perfect_match=False,
+            allow_replace_dist=True
+    ):
         '''Takes information about the distribution, and decides what kind of
         distribution it will be.
 
@@ -60,6 +67,12 @@ def get_distribution(cls, ctx, name=None, recipes=[],
         name : str
             The name of the distribution. If a dist with this name already '
             exists, it will be used.
+        ndk_api : int
+            The NDK API to compile against, included in the dist because it cannot
+            be changed later during APK packaging.
+        arch_name : str
+            The target architecture name to compile against, included in the dist because
+            it cannot be changed later during APK packaging.
         recipes : list
             The recipes that the distribution must contain.
         force_download: bool
@@ -77,17 +90,24 @@ def get_distribution(cls, ctx, name=None, recipes=[],
             a new one with the current requirements.
         '''
 
-        existing_dists = Distribution.get_distributions(ctx)
+        possible_dists = Distribution.get_distributions(ctx)
 
-        possible_dists = existing_dists
+        # Will hold dists that would be built in the same folder as an existing dist
+        folder_match_dist = None
 
-        name_match_dist = None
-
-        # 0) Check if a dist with that name already exists
+        # 0) Check if a dist with that name and architecture already exists
         if name is not None and name:
-            possible_dists = [d for d in possible_dists if d.name == name]
+            possible_dists = [
+                d for d in possible_dists if
+                (d.name == name) and (arch_name in d.archs)]
+
             if possible_dists:
-                name_match_dist = possible_dists[0]
+                # There should only be one folder with a given dist name *and* arch.
+                # We could check that here, but for compatibility let's let it slide
+                # and just record the details of one of them. We only use this data to
+                # possibly fail the build later, so it doesn't really matter if there
+                # was more than one clash.
+                folder_match_dist = possible_dists[0]
 
         # 1) Check if any existing dists meet the requirements
         _possible_dists = []
@@ -110,12 +130,14 @@ def get_distribution(cls, ctx, name=None, recipes=[],
         else:
             info('No existing dists meet the given requirements!')
 
-        # If any dist has perfect recipes and ndk API, return it
+        # If any dist has perfect recipes, arch and NDK API, return it
         for dist in possible_dists:
             if force_build:
                 continue
             if ndk_api is not None and dist.ndk_api != ndk_api:
                 continue
+            if arch_name is not None and arch_name not in dist.archs:
+                continue
             if (set(dist.recipes) == set(recipes) or
                 (set(recipes).issubset(set(dist.recipes)) and
                  not require_perfect_match)):
@@ -123,12 +145,10 @@ def get_distribution(cls, ctx, name=None, recipes=[],
                             .format(dist.name))
                 return dist
 
-        assert len(possible_dists) < 2
-
         # If there was a name match but we didn't already choose it,
         # then the existing dist is incompatible with the requested
         # configuration and the build cannot continue
-        if name_match_dist is not None and not allow_replace_dist:
+        if folder_match_dist is not None and not allow_replace_dist:
             raise BuildInterruptingException(
                 'Asked for dist with name {name} with recipes ({req_recipes}) and '
                 'NDK API {req_ndk_api}, but a dist '
@@ -136,9 +156,11 @@ def get_distribution(cls, ctx, name=None, recipes=[],
                 '({dist_recipes}) or NDK API {dist_ndk_api}'.format(
                     name=name,
                     req_ndk_api=ndk_api,
-                    dist_ndk_api=name_match_dist.ndk_api,
+                    dist_ndk_api=folder_match_dist.ndk_api,
                     req_recipes=', '.join(recipes),
-                    dist_recipes=', '.join(name_match_dist.recipes)))
+                    dist_recipes=', '.join(folder_match_dist.recipes)))
+
+        assert len(possible_dists) < 2
 
         # If we got this far, we need to build a new dist
         dist = Distribution(ctx)
@@ -152,9 +174,16 @@ def get_distribution(cls, ctx, name=None, recipes=[],
             name = filen.format(i)
 
         dist.name = name
-        dist.dist_dir = join(ctx.dist_dir, dist.name)
+        dist.dist_dir = join(
+            ctx.dist_dir,
+            generate_dist_folder_name(
+                name,
+                [arch_name] if arch_name is not None else None,
+            )
+        )
         dist.recipes = recipes
         dist.ndk_api = ctx.ndk_api
+        dist.archs = [arch_name]
 
         return dist
 
@@ -182,7 +211,7 @@ def get_distributions(cls, ctx, extra_dist_dirs=[]):
                 with open(join(folder, 'dist_info.json')) as fileh:
                     dist_info = json.load(fileh)
                 dist = cls(ctx)
-                dist.name = folder.split('/')[-1]
+                dist.name = dist_info['dist_name']
                 dist.dist_dir = folder
                 dist.needs_build = False
                 dist.recipes = dist_info['recipes']
@@ -210,7 +239,7 @@ def save_info(self, dirn):
         with current_directory(dirn):
             info('Saving distribution info')
             with open('dist_info.json', 'w') as fileh:
-                json.dump({'dist_name': self.ctx.dist_name,
+                json.dump({'dist_name': self.name,
                            'bootstrap': self.ctx.bootstrap.name,
                            'archs': [arch.arch for arch in self.ctx.archs],
                            'ndk_api': self.ctx.ndk_api,
@@ -236,3 +265,23 @@ def pretty_log_dists(dists, log_func=info):
 
     for line in infos:
         log_func('\t' + line)
+
+
+def generate_dist_folder_name(base_dist_name, arch_names=None):
+    """Generate the distribution folder name to use, based on a
+    combination of the input arguments.
+
+    Parameters
+    ----------
+    base_dist_name : str
+        The core distribution identifier string
+    arch_names : list of str
+        The architecture compile targets
+    """
+    if arch_names is None:
+        arch_names = ["no_arch_specified"]
+
+    return '{}__{}'.format(
+        base_dist_name,
+        '_'.join(arch_names)
+    )
diff --git a/pythonforandroid/python.py b/pythonforandroid/python.py
index c4c4ebcc19..a46b602c60 100755
--- a/pythonforandroid/python.py
+++ b/pythonforandroid/python.py
@@ -341,8 +341,11 @@ def create_python_bundle(self, dirn, arch):
         python_lib_name = 'libpython' + self.major_minor_version_string
         if self.major_minor_version_string[0] == '3':
             python_lib_name += 'm'
-        shprint(sh.cp, join(python_build_dir, python_lib_name + '.so'),
-                join(self.ctx.dist_dir, self.ctx.dist_name, 'libs', arch.arch))
+        shprint(
+            sh.cp,
+            join(python_build_dir, python_lib_name + '.so'),
+            join(self.ctx.bootstrap.dist_dir, 'libs', arch.arch)
+        )
 
         info('Renaming .so files to reflect cross-compile')
         self.reduce_object_file_names(join(dirn, 'site-packages'))
diff --git a/pythonforandroid/toolchain.py b/pythonforandroid/toolchain.py
index cb15ca0392..04cebfc673 100644
--- a/pythonforandroid/toolchain.py
+++ b/pythonforandroid/toolchain.py
@@ -101,6 +101,8 @@ def check_python_dependencies():
 toolchain_dir = dirname(__file__)
 sys.path.insert(0, join(toolchain_dir, "tools", "external"))
 
+APK_SUFFIX = '.apk'
+
 
 def add_boolean_option(parser, names, no_names=None,
                        default=True, dest=None, description=None):
@@ -163,6 +165,7 @@ def dist_from_args(ctx, args):
         ctx,
         name=args.dist_name,
         recipes=split_argument_list(args.requirements),
+        arch_name=args.arch,
         ndk_api=args.ndk_api,
         force_build=args.force_build,
         require_perfect_match=args.require_perfect_match,
@@ -195,10 +198,10 @@ def build_dist_from_args(ctx, dist, args):
     info('Dist will also contain modules ({}) installed from pip'.format(
         ', '.join(ctx.python_modules)))
 
-    ctx.dist_name = bs.distribution.name
+    ctx.distribution = dist
     ctx.prepare_bootstrap(bs)
     if dist.needs_build:
-        ctx.prepare_dist(ctx.dist_name)
+        ctx.prepare_dist()
 
     build_recipes(build_order, python_modules, ctx,
                   getattr(args, "private", None),
@@ -211,7 +214,7 @@ def build_dist_from_args(ctx, dist, args):
 
     info_main('# Your distribution was created successfully, exiting.')
     info('Dist can be found at (for now) {}'
-         .format(join(ctx.dist_dir, ctx.dist_name)))
+         .format(join(ctx.dist_dir, ctx.distribution.dist_dir)))
 
 
 def split_argument_list(l):
@@ -304,7 +307,7 @@ def __init__(self):
                   '(default: {})'.format(default_storage_dir)))
 
         generic_parser.add_argument(
-            '--arch', help='The archs to build for, separated by commas.',
+            '--arch', help='The arch to build for.',
             default='armeabi-v7a')
 
         # Options for specifying the Distribution
@@ -912,6 +915,7 @@ def export_dist(self, args):
     def _dist(self):
         ctx = self.ctx
         dist = dist_from_args(ctx, self.args)
+        ctx.distribution = dist
         return dist
 
     @require_prebuilt_dist
@@ -1062,9 +1066,9 @@ def apk(self, args):
         info_main('# Found APK file: {}'.format(apk_file))
         if apk_add_version:
             info('# Add version number to APK')
-            apk_name, apk_suffix = basename(apk_file).split("-", 1)
+            apk_name = basename(apk_file)[:-len(APK_SUFFIX)]
             apk_file_dest = "{}-{}-{}".format(
-                apk_name, build_args.version, apk_suffix)
+                apk_name, build_args.version, APK_SUFFIX)
             info('# APK renamed to {}'.format(apk_file_dest))
             shprint(sh.cp, apk_file, apk_file_dest)
         else:
diff --git a/tests/recipes/recipe_ctx.py b/tests/recipes/recipe_ctx.py
index 938b19e471..a162e8f0dc 100644
--- a/tests/recipes/recipe_ctx.py
+++ b/tests/recipes/recipe_ctx.py
@@ -26,7 +26,9 @@ class RecipeCtx:
     contain the target recipe to test as well as a python recipe."""
     recipe_build_order = []
     """A recipe_build_order which should take into account the recipe we want
-    to test as well as the possible dependant recipes"""
+    to test as well as the possible dependent recipes"""
+
+    TEST_ARCH = 'arm64-v8a'
 
     def setUp(self):
         self.ctx = Context()
@@ -37,7 +39,7 @@ def setUp(self):
         self.ctx.setup_dirs(os.getcwd())
         self.ctx.bootstrap = Bootstrap().get_bootstrap("sdl2", self.ctx)
         self.ctx.bootstrap.distribution = Distribution.get_distribution(
-            self.ctx, name="sdl2", recipes=self.recipes
+            self.ctx, name="sdl2", recipes=self.recipes, arch_name=self.TEST_ARCH,
         )
         self.ctx.recipe_build_order = self.recipe_build_order
         self.ctx.python_recipe = Recipe.get_recipe("python3", self.ctx)
diff --git a/tests/test_archs.py b/tests/test_archs.py
index 24c118eb40..ddb18b80e2 100644
--- a/tests/test_archs.py
+++ b/tests/test_archs.py
@@ -50,6 +50,8 @@ class ArchSetUpBaseClass(object):
     ctx = None
     expected_compiler = ""
 
+    TEST_ARCH = 'armeabi-v7a'
+
     def setUp(self):
         self.ctx = Context()
         self.ctx.ndk_api = 21
@@ -59,7 +61,10 @@ def setUp(self):
         self.ctx.setup_dirs(os.getcwd())
         self.ctx.bootstrap = Bootstrap().get_bootstrap("sdl2", self.ctx)
         self.ctx.bootstrap.distribution = Distribution.get_distribution(
-            self.ctx, name="sdl2", recipes=["python3", "kivy"]
+            self.ctx,
+            name="sdl2",
+            recipes=["python3", "kivy"],
+            arch_name=self.TEST_ARCH,
         )
         self.ctx.python_recipe = Recipe.get_recipe("python3", self.ctx)
         # Here we define the expected compiler, which, as per ndk >= r19,
diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py
index 6297eef551..02d54f0d9e 100644
--- a/tests/test_bootstrap.py
+++ b/tests/test_bootstrap.py
@@ -7,10 +7,11 @@
 from pythonforandroid.bootstrap import (
     _cmp_bootstraps_by_priority, Bootstrap, expand_dependencies,
 )
-from pythonforandroid.distribution import Distribution
+from pythonforandroid.distribution import Distribution, generate_dist_folder_name
 from pythonforandroid.recipe import Recipe
 from pythonforandroid.archs import ArchARMv7_a
 from pythonforandroid.build import Context
+from pythonforandroid.util import BuildInterruptingException
 
 from test_graph import get_fake_recipe
 
@@ -22,6 +23,8 @@ class BaseClassSetupBootstrap(object):
     `setUp` and `tearDown` methods.
     """
 
+    TEST_ARCH = 'armeabi-v7a'
+
     def setUp(self):
         self.ctx = Context()
         self.ctx.ndk_api = 21
@@ -43,7 +46,9 @@ def setUp_distribution_with_bootstrap(self, bs):
         """
         self.ctx.bootstrap = bs
         self.ctx.bootstrap.distribution = Distribution.get_distribution(
-            self.ctx, name="test_prj", recipes=["python3", "kivy"]
+            self.ctx, name="test_prj",
+            recipes=["python3", "kivy"],
+            arch_name=self.TEST_ARCH,
         )
 
     def tearDown(self):
@@ -71,15 +76,16 @@ def test_attributes(self):
         self.assertEqual(bs.jni_dir, "sdl2/jni")
         self.assertEqual(bs.get_build_dir_name(), "sdl2-python3")
 
-        # test dist_dir error
+        # bs.dist_dir should raise an error if there is no distribution to query
         bs.distribution = None
-        with self.assertRaises(SystemExit) as e:
+        with self.assertRaises(BuildInterruptingException):
             bs.dist_dir
-        self.assertEqual(e.exception.args[0], 1)
 
         # test dist_dir success
         self.setUp_distribution_with_bootstrap(bs)
-        self.assertTrue(bs.dist_dir.endswith("dists/test_prj"))
+        expected_folder_name = generate_dist_folder_name('test_prj', [self.TEST_ARCH])
+        self.assertTrue(
+            bs.dist_dir.endswith(f"dists/{expected_folder_name}"))
 
     def test_build_dist_dirs(self):
         """A test which will initialize a bootstrap and will check if the
@@ -249,8 +255,8 @@ def test_prepare_dist_dir(self, mock_ensure_dir):
         """
         bs = Bootstrap().get_bootstrap("sdl2", self.ctx)
 
-        bs.prepare_dist_dir("fake_name")
-        mock_ensure_dir.assert_called_once_with(bs.dist_dir)
+        bs.prepare_dist_dir()
+        mock_ensure_dir.assert_called_once()
 
     @mock.patch("pythonforandroid.bootstrap.open", create=True)
     @mock.patch("pythonforandroid.util.chdir")
diff --git a/tests/test_distribution.py b/tests/test_distribution.py
index e533a5af5e..3342b5f143 100644
--- a/tests/test_distribution.py
+++ b/tests/test_distribution.py
@@ -10,7 +10,7 @@
 from pythonforandroid.build import Context
 
 dist_info_data = {
-    "dist_name": None,
+    "dist_name": "sdl2_dist",
     "bootstrap": "sdl2",
     "archs": ["armeabi", "armeabi-v7a", "x86", "x86_64", "arm64-v8a"],
     "ndk_api": 21,
@@ -27,6 +27,8 @@ class TestDistribution(unittest.TestCase):
     :mod:`~pythonforandroid.distribution`.
     """
 
+    TEST_ARCH = 'armeabi-v7a'
+
     def setUp(self):
         """Configure a :class:`~pythonforandroid.build.Context` so we can
         perform our unittests"""
@@ -51,6 +53,7 @@ def setUp_distribution_with_bootstrap(self, bs, **kwargs):
             self.ctx,
             name=kwargs.pop("name", "test_prj"),
             recipes=kwargs.pop("recipes", ["python3", "kivy"]),
+            arch_name=self.TEST_ARCH,
             **kwargs
         )
 
@@ -108,7 +111,7 @@ def test_get_distribution_no_name(self, mock_exists):
         returns the proper result which should `unnamed_dist_1`."""
         mock_exists.return_value = False
         self.ctx.bootstrap = Bootstrap().get_bootstrap("sdl2", self.ctx)
-        dist = Distribution.get_distribution(self.ctx)
+        dist = Distribution.get_distribution(self.ctx, arch_name=self.TEST_ARCH)
         self.assertEqual(dist.name, "unnamed_dist_1")
 
     @mock.patch("pythonforandroid.util.chdir")
@@ -157,7 +160,8 @@ def test_get_distributions(
         self.assertIsInstance(dists, list)
         self.assertEqual(len(dists), 1)
         self.assertIsInstance(dists[0], Distribution)
-        self.assertEqual(dists[0].name, "sdl2-python3")
+        self.assertEqual(dists[0].name, "sdl2_dist")
+        self.assertEqual(dists[0].dist_dir, "sdl2-python3")
         self.assertEqual(dists[0].ndk_api, 21)
         self.assertEqual(
             dists[0].recipes,
@@ -206,7 +210,10 @@ def test_get_distributions_error_ndk_api_mismatch(
         distribution with the same `name` but different `ndk_api`.
         """
         expected_dist = Distribution.get_distribution(
-            self.ctx, name="test_prj", recipes=["python3", "kivy"]
+            self.ctx,
+            name="test_prj",
+            recipes=["python3", "kivy"],
+            arch_name=self.TEST_ARCH,
         )
         mock_get_dists.return_value = [expected_dist]
         mock_glob.return_value = ["sdl2-python3"]
@@ -254,7 +261,10 @@ def test_get_distributions_possible_dists(self, mock_get_dists):
         `:class:`~pythonforandroid.distribution.Distribution`.
         """
         expected_dist = Distribution.get_distribution(
-            self.ctx, name="test_prj", recipes=["python3", "kivy"]
+            self.ctx,
+            name="test_prj",
+            recipes=["python3", "kivy"],
+            arch_name=self.TEST_ARCH,
         )
         mock_get_dists.return_value = [expected_dist]
         self.setUp_distribution_with_bootstrap(