Merge pull request #1887 from JonasT/fix_unpredictable_listbootstraps

Fix Bootstrap.get_bootstrap_from_recipes() so it's smarter and deterministic, fixes #1875

pythonforandroid/bootstrap.py

@@ -1,10 +1,11 @@
+import functools
+import glob
+import importlib
+import os
 from os.path import (join, dirname, isdir, normpath, splitext, basename)
 from os import listdir, walk, sep
 import sh
 import shlex
-import glob
-import importlib
-import os
 import shutil
 
 from pythonforandroid.logger import (warning, shprint, info, logger,
@@ -34,6 +35,35 @@ def copy_files(src_root, dest_root, override=True):
                 os.makedirs(dest_file)
 
 
+default_recipe_priorities = [
+    "webview", "sdl2", "service_only"  # last is highest
+]
+# ^^ NOTE: these are just the default priorities if no special rules
+# apply (which you can find in the code below), so basically if no
+# known graphical lib or web lib is used - in which case service_only
+# is the most reasonable guess.
+
+
+def _cmp_bootstraps_by_priority(a, b):
+    def rank_bootstrap(bootstrap):
+        """ Returns a ranking index for each bootstrap,
+            with higher priority ranked with higher number. """
+        if bootstrap.name in default_recipe_priorities:
+            return default_recipe_priorities.index(bootstrap.name) + 1
+        return 0
+
+    # Rank bootstraps in order:
+    rank_a = rank_bootstrap(a)
+    rank_b = rank_bootstrap(b)
+    if rank_a != rank_b:
+        return (rank_b - rank_a)
+    else:
+        if a.name < b.name:  # alphabetic sort for determinism
+            return -1
+        else:
+            return 1
+
+
 class Bootstrap(object):
     '''An Android project template, containing recipe stuff for
     compilation and templated fields for APK info.
@@ -138,36 +168,43 @@ def run_distribute(self):
         self.distribution.save_info(self.dist_dir)
 
     @classmethod
-    def list_bootstraps(cls):
+    def all_bootstraps(cls):
         '''Find all the available bootstraps and return them.'''
         forbidden_dirs = ('__pycache__', 'common')
         bootstraps_dir = join(dirname(__file__), 'bootstraps')
+        result = set()
         for name in listdir(bootstraps_dir):
             if name in forbidden_dirs:
                 continue
             filen = join(bootstraps_dir, name)
             if isdir(filen):
-                yield name
+                result.add(name)
+        return result
 
     @classmethod
-    def get_bootstrap_from_recipes(cls, recipes, ctx):
-        '''Returns a bootstrap whose recipe requirements do not conflict with
-        the given recipes.'''
+    def get_usable_bootstraps_for_recipes(cls, recipes, ctx):
+        '''Returns all bootstrap whose recipe requirements do not conflict
+        with the given recipes, in no particular order.'''
         info('Trying to find a bootstrap that matches the given recipes.')
         bootstraps = [cls.get_bootstrap(name, ctx)
-                      for name in cls.list_bootstraps()]
-        acceptable_bootstraps = []
+                      for name in cls.all_bootstraps()]
+        acceptable_bootstraps = set()
+
+        # Find out which bootstraps are acceptable:
         for bs in bootstraps:
             if not bs.can_be_chosen_automatically:
                 continue
-            possible_dependency_lists = expand_dependencies(bs.recipe_depends)
+            possible_dependency_lists = expand_dependencies(bs.recipe_depends, ctx)
             for possible_dependencies in possible_dependency_lists:
                 ok = True
+                # Check if the bootstap's dependencies have an internal conflict:
                 for recipe in possible_dependencies:
                     recipe = Recipe.get_recipe(recipe, ctx)
                     if any([conflict in recipes for conflict in recipe.conflicts]):
                         ok = False
                         break
+                # Check if bootstrap's dependencies conflict with chosen
+                # packages:
                 for recipe in recipes:
                     try:
                         recipe = Recipe.get_recipe(recipe, ctx)
@@ -180,14 +217,58 @@ def get_bootstrap_from_recipes(cls, recipes, ctx):
                         ok = False
                         break
                 if ok and bs not in acceptable_bootstraps:
-                    acceptable_bootstraps.append(bs)
+                    acceptable_bootstraps.add(bs)
+
         info('Found {} acceptable bootstraps: {}'.format(
             len(acceptable_bootstraps),
             [bs.name for bs in acceptable_bootstraps]))
-        if acceptable_bootstraps:
-            info('Using the first of these: {}'
-                 .format(acceptable_bootstraps[0].name))
-            return acceptable_bootstraps[0]
+        return acceptable_bootstraps
+
+    @classmethod
+    def get_bootstrap_from_recipes(cls, recipes, ctx):
+        '''Picks a single recommended default bootstrap out of
+           all_usable_bootstraps_from_recipes() for the given reicpes,
+           and returns it.'''
+
+        known_web_packages = {"flask"}  # to pick webview over service_only
+        recipes_with_deps_lists = expand_dependencies(recipes, ctx)
+        acceptable_bootstraps = cls.get_usable_bootstraps_for_recipes(
+            recipes, ctx
+        )
+
+        def have_dependency_in_recipes(dep):
+            for dep_list in recipes_with_deps_lists:
+                if dep in dep_list:
+                    return True
+            return False
+
+        # Special rule: return SDL2 bootstrap if there's an sdl2 dep:
+        if (have_dependency_in_recipes("sdl2") and
+                "sdl2" in [b.name for b in acceptable_bootstraps]
+                ):
+            info('Using sdl2 bootstrap since it is in dependencies')
+            return cls.get_bootstrap("sdl2", ctx)
+
+        # Special rule: return "webview" if we depend on common web recipe:
+        for possible_web_dep in known_web_packages:
+            if have_dependency_in_recipes(possible_web_dep):
+                # We have a web package dep!
+                if "webview" in [b.name for b in acceptable_bootstraps]:
+                    info('Using webview bootstrap since common web packages '
+                         'were found {}'.format(
+                             known_web_packages.intersection(recipes)
+                         ))
+                    return cls.get_bootstrap("webview", ctx)
+
+        prioritized_acceptable_bootstraps = sorted(
+            list(acceptable_bootstraps),
+            key=functools.cmp_to_key(_cmp_bootstraps_by_priority)
+        )
+
+        if prioritized_acceptable_bootstraps:
+            info('Using the highest ranked/first of these: {}'
+                 .format(prioritized_acceptable_bootstraps[0].name))
+            return prioritized_acceptable_bootstraps[0]
         return None
 
     @classmethod
@@ -299,9 +380,26 @@ def fry_eggs(self, sitepackages):
                 shprint(sh.rm, '-rf', d)
 
 
-def expand_dependencies(recipes):
+def expand_dependencies(recipes, ctx):
+    """ This function expands to lists of all different available
+        alternative recipe combinations, with the dependencies added in
+        ONLY for all the not-with-alternative recipes.
+        (So this is like the deps graph very simplified and incomplete, but
+         hopefully good enough for most basic bootstrap compatibility checks)
+    """
+
+    # Add in all the deps of recipes where there is no alternative:
+    recipes_with_deps = list(recipes)
+    for entry in recipes:
+        if not isinstance(entry, (tuple, list)) or len(entry) == 1:
+            if isinstance(entry, (tuple, list)):
+                entry = entry[0]
+            recipe = Recipe.get_recipe(entry, ctx)
+            recipes_with_deps += recipe.depends
+
+    # Split up lists by available alternatives:
     recipe_lists = [[]]
-    for recipe in recipes:
+    for recipe in recipes_with_deps:
         if isinstance(recipe, (tuple, list)):
             new_recipe_lists = []
             for alternative in recipe:
@@ -311,6 +409,6 @@ def expand_dependencies(recipes):
                     new_recipe_lists.append(new_list)
             recipe_lists = new_recipe_lists
         else:
-            for old_list in recipe_lists:
-                old_list.append(recipe)
+            for existing_list in recipe_lists:
+                existing_list.append(recipe)
     return recipe_lists

tests/test_bootstrap.py

@@ -1,6 +1,6 @@
+
 import os
 import sh
-
 import unittest
 
 try:
@@ -9,12 +9,16 @@
     # `Python 2` or lower than `Python 3.3` does not
     # have the `unittest.mock` module built-in
     import mock
-from pythonforandroid.bootstrap import Bootstrap
+from pythonforandroid.bootstrap import (
+    _cmp_bootstraps_by_priority, Bootstrap, expand_dependencies,
+)
 from pythonforandroid.distribution import Distribution
 from pythonforandroid.recipe import Recipe
 from pythonforandroid.archs import ArchARMv7_a
 from pythonforandroid.build import Context
 
+from test_graph import get_fake_recipe
+
 
 class BaseClassSetupBootstrap(object):
     """
@@ -90,7 +94,7 @@ def test_build_dist_dirs(self):
             - :meth:`~pythonforandroid.bootstrap.Bootstrap.get_dist_dir`
             - :meth:`~pythonforandroid.bootstrap.Bootstrap.get_common_dir`
         """
-        bs = Bootstrap().get_bootstrap("sdl2", self.ctx)
+        bs = Bootstrap.get_bootstrap("sdl2", self.ctx)
 
         self.assertTrue(
             bs.get_build_dir().endswith("build/bootstrap_builds/sdl2-python3")
@@ -100,32 +104,133 @@ def test_build_dist_dirs(self):
             bs.get_common_dir().endswith("pythonforandroid/bootstraps/common")
         )
 
-    def test_list_bootstraps(self):
+    def test__cmp_bootstraps_by_priority(self):
+        # Test service_only has higher priority than sdl2:
+        # (higher priority = smaller number/comes first)
+        self.assertTrue(_cmp_bootstraps_by_priority(
+            Bootstrap.get_bootstrap("service_only", self.ctx),
+            Bootstrap.get_bootstrap("sdl2", self.ctx)
+        ) < 0)
+
+        # Test a random bootstrap is always lower priority than sdl2:
+        class _FakeBootstrap(object):
+            def __init__(self, name):
+                self.name = name
+        bs1 = _FakeBootstrap("alpha")
+        bs2 = _FakeBootstrap("zeta")
+        self.assertTrue(_cmp_bootstraps_by_priority(
+            bs1,
+            Bootstrap.get_bootstrap("sdl2", self.ctx)
+        ) > 0)
+        self.assertTrue(_cmp_bootstraps_by_priority(
+            bs2,
+            Bootstrap.get_bootstrap("sdl2", self.ctx)
+        ) > 0)
+
+        # Test bootstraps that aren't otherwise recognized are ranked
+        # alphabetically:
+        self.assertTrue(_cmp_bootstraps_by_priority(
+            bs2,
+            bs1,
+        ) > 0)
+        self.assertTrue(_cmp_bootstraps_by_priority(
+            bs1,
+            bs2,
+        ) < 0)
+
+    def test_all_bootstraps(self):
         """A test which will initialize a bootstrap and will check if the
         method :meth:`~pythonforandroid.bootstrap.Bootstrap.list_bootstraps`
         returns the expected values, which should be: `empty", `service_only`,
         `webview` and `sdl2`
         """
         expected_bootstraps = {"empty", "service_only", "webview", "sdl2"}
-        set_of_bootstraps = set(Bootstrap().list_bootstraps())
+        set_of_bootstraps = Bootstrap.all_bootstraps()
         self.assertEqual(
             expected_bootstraps, expected_bootstraps & set_of_bootstraps
         )
         self.assertEqual(len(expected_bootstraps), len(set_of_bootstraps))
 
+    def test_expand_dependencies(self):
+        # Test dependency expansion of a recipe with no alternatives:
+        expanded_result_1 = expand_dependencies(["pysdl2"], self.ctx)
+        self.assertTrue(
+            {"sdl2", "pysdl2", "python3"} in
+            [set(s) for s in expanded_result_1]
+        )
+
+        # Test expansion of a single element but as tuple:
+        expanded_result_1 = expand_dependencies([("pysdl2",)], self.ctx)
+        self.assertTrue(
+            {"sdl2", "pysdl2", "python3"} in
+            [set(s) for s in expanded_result_1]
+        )
+
+        # Test all alternatives are listed (they won't have dependencies
+        # expanded since expand_dependencies() is too simplistic):
+        expanded_result_2 = expand_dependencies([("pysdl2", "kivy")], self.ctx)
+        self.assertEqual([["pysdl2"], ["kivy"]], expanded_result_2)
+
     def test_get_bootstraps_from_recipes(self):
         """A test which will initialize a bootstrap and will check if the
         method :meth:`~pythonforandroid.bootstrap.Bootstrap.
         get_bootstraps_from_recipes` returns the expected values
         """
+
+        import pythonforandroid.recipe
+        original_get_recipe = pythonforandroid.recipe.Recipe.get_recipe
+
+        # Test that SDL2 works with kivy:
         recipes_sdl2 = {"sdl2", "python3", "kivy"}
-        bs = Bootstrap().get_bootstrap_from_recipes(recipes_sdl2, self.ctx)
+        bs = Bootstrap.get_bootstrap_from_recipes(recipes_sdl2, self.ctx)
+        self.assertEqual(bs.name, "sdl2")
 
+        # Test that pysdl2 or kivy alone will also yield SDL2 (dependency):
+        recipes_pysdl2_only = {"pysdl2"}
+        bs = Bootstrap.get_bootstrap_from_recipes(
+            recipes_pysdl2_only, self.ctx
+        )
         self.assertEqual(bs.name, "sdl2")
+        recipes_kivy_only = {"kivy"}
+        bs = Bootstrap.get_bootstrap_from_recipes(
+            recipes_kivy_only, self.ctx
+        )
+        self.assertEqual(bs.name, "sdl2")
+
+        with mock.patch("pythonforandroid.recipe.Recipe.get_recipe") as \
+                mock_get_recipe:
+            # Test that something conflicting with sdl2 won't give sdl2:
+            def _add_sdl2_conflicting_recipe(name, ctx):
+                if name == "conflictswithsdl2":
+                    if name not in pythonforandroid.recipe.Recipe.recipes:
+                        pythonforandroid.recipe.Recipe.recipes[name] = (
+                            get_fake_recipe("sdl2", conflicts=["sdl2"])
+                        )
+                return original_get_recipe(name, ctx)
+            mock_get_recipe.side_effect = _add_sdl2_conflicting_recipe
+            recipes_with_sdl2_conflict = {"python3", "conflictswithsdl2"}
+            bs = Bootstrap.get_bootstrap_from_recipes(
+                recipes_with_sdl2_conflict, self.ctx
+            )
+            self.assertNotEqual(bs.name, "sdl2")
+
+        # Test using flask will default to webview:
+        recipes_with_flask = {"python3", "flask"}
+        bs = Bootstrap.get_bootstrap_from_recipes(
+            recipes_with_flask, self.ctx
+        )
+        self.assertEqual(bs.name, "webview")
+
+        # Test using random packages will default to service_only:
+        recipes_with_no_sdl2_or_web = {"python3", "numpy"}
+        bs = Bootstrap.get_bootstrap_from_recipes(
+            recipes_with_no_sdl2_or_web, self.ctx
+        )
+        self.assertEqual(bs.name, "service_only")
 
-        # test wrong recipes
+        # Test wrong recipes
         wrong_recipes = {"python2", "python3", "pyjnius"}
-        bs = Bootstrap().get_bootstrap_from_recipes(wrong_recipes, self.ctx)
+        bs = Bootstrap.get_bootstrap_from_recipes(wrong_recipes, self.ctx)
         self.assertIsNone(bs)
 
     @mock.patch("pythonforandroid.bootstrap.ensure_dir")