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

Android unit tests #50

Merged
merged 10 commits into from
Mar 14, 2017
Merged

Conversation

kneth
Copy link

@kneth kneth commented Feb 20, 2017

My first attempt on getting the unit tests was far too complex.

With this PR I take a much simpler route: running the unit tests on the command-line. I have bumped the required version of CMake (2.6.3 is required by the Android toolchain). Moreover, I have added PEGTL_PLATFORM as a shortcut of setting everything but it is possible to override settings on the command-line. The default settings assume that you are using an x86 based Android device (aka emulator - commonly used by Android developers).

Building and running unit tests for an Android can be done by:

cmake -DANDROID_NDK=/path/to/android-ndk -DPEGTL_PLATFORM=Android
make
make test

(if you have an Android device, adding -DANDROID_ABI=armeabi-v7a is useful)

During testing it is required that the device is connected (an emulator is always connected). The script run-on-android.sh assumes that only one device is connected.

I have tested it on an Android device (OnePlus One) and emulator using Android NDK r10e and r13b.

Unfortunately, Travis CI doesn't support Android NDK (travis-ci/travis-ci#5395) so we might find another way to run the tests in an automated way.

@d-frey
Copy link
Member

d-frey commented Feb 20, 2017

I like this direction, even though requiring CMake 3.6.0 as a minimum does exclude quite a few systems. But we are using it more for testing purposes than as a recommended build/installation system towards the user, so it's not a problem. Currently, we use CMake on AppVeyor and on the Fedora 24 builds in Doozer.io. The AppVeyor environment has CMake 3.7.x at the moment, so this, too, is no problem. I'm not sure about Doozer.io, but that should not hold us back, in case of doubt I'd sacrifice those builds.

Now for the real issue: Automated builds for Android. If I read the travis-ci issue you linked correctly, it seems that the Android NDK is not installed by default, but you can do it manually. In travis-ci/travis-ci#5395, the reporter links a working (manual) build and shows the "old" and his envisioned "new" way. And ultimately, the idea seems to be to speed up the build process which I don't care too much about. If it takes 2 minutes longer, so be it 😃. Could you try to replicate the manual way of installing the NDK and running the PEGTL tests with it?

@kneth
Copy link
Author

kneth commented Mar 1, 2017

@d-frey Sorry for not coming back sooner. I have not experience with Travis, and in the last week I have been busy. But I will give the manual method a shot.

@d-frey
Copy link
Member

d-frey commented Mar 1, 2017

Thanks Kenneth.

If you are working on your branch, feel free to nuke all other builds in the .travis.yml and just focus on the Android build. This should help you to speed things up considerably, as usually the MacOS build take quite some time before the Travis infrastructure has a free slot for them. I am positive that I'll be able to merge whatever you end up with in your .travis.yml with the existing builds.

@kneth
Copy link
Author

kneth commented Mar 13, 2017

@d-frey It tooks a few attempts.

@ColinH
Copy link
Member

ColinH commented Mar 14, 2017

@kneth Thanks for keeping at it!

@d-frey d-frey merged commit 7f8cfee into taocpp:master Mar 14, 2017
@d-frey
Copy link
Member

d-frey commented Mar 14, 2017

Thanks Kenneth. I accepted the merge request and I'll now try to reunite it with the "old" .travis.yml.

@d-frey
Copy link
Member

d-frey commented Mar 14, 2017

Everything looks good so far, the new, combined .travis.yml is working as expected and I committed some cleanups. Quick question for our main README.md: It currently says "Android" in the bullet list in the Status section. I'd like to add some version number, but I have no clue about Android version numbers and what people usually use the refer to a version. I've seen 22 as well as r10e. What would, in your opinion, a better entry look like? "Android, Version 22"? "Android, NDK 22"?

@kneth
Copy link
Author

kneth commented Mar 15, 2017

API 22 is Android 5.1 (Lollipop), while r10e is NDK release 10e. They are not directly related. So the line might be "Android 5.1 (API 22) using NDK r10e".

@kneth kneth deleted the kneth/android-cmake-and-unittests branch March 15, 2017 08:09
@d-frey
Copy link
Member

d-frey commented Mar 16, 2017

I've updated the README, which leaves the question: Which is the minimal version required for the PEGTL to work? Is it possible and reasonable to find that out? Should we add multiple versions of Android and/or the NDK? If you have any insight/advice, please share it with us :)

@kneth
Copy link
Author

kneth commented Mar 17, 2017

I have built against API 9 (Android 2.3) using NDK r10e and executed tests on a HTC Desire S (Android 2.3.5) with success.

@d-frey
Copy link
Member

d-frey commented Mar 17, 2017

OK, thanks. I think this means that the API version is not really important for the PEGTL as we are not using much from the system, we only need good-enought C++11 support and that depends on the NDK? Any idea if earlier NDKs work or if they are still relevant?

@kneth
Copy link
Author

kneth commented Mar 17, 2017

As I recall, gcc 4.9 has been part of NDK through the r10 series so it is probably safe to say NDK r10 or later.

@d-frey
Copy link
Member

d-frey commented Mar 17, 2017

I will leave the entry as is then. Thanks again for all your work on PEGTL+Android! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants