Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to get p4a's recommended android's NDK version #947

Merged
merged 1 commit into from
Aug 24, 2019

Conversation

opacam
Copy link
Member

@opacam opacam commented Aug 1, 2019

So this way, if an user has not specified the android's NDK version via buildozer.spec's file, We will try to get the recommended one by p4a, established in pythonforandroid.recommendations.py file, so depending on the version of p4a used, we can use an android NDK or another.

This will allow us to solve the problem where p4a needs to move to a newer android NDK in develop branch, but also we need to preserve the old one for p4a's master branch (like the ndk-r19-migration thing that we are dealing this days in p4a, kivy/python-for-android#1722).

Note: in case that an old version of p4a is used, previous to 2019, where the p4a's recommendations file still not existed, We will fallback to buildozer's hardcoded NDK version

@AndreMiras
Copy link
Member

I haven't deeply checked yet, but that sounds like a very cool idea 👏
Could you also add the unit tests for this new feature?

@opacam
Copy link
Member Author

opacam commented Aug 1, 2019

Oh yes, I will try to do something to cover this 😉

@opacam
Copy link
Member Author

opacam commented Aug 1, 2019

Ok, added unittests,
this should cover almost all the lines I added 😉

@AndreMiras
Copy link
Member

Awesome, thanks for that! I'll try reviewing later. I still have a lot to catch up with on other PR, but I have to say PR with tests usually get higher prio for me 😄 So you may have yours reviewed earlier than expected

AndreMiras
AndreMiras previously approved these changes Aug 23, 2019
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I've installed the buildozer version of your fork.

pip3 install --upgrade--user https://github.com/opacam/buildozer/archive/feature-use-p4a-ndk.zip

And I made a build with p4a.branch = develop.
Logs gave:

# Recommended android's NDK version by p4a is: 17c                                                                                                                                                                                                                                       

Which is the ndk I'm using and build succeeded

# recommended android's NDK version, otherwise return buildozer's one
ndk_version = ANDROID_NDK_VERSION
rec_file = join(pa_dir, "pythonforandroid", "recommendations.py")
if (not os.path.exists(pa_dir)) or (not os.path.isfile(rec_file)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: why not only checking for os.path.isfile(rec_file)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AndreMiras 😄

Only if the p4a version used has the file `pythonforandroid/recommendations.py`, which contains the info we want, otherwise we will return the buildozer's one
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looks good to merge to me

@opacam
Copy link
Member Author

opacam commented Aug 24, 2019

thanks @AndreMiras 😄

since it seems that @inclement has no opposition on this one (kivy/python-for-android#1962)

I merge this 👍

@opacam opacam merged commit 182d13f into kivy:master Aug 24, 2019
@opacam opacam deleted the feature-use-p4a-ndk branch August 24, 2019 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants