Drop CrystaX support and code base #1913

opacam · 2019-07-10T08:56:44Z

There is an open issue, a proposal to drop CrystaX support (#1905), so this PR takes care of this...in case that the team wants to take this road, we will see in here I guess..:wink:

Note: labeled WIP because the tests must be updated

AndreMiras · 2019-07-10T09:56:46Z

doc/source/buildoptions.rst

+.. note:: Since we don't support `python3crystax` anymore, the old instructions
+          has been removed from here. If you, still have the need to make use
+          of this old recipe, you should do it with an old `python-for-android`
+          release. Probably, a got starting point would be `version 0.7.0


Do you meen "a good starting point? `s/a got/a good/

AndreMiras · 2019-07-10T10:04:35Z

doc/source/docker.rst

+   Within the custom diff made by building the distribution there is another
+   big chunk of space eaten. The very basic stuff such as a distribution with:
+   CPython 3, setuptools, Python for Android ``android`` module, SDL2 (+ deps),
+   PyJNIus and Kivy takes almost 13 GB. Check your free space first!


Since CrystaX is gone from the Dockerfile I think the image size is way less.
DockerHub shows 2 GB.
https://cloud.docker.com/u/kivy/repository/docker/kivy/python-for-android/tags

pythonforandroid/recipes/genericndkbuild/__init__.py

AndreMiras · 2019-07-10T10:09:14Z

Awesome start 👏 can't wait to see it goes 😄

Closes kivy#1905

@AndreMiras

- Fix misspelled word - update docker image size to the current situation Thanks @AndreMiras!!

Because we already build them whenever we build our python recipes, so this way we will avoid to make it fail the `CI` test `rebuild_updated_recipes` in the case that we have to touch both hostpython recipes at the same time

Since now we have an smartest way to determine the right bootstrap based on recipes, it's time to rely on it for our `rebuild_updated_recipes` Closes: kivy#1594

ghost · 2019-07-13T11:40:05Z

pythonforandroid/recipe.py

+
+    depends = [('python2', 'python3')]
+    '''
+    .. note:: it's important to keep this depends as a class attribute, outside


The note is helpful, but it seems a bit overly long. I am not trying to be nitpicky, but it is actually a problem that when all explanatory comments get too long one tends to not read them. So I recommend shortening it down to:

.. note:: it's important to keep this depends as a class attribute outside `__init__` because sometimes we only initialize the class, so the `__init__` call won't be called and the deps would be missing (which breaks the dependency graph computation)

pythonforandroid/recipe.py

ghost · 2019-07-13T11:45:27Z

pythonforandroid/recipe.py

+              build order for dependencies will not be computed as expected (if
+              computed...). So be very careful with this line!!
+
+    .. warning:: this `depends` may be overwrote in inherited classes of


I would shorten this to:

.. warning:: don't forget to call `super().__init__()` in any recipe's `__init__`, or otherwise it may not be ensured that it depends on python2 or python3 which can break the dependency graph

ghost · 2019-07-13T11:48:27Z

pythonforandroid/recipe.py

+                if d in self.depends
+            ]
+        ):
+            # we overwrote `depends` in inherited recipe, so we must add it


I would rephrase this comment to:

# We ensure here that the recipe depends on python even it overrode `depends`. # We only do this if it doesn't already depend on any python, # since some recipes intentionally don't depend on/work with all python variants

ghost · 2019-07-13T11:50:43Z

Looks good to me in overall, I suggested a few minor things but don't consider them blocking on the merge.

I suggest we should merge this soon, because it looks like this may conflict with things and crystax really is annoying to keep around

inclement

Looks great to me, thanks!

inclement · 2019-07-14T11:30:30Z

doc/source/buildoptions.rst

+~~~~~~~~~~~~~~~
+
+python-for-android originally supported Python 3 using the CrystaX NDK.  Since
+we have a working python3 recipe, we don't support CrystaX NDK anymore. If you


I would be more direct with this kind of doc, you can just write

python-for-android no longer supports building for Python 3 using the CrystaX NDK. Instead, use the python3 recipe, which can be built using the normal Google NDK.

The last python-for-android version supporting CrystaX was 0.7.0.

No need for this to hold up merging the PR, we can improve the doc later (there are plenty of parts that I wrote that also need improvement), just noting as something I've been thinking about.

inclement · 2019-07-14T11:34:11Z

Not merging because I didn't check the status of the other comments, but it's fine to merge for me.

AndreMiras

Also looks good to me @opacam 👍
Let's address @Jonast comments and merge it

@Jonast

By shortening a little some long texts, thanks @Jonast and @inclement!!

opacam · 2019-07-14T19:44:22Z

Thanks @Jonast, @inclement and @AndreMiras,

for your reviews and text corrections 👍

All your suggestions as been applied as you wanted, so I think that we have this 😄

inclement · 2019-07-14T21:00:12Z

Thanks @opacam!

AndreMiras reviewed Jul 10, 2019

View reviewed changes

pythonforandroid/recipes/genericndkbuild/__init__.py Show resolved Hide resolved

opacam added 4 commits July 11, 2019 11:36

[crystax] Drop CrystaX support and code base

e3f2d81

Closes kivy#1905

[crystax] Text corrections for doc files

21f93d2

- Fix misspelled word - update docker image size to the current situation Thanks @AndreMiras!!

[crystax] Fix doc string typo for PythonRecipe

aad2730

[crystax] Fix the dependencies resolution

dc589c4

opacam force-pushed the feature-remove-crystax branch from 230fc1c to dc589c4 Compare July 11, 2019 12:03

opacam added 3 commits July 11, 2019 14:23

[crystax] Add hostpython recipes to CORE_RECIPES

7c1223e

Because we already build them whenever we build our python recipes, so this way we will avoid to make it fail the `CI` test `rebuild_updated_recipes` in the case that we have to touch both hostpython recipes at the same time

[crystax] Detect removed recipes for rebuild_updated_recipes

ac8df83

[crystax] Remove hardcoded sdl2 bootstrap in rebuild_updated_recipes

185936e

Since now we have an smartest way to determine the right bootstrap based on recipes, it's time to rely on it for our `rebuild_updated_recipes` Closes: kivy#1594

opacam changed the title ~~[WIP] Drop CrystaX support and code base~~ Drop CrystaX support and code base Jul 11, 2019

ghost reviewed Jul 13, 2019

View reviewed changes

pythonforandroid/recipe.py Show resolved Hide resolved

ghost reviewed Jul 13, 2019

View reviewed changes

inclement previously approved these changes Jul 14, 2019

View reviewed changes

AndreMiras previously approved these changes Jul 14, 2019

View reviewed changes

[crystax] Improve docs and inline comments

5ad8ad7

By shortening a little some long texts, thanks @Jonast and @inclement!!

opacam dismissed stale reviews from AndreMiras and inclement via 5ad8ad7 July 14, 2019 19:35

inclement approved these changes Jul 14, 2019

View reviewed changes

inclement merged commit fba37de into kivy:develop Jul 14, 2019

opacam deleted the feature-remove-crystax branch July 15, 2019 07:24

opacam mentioned this pull request Jul 15, 2019

P4A is broken with kivy==master kivy/kivy#6419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop CrystaX support and code base #1913

Drop CrystaX support and code base #1913

opacam commented Jul 10, 2019 •

edited

Loading

AndreMiras Jul 10, 2019

AndreMiras Jul 10, 2019

AndreMiras commented Jul 10, 2019

ghost Jul 13, 2019

ghost Jul 13, 2019

ghost Jul 13, 2019 •

edited by ghost

Loading

ghost commented Jul 13, 2019

inclement left a comment

inclement Jul 14, 2019

inclement commented Jul 14, 2019

AndreMiras left a comment

opacam commented Jul 14, 2019

inclement commented Jul 14, 2019

Drop CrystaX support and code base #1913

Drop CrystaX support and code base #1913

Conversation

opacam commented Jul 10, 2019 • edited Loading

AndreMiras Jul 10, 2019

Choose a reason for hiding this comment

AndreMiras Jul 10, 2019

Choose a reason for hiding this comment

AndreMiras commented Jul 10, 2019

ghost Jul 13, 2019

Choose a reason for hiding this comment

ghost Jul 13, 2019

Choose a reason for hiding this comment

ghost Jul 13, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Jul 13, 2019

inclement left a comment

Choose a reason for hiding this comment

inclement Jul 14, 2019

Choose a reason for hiding this comment

inclement commented Jul 14, 2019

AndreMiras left a comment

Choose a reason for hiding this comment

opacam commented Jul 14, 2019

inclement commented Jul 14, 2019

opacam commented Jul 10, 2019 •

edited

Loading

ghost Jul 13, 2019 •

edited by ghost

Loading