Code improvements around NDK download #961

inclement · 2019-08-31T20:27:12Z

Made a few improvements while checking NDK version downloads were working correctly.

I'm pretty satisfied that everything is good, thanks @opacam for doing the real work here.

In particular, the scenarios that need covering are:

New install: buildozer downloads p4a latest release then a matching NDK.
P4a already downloaded: buildozer checks the p4a recommendations and fetches a matching NDK.
NDK already downloaded: buildozer downloads p4a latest release, then fetches a matching NDK if necessary.

I was worried there might be some issues with how buildozer stores the NDKs, but it all seems basically good.

Also added a new command buildozer appclean that deletes the build directory (i.e. app_dir/.buildozer), because this is much nicer than telling users to delete it themselves.

AndreMiras

This is looking very good, thank you!
I wrote some comments, but most of them are remark or nitpick, just so we're aware of. So as usual feel free to ignore obviously.
Something more major however is tests are failing. Do you have an idea why? Do you need help investigating?

======================================================================
ERROR: test_p4a_recommended_android_ndk_found (tests.test_buildozer.TestBuildozer)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.4/unittest/mock.py", line 1128, in patched
    arg = patching.__enter__()
  File "/usr/lib/python3.4/unittest/mock.py", line 1197, in __enter__
    original, local = self.get_original()
  File "/usr/lib/python3.4/unittest/mock.py", line 1171, in get_original
    "%s does not have the attribute %r" % (target, name)
AttributeError: <module 'buildozer.targets.android' from '/home/travis/build/kivy/buildozer/buildozer/targets/android.py'> does not have the attribute 'open'
----------------------------------------------------------------------

buildozer/__init__.py

AndreMiras · 2019-09-17T12:31:50Z

buildozer/__init__.py

@@ -879,10 +871,27 @@ def namify(self, name):
    def root_dir(self):
        return realpath(dirname(self.specfilename))

+    @property
+    def user_build_dir(self):
+        '''The user-provided build dir, if any.'''


I like that we now have a dedicated method for this. But I also have a nitpick.
PEP-0257 say docstring should be double quotes, not single. Same in other docstrings. I know buildozer and p4a is already inconsistent here.
https://www.python.org/dev/peps/pep-0257/#what-is-a-docstring

AndreMiras · 2019-09-17T12:34:41Z

buildozer/__init__.py

+        if self.user_build_dir is not None:
+            self.error(
+                ('Failed: build_dir is specified as {} in the buildozer config. `appclean` will '
+                 'not attempt to delete files in a user-specified build directory.').format(self.user_build_dir))


Nice that you thought about preventing that

AndreMiras · 2019-09-17T12:36:14Z

buildozer/targets/android.py

+# that python-for-android cannot provide a recommendation, which in
+# turn only happens if the python-for-android is old and probably
+# doesn't support any newer NDK.
+DEFAULT_ANDROID_NDK_VERSION = '17c'


Thanks for that comment

AndreMiras · 2019-09-17T12:38:02Z

buildozer/targets/android.py

+        # Default p4a dir
+        p4a_dir = join(self.buildozer.platform_dir, self.p4a_directory_name)
+
+        # Possibly overriden by user setting


s/overriden/overridden/

AndreMiras · 2019-09-17T12:40:42Z

buildozer/targets/android.py

-        else:
-            pa_dir = self.pa_dir
+        # make sure to read p4a version only the first time
+        if self.p4a_recommended_ndk_version is not None:


if we implement some kind of caching, then maybe in the future we want to make it a dedicated property that automagically deals with it

AndreMiras · 2019-09-17T12:42:01Z

buildozer/targets/android.py

@@ -430,7 +437,7 @@ def _install_android_ndk(self):
                archive = 'android-ndk-r{0}-linux-{1}.tar.bz2'
            is_64 = (os.uname()[4] == 'x86_64')
        else:
-            raise SystemError('Unsupported platform: {0}'.format(platform))
+            raise SystemError('Unsupported platform: {}'.format(platform))


I'm wondering if you caught that with unit tests or with sharp eyes 😄

inclement · 2019-09-22T21:27:04Z

I fixed the previous test failure about the open function (which seemed like a reasonable failure, I don't know why it worked locally), but now I get another issue that I can't reproduce locally:

======================================================================

FAIL: test_p4a_recommended_android_ndk_found (tests.test_buildozer.TestBuildozer)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/usr/lib/python3.4/unittest/mock.py", line 1136, in patched

    return func(*args, **keywargs)

  File "/home/travis/build/kivy/buildozer/tests/test_buildozer.py", line 247, in test_p4a_recommended_android_ndk_found

    assert ndk_version == expected_ndk

AssertionError

The failure is straightforward enough, but it isn't clear why it's failing and why it only fails in travis...

inclement · 2019-10-01T21:28:42Z

I've pushed an improvement to how the dist dir is calculated (including a backwards compatibility fix, I think this was broken in #978 for existing dists). Let's see if the tests will pass now...

inclement · 2019-10-01T21:46:26Z

Actually there might be some further rebase issues, not for merging quite yet.

AndreMiras · 2019-10-02T21:58:38Z

I fixed the previous test failure about the open function (which seemed like a reasonable failure, I don't know why it worked locally), but now I get another issue that I can't reproduce locally:
======================================================================

FAIL: test_p4a_recommended_android_ndk_found (tests.test_buildozer.TestBuildozer)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/usr/lib/python3.4/unittest/mock.py", line 1136, in patched

    return func(*args, **keywargs)

  File "/home/travis/build/kivy/buildozer/tests/test_buildozer.py", line 247, in test_p4a_recommended_android_ndk_found

    assert ndk_version == expected_ndk

AssertionError
The failure is straightforward enough, but it isn't clear why it's failing and why it only fails in travis...

I could reproduce on my local Ubuntu.

======================================================================
FAIL: test_p4a_recommended_android_ndk_found (tests.test_buildozer.TestBuildozer)
----------------------------------------------------------------------                                                                             
Traceback (most recent call last):                                                                                                                                                                                                                                                       
  File "/home/andre/workspace/buildozer/.tox/py27/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/home/andre/workspace/buildozer/tests/test_buildozer.py", line 253, in test_p4a_recommended_android_ndk_found
    assert ndk_version == expected_ndk                                          
AssertionError                                                           
                                                                                                           
----------------------------------------------------------------------

Using pytest gave more insights:

venv/bin/pytest tests/test_buildozer.py
================================================================================================================================== test session starts ==================================================================================================================================
platform linux -- Python 3.6.7, pytest-4.1.1, py-1.7.0, pluggy-0.8.1
rootdir: /home/andre/workspace/buildozer, inifile:
collected 11 items                                                                                                                                                                                                                                                                      

tests/test_buildozer.py ........F..                                                                                                                                                                                                                                               [100%]

======================================================================================================================================= FAILURES ========================================================================================================================================
_________________________________________________________________________________________________________________ TestBuildozer.test_p4a_recommended_android_ndk_found __________________________________________________________________________________________________________________

self = <tests.test_buildozer.TestBuildozer testMethod=test_p4a_recommended_android_ndk_found>, mock_open = <MagicMock name='open' id='140137745055416'>, mock_exists = <MagicMock name='exists' id='140137745014624'>, mock_isfile = <MagicMock name='isfile' id='140137744984832'>

    @mock.patch('buildozer.targets.android.os.path.isfile')
    @mock.patch('buildozer.targets.android.os.path.exists')
    @mock.patch('buildozer.targets.android.open', create=True)
    def test_p4a_recommended_android_ndk_found(
            self, mock_open, mock_exists, mock_isfile
    ):
        self.set_specfile_log_level(self.specfile.name, 1)
        buildozer = Buildozer(self.specfile.name, 'android')
    
        expected_ndk = '19b'
        mock_open.side_effect = [
            mock.mock_open(
                read_data='RECOMMENDED_NDK_VERSION = {expected_ndk}\n'.format(
                    expected_ndk=expected_ndk)
            ).return_value
        ]
        ndk_version = buildozer.target.p4a_recommended_android_ndk
        p4a_dir = os.path.join(
            buildozer.platform_dir, buildozer.target.p4a_directory_name)
        mock_open.assert_called_once_with(
            os.path.join(p4a_dir, "pythonforandroid", "recommendations.py"), 'r'
        )
        print('Found "{}" vs expected "{}"'.format(ndk_version, expected_ndk))
        print('Values are equal: {}'.format(ndk_version == expected_ndk))
        print('Types {} {}'.format(type(ndk_version), type(expected_ndk)))
        print('Lengths {} {}'.format(len(ndk_version), len(expected_ndk)))
>       assert ndk_version == expected_ndk
E       AssertionError: assert '17c' == '19b'
E         - 17c
E         + 19b

tests/test_buildozer.py:253: AssertionError

Also as I suggested on Discord, maybe we should also migrate that build to docker (and Makefile), I promise build won't run for over 10 minutes 😬

AndreMiras · 2019-10-04T22:33:10Z

buildozer/targets/android.py

@@ -47,26 +47,28 @@
 # does.
 DEFAULT_SDK_TAG = '4333796'

+DEFAULT_ARCH = 'armeabi-v7a'


Actually at some point maybe the default should be arm64-v8a since the play store requires at least that one when uploading APK

I agree, but chose not to change the default in this particular PR.

inclement · 2019-10-04T22:44:51Z

Hah, looks like just importing mock works. Must be a unittest.mock change between Python 3.6 and 3.7.

I've just changed the code to import mock for now, this can easily be changed and investigated later. I'd rather get this merged as soon as possible.

AndreMiras

I've tried it locally the APK build OK. Let's squash and merge it

AndreMiras · 2019-10-05T13:06:29Z

Here's the follow up PR that fixes mocking #983

inclement · 2019-10-05T16:39:39Z

Thanks @AndreMiras!

inclement mentioned this pull request Aug 31, 2019

Tidy up NDK 19+ support kivy/python-for-android#1962

Closed

7 tasks

AndreMiras previously approved these changes Sep 17, 2019

View reviewed changes

inclement dismissed AndreMiras’s stale review via 7e98381 September 22, 2019 20:11

inclement force-pushed the make_ndk_match_recommendation branch from d5ac294 to f3dab17 Compare October 1, 2019 21:27

inclement added 5 commits October 1, 2019 22:29

Cleaned up some variable references

01930b7

Improved how user build dir is accessed

1172ae0

Fixed tests

2986443

Minor code style improvements

4baf46c

Tried changing open mock to create=True for travis build

7d18860

inclement force-pushed the make_ndk_match_recommendation branch from f3dab17 to 5d1ddd2 Compare October 1, 2019 21:42

Fixed dist_dir calculation following p4a changes

5d1ddd2

inclement force-pushed the make_ndk_match_recommendation branch 2 times, most recently from 59d744b to 877fc87 Compare October 2, 2019 21:48

inclement force-pushed the make_ndk_match_recommendation branch from 877fc87 to 8bc5ffd Compare October 4, 2019 22:13

Committed debug print function to examine travis test output

8bc5ffd

AndreMiras reviewed Oct 4, 2019

View reviewed changes

Reverted debug prints

7c8c1c8

AndreMiras approved these changes Oct 5, 2019

View reviewed changes

AndreMiras merged commit 9fcf28b into kivy:master Oct 5, 2019

AndreMiras mentioned this pull request Mar 30, 2020

AttributeError: 'function' object has no attribute 'glob' #1044

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code improvements around NDK download #961

Code improvements around NDK download #961

inclement commented Aug 31, 2019

AndreMiras left a comment

AndreMiras Sep 17, 2019

AndreMiras Sep 17, 2019

AndreMiras Sep 17, 2019

AndreMiras Sep 17, 2019

AndreMiras Sep 17, 2019

AndreMiras Sep 17, 2019

inclement commented Sep 22, 2019

inclement commented Oct 1, 2019

inclement commented Oct 1, 2019

AndreMiras commented Oct 2, 2019

AndreMiras Oct 4, 2019

inclement Oct 4, 2019

inclement commented Oct 4, 2019

AndreMiras left a comment

AndreMiras commented Oct 5, 2019

inclement commented Oct 5, 2019

Code improvements around NDK download #961

Code improvements around NDK download #961

Conversation

inclement commented Aug 31, 2019

AndreMiras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inclement commented Sep 22, 2019

inclement commented Oct 1, 2019

inclement commented Oct 1, 2019

AndreMiras commented Oct 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inclement commented Oct 4, 2019

AndreMiras left a comment

Choose a reason for hiding this comment

AndreMiras commented Oct 5, 2019

inclement commented Oct 5, 2019