Enhance distribution reuse: `android_api` #2031

opacam · 2019-11-30T15:17:03Z

In here we solve a problem we have when changing android_api if we already have a build distribution that we can reuse. The problem is that the changes are not reflected in the final APK, because there is some hardcoded android_api in some of the distribution files. So in here we will update those files so when we perform the build of the APK with gradle it will change the necessary stuff with the new android_api.

We also:

Store the android_api value to dist_info.json file (and update it if we reuse the dist with a new android_api)
Prettify dist_info.json (to be easier to read it for us)
Reduce complexity of Distribution.get_distributions
Add unittests for the added code

How to test this

Well, it will require two builds, the first one with one android_api and then another with a new android_api. Once the second build complete you will see that the distribution file: AndroidManifest.xml has the value of the second build (btw, we never modify this file in this PR, this file is modified by gradle). You can check around line 22, for something like:

    <uses-sdk android:minSdkVersion="21" android:targetSdkVersion="28" />

The value of android:targetSdkVersion should be the one you used with android_api in your second build.

How will look like the `dist_info.json` file

{
    "android_api": 28,
    "archs": [
        "arm64-v8a"
    ],
    "bootstrap": "sdl2",
    "dist_name": "bdisttest_python3_sqlite_openssl_googlendk",
    "hostpython": "/home/opacam/.local/share/python-for-android/build/other_builds/hostpython3/desktop/hostpython3/native-build/python3",
    "ndk_api": 21,
    "python_version": "3.7",
    "recipes": [
        "hostpython3",
        "libffi",
        "openssl",
        "sdl2_image",
        "sdl2_mixer",
        "sdl2_ttf",
        "sqlite3",
        "python3",
        "sdl2",
        "setuptools",
        "requests",
        "six",
        "pyjnius",
        "android",
        "kivy",
        "peewee"
    ],
    "use_setup_py": false
}

As you see, the keys are sorted and indented with 4 spaces 😉...imho, far readable than all in one line, right?

Notes

Some parts of the code where taken from #1926, so this PR is a portion of what we presented there, but refreshed and enhanced.

inclement · 2019-12-08T12:23:45Z

Thanks @opacam, that's a great improvement. Just to let you know I've seen this, but probably won't be able to review it for a few days.

AndreMiras

I missed that PR. I made a quick first look and seems overall good to me.
I have nothing against merging it as it is after inclement view it

AndreMiras · 2019-12-08T13:09:20Z

pythonforandroid/distribution.py

        return dists

+    @classmethod
+    def get_dist_from_dist_info(cls, ctx, dist_info, dist_dir):


I like that you split concerns making a dedicated method for this

AndreMiras · 2019-12-08T13:13:53Z

pythonforandroid/distribution.py

+        with open(dist_json_file, 'r') as file_opened:
+            dist_info = json.load(file_opened)
+        return dist_info
+
    def save_info(self, dirn):
        '''
        Save information about the distribution in its dist_dir.
        '''
        with current_directory(dirn):
            info('Saving distribution info')
            with open('dist_info.json', 'w') as fileh:


I feel like we could reuse the newly created dist_info_file property here

AndreMiras · 2019-12-08T13:15:27Z

pythonforandroid/distribution.py

+                    sort_keys=True,
+                )
+
+    def update_dist_info(self, key, value):


it feels like this method could share some code with save_info(), could it?

AndreMiras · 2019-12-08T15:25:30Z

tests/test_distribution.py

+        )
+        mock_open.side_effect = [
+            mock.mock_open(read_data=json.dumps(dist_info_data)).return_value
+        ]


Using mock open this way is somehow bugging me, I guess I'm not used to it.
How about something more conventional?

def test_get_dist_info(self): """Test that method :meth:`~pythonforandroid.distribution.Distribution.get_dist_info` calls the proper methods and returns the proper value.""" self.setUp_distribution_with_bootstrap( Bootstrap().get_bootstrap("sdl2", self.ctx) ) mock_open = mock.mock_open(read_data=json.dumps(dist_info_data)) with mock.patch("pythonforandroid.distribution.open", mock_open): dist_info = self.ctx.bootstrap.distribution.get_dist_info("/fake_dir") mock_open.assert_called_once_with("/fake_dir/dist_info.json", "r") self.assertIsInstance(dist_info, dict)

It's really passing the mock_open.side_effect with the other return_value that is bugging me, it feels unnatural.

AndreMiras · 2019-12-08T15:25:49Z

tests/test_distribution.py

+        ]
+        dist_info = self.ctx.bootstrap.distribution.get_dist_info("/fake_dir")
+        mock_open.assert_called_once_with("/fake_dir/dist_info.json", "r")
+        self.assertIsInstance(dist_info, dict)


Would it make sense to go as far as:

self.assertEqual(dist_info, dist_info_data)

opacam · 2019-12-09T10:14:12Z

Thanks @opacam, that's a great improvement. Just to let you know I've seen this, but probably won't be able to review it for a few days.

@inclement, no problem at all, this way we can polish this thing thanks to the @AndreMiras review 😉

@AndreMiras, I will take care of your comments ASAP...I did a quick look to your comments and I see that we can do a little refactoring, nice !! 😆

Because later we will add a little more complexity to this part of the code and it would be too much.

So it's easy for us to read it. Note: Also formatted lines to be PEP8 friendly (below 79 characters)

So we can later check if the dist was created with an different `android api`

So the gradle build can reuse an existing dist and build the apk with the new target api.

…oid_api`

So we can reduce the a little more the complexity of cls `Distribution`

Which is in this case: `'pythonforandroid.util.current_directory' imported but unused`

Caused by the latest changes to `Distribution.save_info`

Fak3 · 2019-12-13T22:07:34Z

Just a thought - could this dist-info.json also be used to hold the environment variables for debugging purposes? Currently they only exist in the temp file during build, so some time ago I raised a PR to keep this file permanently inside the dist dir: #1902

inclement

Sorry for the especially bad delay on this one. The change is good, but I wonder if it would be better to simply move the android api setting to a templated file like everything else, rather than adding a build toolchain step for it? Maybe I missed a reason not to do that.

AndreMiras reviewed Dec 8, 2019

View reviewed changes

opacam added 13 commits December 9, 2019 22:50

[dist] Reduce complexity of Distribution.get_distributions

7fa47b5

Because later we will add a little more complexity to this part of the code and it would be too much.

[dist] Prettify dist_info.json file

a880d5a

So it's easy for us to read it. Note: Also formatted lines to be PEP8 friendly (below 79 characters)

[dist] Save android api into dist_info.json

d58c0d2

So we can later check if the dist was created with an different `android api`

[dist] Be sure that android api changes updates distribution files

6c6fc58

So the gradle build can reuse an existing dist and build the apk with the new target api.

[test] Add test for Distribution.dist_info_file

9ebca5a

[test] Add test for Distribution.get_dist_info

5bb9b04

[test] Add test for Distribution.test_update_dist_info

87f664a

[test] Add test for `Distribution.update_dist_project_properties_andr…

fe35703

…oid_api`

[test] Add test for Distribution.update_dist_android_api

9942f09

[dist] Convert get_dist_info into the cls property dist_info

948b83b

So we can reduce the a little more the complexity of cls `Distribution`

[dist] Make use of property dist_info_file at save_info

cfad9a5

[dist] Fix PEP8 F401

880e432

Which is in this case: `'pythonforandroid.util.current_directory' imported but unused`

[dist] Fix error with test_bootstraps

de8c2d5

Caused by the latest changes to `Distribution.save_info`

opacam force-pushed the feature-dist-android-api branch from d5dacf1 to de8c2d5 Compare December 9, 2019 21:52

inclement reviewed Mar 30, 2020

View reviewed changes

kuzeyron added the core-providers Core code that's not a recipe label Apr 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enhance distribution reuse: `android_api` #2031

Enhance distribution reuse: `android_api` #2031

Uh oh!

opacam commented Nov 30, 2019

Uh oh!

inclement commented Dec 8, 2019

Uh oh!

AndreMiras left a comment

Uh oh!

AndreMiras Dec 8, 2019

Uh oh!

AndreMiras Dec 8, 2019

Uh oh!

AndreMiras Dec 8, 2019

Uh oh!

AndreMiras Dec 8, 2019

Uh oh!

AndreMiras Dec 8, 2019

Uh oh!

opacam commented Dec 9, 2019

Uh oh!

Fak3 commented Dec 13, 2019

Uh oh!

inclement left a comment

Uh oh!

Uh oh!

Enhance distribution reuse: android_api #2031

Are you sure you want to change the base?

Enhance distribution reuse: android_api #2031

Uh oh!

Conversation

opacam commented Nov 30, 2019

How to test this

How will look like the dist_info.json file

Notes

Uh oh!

inclement commented Dec 8, 2019

Uh oh!

AndreMiras left a comment

Choose a reason for hiding this comment

Uh oh!

AndreMiras Dec 8, 2019

Choose a reason for hiding this comment

Uh oh!

AndreMiras Dec 8, 2019

Choose a reason for hiding this comment

Uh oh!

AndreMiras Dec 8, 2019

Choose a reason for hiding this comment

Uh oh!

AndreMiras Dec 8, 2019

Choose a reason for hiding this comment

Uh oh!

AndreMiras Dec 8, 2019

Choose a reason for hiding this comment

Uh oh!

opacam commented Dec 9, 2019

Uh oh!

Fak3 commented Dec 13, 2019

Uh oh!

inclement left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Enhance distribution reuse: `android_api` #2031

Enhance distribution reuse: `android_api` #2031

How will look like the `dist_info.json` file