Make it raise an error if an old ndk is used #1883

opacam · 2019-06-22T19:19:05Z

In order to avoid issues with old android's NDKs, we force to raise an error in case that an NDK version lower than the specified is detected. We also add the ability to extract the android's NDK letter version, which for now, will only be used to inform the user of the version which is using.

Important note: Don't merge this before the ndk-r19 core part I (we can easily adapt this for ndk-r17 but I leave it to ndk-19 so we make sure that travis fails as expected), once the Part I gets merged, we can rebase this into develop, push it and the tests should pass or we could adapt it to ndk-r17 and merge it before

Note: The idea to create this pr comes from this comment from the pull request ndk-r19 initial migration

See also: The reported error in travis test

In order to avoid issues with old android's NDKs, we force to raise an error in case that an NDK version lower than the specified is detected. We also add the ability to extract the android's NDK letter version, which for now, will only be used to inform the user of the version which is using closes: kivy#1883

AndreMiras · 2019-07-18T20:28:15Z

Nice PR!
Yes I would be tempted to adapt it for ndk-r17 so we can make an early merge with easy adaptations later.
Also we may want to unit test this feature

In order to avoid issues with old android's NDKs, we force to raise an error in case that an NDK version lower than the specified is detected. We also add the ability to extract the android's NDK letter version, which for now, will only be used to inform the user of the version which is using

opacam · 2019-07-19T10:18:40Z

Ok, adapted to the current situation ndk-r17 and also added unittests for recommendations module.

As a side note: I skipped some of the tests for py2, because I make use of assertLog which is introduced in Python 3.4 and it fails for our tox py2 tests. There are solutions for this, but it would add an extra complexity and I don't think that it's worth it...@AndreMiras, let me know if you think different in this matter 😉

inclement · 2019-07-19T17:56:09Z

Please feel free to make this Python 3 only if you prefer, I'll fill out #1918 soon and I think it shouldn't be an issue to merge that PR first.

I didn't raise the question there yet, but 3.4+ seems like a fine support base for now.

opacam · 2019-07-19T18:09:10Z

I didn't raise the question there yet, but 3.4+ seems like a fine support base for now.

Aaahhh, I would go with Python 3.6+ so we could make use of f-strings without external dependencies.

inclement

Nice, thanks @opacam

inclement · 2019-07-19T17:58:36Z

pythonforandroid/recommendations.py

+    specified via attribute `MIN_NDK_VERSION`.
+
+    .. versionchanged:: 2019.06.06.1.dev0
+        Added the ability to get android's ndk `letter version` and also


Could you capitalise NDK throughout, just for consistency?

inclement · 2019-07-19T18:02:09Z

pythonforandroid/recommendations.py

+            'Note: If you got build errors, consider to download the'
+            ' recommended ndk version which is {rec_version} and try'
+            ' it again (after removing all the files generated with this'
+            ' build). To download the android NDK visit the following page: '


I think this should be worded more directly, something like:

warning("Unable to read the NDK version from the given directory {}".format(ndk_dir)) warning("Make sure your NDK version is greater than {min_supported}. If you get build errors, download the recommended NDK {rec_version} from {ndk_url}".format(...))

inclement · 2019-07-19T18:03:54Z

pythonforandroid/recommendations.py

-        warning(OLD_NDK_MESSAGE)
+        raise BuildInterruptingException(
+            'Unsupported NDK version detected {user_version}\n'
+            '* Note: Minimum supported NDK version is {min_supported}'.format(


Again, I'd write something shorter like "The minimum supported NDK version is {min_supported}. You can download it from {ndk_url}".

inclement · 2019-07-19T18:07:40Z

tests/test_recommendations.py

+        self.assertEqual(
+            cm.output,
+            [
+                "INFO:p4a:[INFO]:    Could not determine NDK version, "


I think all these exact output checks may be annoying to keep updated. How about storing the unformatted strings in recommendations.py, and importing them here when checking the printed output matches them? I wouldn't necessarily suggest that for most log output, but I think it makes sense for this kind of important message which is a meaningful string in itself and not a throwaway log.

More generally, it might be nice to have helper functions that can pattern match against the expected log level (info, warning, ...) and some of the string content, when the whole string isn't important. That's just a thought, not a request here.

AndreMiras · 2019-07-19T18:10:58Z

Yes @opacam of course we can skip python2 now 👍
Yes @inclement good question for python3, honestly I'd rather just support 3.6 and 3.7. It's purely arbitrary. Just because I like f-strings and also Ubuntu LTS doesn't have 3.5 😛

inclement · 2019-07-19T18:11:58Z

@opacam I'm not certain about 3.6+ but I'm open for discussion, feel free to raise it on the "drop python2" PR. I suggested assuming 3.4+ here only because that's the version you mentioned needing.

opacam · 2019-07-19T18:16:46Z

Guys, If you don't mind, please don't merge this, I would like to address the @inclement comments before 😉

So it will be easier for us to maintain the tests for recommendations module

opacam · 2019-07-19T21:29:06Z

Ok, I think I addressed all of the @inclement comments, going to dinner now...I will check later or tomorrow, let me know if you find anything to be changed 😄

AndreMiras · 2019-07-26T21:44:07Z

pythonforandroid/recommendations.py

+TARGET_NDK_API_GREATER_THAN_TARGET_API_MESSAGE = (
+    'Target NDK API is {ndk_api}, '
+    'higher than the target Android API {android_api}.'
+)


Just wondering, why in p4a we don't have all the const on the top of the file? It's probably fine, it's just that I'm more used to do it this way, or even sometimes in a dedicated file

AndreMiras · 2019-07-26T21:49:45Z

tests/test_recommendations.py

+        mock_open_src_prop.assert_called_once_with(
+            join(self.ndk_dir, "source.properties")
+        )
+        assert version == "17.2.4988734"


minor: inconsistent assert. I personally prefer this version, but it's not what you seem to be using in the rest of the file.
Same thing in test_read_ndk_version_error below.
But anyway we're far from being consistent in this with p4a currently. Don't know if we want to address or not

AndreMiras · 2019-07-26T21:50:51Z

tests/test_recommendations.py

+        assert version is None
+
+    def test_check_target_api_error_arch_armeabi(self):
+


Nitpick: why are we sometimes leaving a blank space after method definition and sometimes not?

AndreMiras · 2019-07-26T21:51:57Z

tests/test_recommendations.py

+    def test_check_ndk_api_warning_old_ndk(self):
+        """
+        Given an `android api` lower than the supported by p4a, we should
+        get an `BuildInterruptingException`.


This comment seem to be a lie, right?

AndreMiras · 2019-07-26T21:53:20Z

Very neat! I'm going to give it a try and if nothing seems to break, I'm in for a merge 👍
Edit: yes I forgot, are we rebased on current develop?

AndreMiras

Tested and didn't see any regression.
@opacam feel free to merge when you're ready

inclement · 2019-07-26T22:40:00Z

I'm assuming this is fine to merge. Thanks @opacam :)

opacam added the ndk-r19+ pull requests or issues related with android ndk-r19 label Jun 22, 2019

opacam added 2 commits July 19, 2019 12:05

[test] Add unittest for pythonforandroid.recommendations

19972b4

opacam force-pushed the feature-ndk-check branch from 59cc895 to 19972b4 Compare July 19, 2019 10:09

opacam removed the ndk-r19+ pull requests or issues related with android ndk-r19 label Jul 19, 2019

opacam changed the title ~~[WIP] Make it raise an error if an old ndk is used~~ Make it raise an error if an old ndk is used Jul 19, 2019

inclement requested changes Jul 19, 2019

View reviewed changes

inclement previously approved these changes Jul 19, 2019

View reviewed changes

opacam added 3 commits July 19, 2019 22:55

[texts] Capitalise NDK to be consistent

cbfb51f

[texts] Rewrite log messages to be more direct

6a8b3cb

[recommendations] Refactor important log messages

6688562

So it will be easier for us to maintain the tests for recommendations module

opacam dismissed inclement’s stale review via 6688562 July 19, 2019 21:25

opacam mentioned this pull request Jul 20, 2019

Drop Python 2 support #1918

Merged

AndreMiras reviewed Jul 26, 2019

View reviewed changes

AndreMiras mentioned this pull request Jul 26, 2019

New feature: allow to use a p4a fork kivy/buildozer#940

Merged

AndreMiras approved these changes Jul 26, 2019

View reviewed changes

inclement merged commit 13c3b51 into kivy:develop Jul 26, 2019

opacam mentioned this pull request Aug 24, 2019

Tidy up NDK 19+ support #1962

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it raise an error if an old ndk is used #1883

Make it raise an error if an old ndk is used #1883

opacam commented Jun 22, 2019 •

edited

Loading

AndreMiras commented Jul 18, 2019

opacam commented Jul 19, 2019

inclement commented Jul 19, 2019

opacam commented Jul 19, 2019

inclement left a comment

inclement Jul 19, 2019

inclement Jul 19, 2019

inclement Jul 19, 2019

inclement Jul 19, 2019

AndreMiras commented Jul 19, 2019

inclement commented Jul 19, 2019

opacam commented Jul 19, 2019

opacam commented Jul 19, 2019

AndreMiras Jul 26, 2019

AndreMiras Jul 26, 2019

AndreMiras Jul 26, 2019

AndreMiras Jul 26, 2019

AndreMiras commented Jul 26, 2019 •

edited

Loading

AndreMiras left a comment

inclement commented Jul 26, 2019

		assert version is None

		def test_check_target_api_error_arch_armeabi(self):

Make it raise an error if an old ndk is used #1883

Make it raise an error if an old ndk is used #1883

Conversation

opacam commented Jun 22, 2019 • edited Loading

AndreMiras commented Jul 18, 2019

opacam commented Jul 19, 2019

inclement commented Jul 19, 2019

opacam commented Jul 19, 2019

inclement 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

AndreMiras commented Jul 19, 2019

inclement commented Jul 19, 2019

opacam commented Jul 19, 2019

opacam commented Jul 19, 2019

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

AndreMiras commented Jul 26, 2019 • edited Loading

AndreMiras left a comment

Choose a reason for hiding this comment

inclement commented Jul 26, 2019

opacam commented Jun 22, 2019 •

edited

Loading

AndreMiras commented Jul 26, 2019 •

edited

Loading