Made p4a use per-arch dist build dirs (#1986)

inclement · web-flow · commit 01c060704f48 · 2019-09-29T15:15:49.000+01:00
* 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
diff --git 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
@@ -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
@@ -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,35 +130,37 @@ 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)):
                 info_notify('{} has compatible recipes, using this one'
                             .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 '
                 'with this name already exists and has either incompatible 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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/tests/test_distribution.py b/tests/test_distribution.py
