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

Simplify Android build and bring up to date #273

Merged
merged 12 commits into from Jan 5, 2018
Merged

Conversation

@ignatk
Copy link
Contributor

ignatk commented Dec 29, 2017

This PR improves Themis Android build:

  • updates used Android build tools to latest versions
  • adds x86_64 build architecture (now the default for Android native code builds)
  • checks-in BoringSSL as a submodule to Themis as recommended by BoringSSL project: https://boringssl.googlesource.com/boringssl/+/HEAD/INCORPORATING.md
  • integrates BoringSSL build to main Themis build, so no separate "build BoringSSL" step needed
  • bumps API level to 21 for better support of 64 bit platforms

The PR also includes days of messing with Circle CI to ensure it does not OOM with the new build system.

Relates to #235

@ignatk ignatk requested review from vixentael, Lagovas and storojs72 Dec 29, 2017
Copy link
Contributor

Lagovas left a comment

awesome, good job.it's great if after these changes tests will not down on android tests)

@@ -39,15 +46,21 @@ dependencies:
- sudo make rubythemis_install
- sudo make phpthemis_install
- if [ ! -d $BORINGSSL_PATH ]; then cd $HOME && git clone https://boringssl.googlesource.com/boringssl && cd boringssl && git checkout chromium-stable && mkdir build && cd build && cmake .. && make && cp decrepit/libdecrepit.a crypto/; fi
- if [ ! -d $BORINGSSL_PATH/build-armeabi-v7a ]; then cd $BORINGSSL_PATH && mkdir build-armeabi-v7a && cd build-armeabi-v7a && cmake -DANDROID_ABI=armeabi-v7a -DCMAKE_TOOLCHAIN_FILE=../third_party/android-cmake/android.toolchain.cmake -DANDROID_NATIVE_API_LEVEL=16 -GNinja .. && ninja -j 20; fi

This comment has been minimized.

Copy link
@Lagovas

Lagovas Dec 29, 2017

Contributor

as I understand here run tests with different android abi. we don't need it more?

This comment has been minimized.

Copy link
@ignatk

ignatk Dec 29, 2017

Author Contributor

The tests used to run (and still are running) with only one ABI. The ABI depends on emulator we start in the CI (which is arm 32 bit ATM). The compilation is done for all ABIs however.

@Lagovas

This comment has been minimized.

Copy link
Contributor

Lagovas commented Dec 29, 2017

as I see now tests running ~50 minutes and gradlew download dependencies before building (... Download https://dl.google.com/dl/android/maven2/com/android/tools/external/org-jetbrains/uast/26.0.1/uast-26.0.1.pom ...). Can we cache something of that (external archives/files/tools) to decrease time of tests?

@ignatk

This comment has been minimized.

Copy link
Contributor Author

ignatk commented Dec 29, 2017

Most of the time is taken not by downloading dependencies, but building BoringSSL. Because of Circle CI memory limit I had to almost disable concurrent compilation, so it takes ages to compile BoringSSL for 4 architectures with 2 flavour build (Debug, Release) - totally 8 builds.

@vixentael

This comment has been minimized.

Copy link
Member

vixentael commented Jan 2, 2018

Thank you @secumod! These changes are great and very welcomed!

However, could you please help me building Android tests on macOS? I've pulled your branch to init dependences, installed SDK Platform-Tools 27, SDK Build Tools 27 and CMake using Android Studio.

When executing command ./gradlew build, I receive following error:

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':boringssl:generateJsonModelDebug'.
> Build command failed.
  Error while executing process /Users/vxtl/Library/Android/sdk/cmake/3.6.4111459/bin/cmake with arguments {-H/Users/vxtl/projects/themis-test/mainfork/third_party/boringssl/src -B/Users/vxtl/projects/themis-test/mainfork/third_party/boringssl/.externalNativeBuild/cmake/debug/mips64 -DANDROID_ABI=mips64 -DANDROID_PLATFORM=android-21 -DCMAKE_LIBRARY_OUTPUT_DIRECTORY=/Users/vxtl/projects/themis-test/mainfork/third_party/boringssl/build/intermediates/cmake/debug/obj/mips64 -DCMAKE_BUILD_TYPE=Debug -DANDROID_NDK=/Users/vxtl/Library/Android/sdk/ndk-bundle -DCMAKE_TOOLCHAIN_FILE=/Users/vxtl/Library/Android/sdk/ndk-bundle/build/cmake/android.toolchain.cmake -DCMAKE_MAKE_PROGRAM=/Users/vxtl/Library/Android/sdk/cmake/3.6.4111459/bin/ninja -GAndroid Gradle - Ninja -DCMAKE_TOOLCHAIN_FILE=/Users/vxtl/Library/Android/sdk/ndk-bundle/build/cmake/android.toolchain.cmake -DANDROID_NATIVE_API_LEVEL=21 -DANDROID_TOOLCHAIN=gcc -DCMAKE_BUILD_TYPE=Release -GNinja}
  -- The C compiler identification is GNU 4.9.0
  -- Check for working C compiler: /Users/vxtl/Library/Android/sdk/ndk-bundle/toolchains/mips64el-linux-android-4.9/prebuilt/darwin-x86_64/bin/mips64el-linux-android-gcc
  -- Check for working C compiler: /Users/vxtl/Library/Android/sdk/ndk-bundle/toolchains/mips64el-linux-android-4.9/prebuilt/darwin-x86_64/bin/mips64el-linux-android-gcc -- works
  -- Detecting C compiler ABI info
  -- Detecting C compiler ABI info - done
  -- Detecting C compile features
  -- Detecting C compile features - done
  -- The CXX compiler identification is GNU 4.9.0
  -- Check for working CXX compiler: /Users/vxtl/Library/Android/sdk/ndk-bundle/toolchains/mips64el-linux-android-4.9/prebuilt/darwin-x86_64/bin/mips64el-linux-android-g++
  -- Check for working CXX compiler: /Users/vxtl/Library/Android/sdk/ndk-bundle/toolchains/mips64el-linux-android-4.9/prebuilt/darwin-x86_64/bin/mips64el-linux-android-g++ -- works
  -- Detecting CXX compiler ABI info
  -- Detecting CXX compiler ABI info - done
  -- Detecting CXX compile features
  -- Detecting CXX compile features - done
  CMake Error at CMakeLists.txt:339 (message):
    Unknown processor:mips64

This corresponds to the following line in CMakeLists.txt inside BoringSSL repo.

I found one Stack Overflow issue with similar question, but I don't think we were supposed to build BoringSSL for mips64, were we?

As far as I can say, there is no such line in CircleCI build output, I don't think that CircleCI was compiling BoringSSL for mips64.

Moreover, I've noticed that submodule is linked to master branch, but circle.yml is configured to download chromium-stable branch. However, changing submodule to use chromium-stable branch doesn't solve the issue :/

@vixentael

This comment has been minimized.

Copy link
Member

vixentael commented Jan 2, 2018

I've specified ABI version adding the following line to the thrid_party/boringssl/build.gradle file:

"-DANDROID_ABI=armeabi-v7a"

This fixed compiling mips64 problem, but I can't push to your branch.

Is it a good fix?

@ignatk

This comment has been minimized.

Copy link
Contributor Author

ignatk commented Jan 2, 2018

Circle CI is not building mips64. AFAIK, currently if you don't specify architectures, Android build system should build x86, x86_64, armeabi-v7a and arm64.

The fact that your environment tries to build mips64 may be some override in your configs. Adding "-DANDROID_ABI=armeabi-v7a" is probably not a valid fix, because you are configuring boringssl build for one architecture only.

As for branches, I noticed we used to use chromium-stable, but I explicitly changed it to master for Android (and probably we should change for regular PCs) according to https://groups.google.com/a/chromium.org/forum/#!topic/security-dev/coiM8IZ8Fsc

@vixentael

This comment has been minimized.

Copy link
Member

vixentael commented Jan 2, 2018

Right, in my case I get incompatible target error

  [x86] SharedLibrary  : libthemis_jni.so
  /Users/vxtl/Library/Android/sdk/ndk-bundle/toolchains/x86-4.9/prebuilt/darwin-x86_64/bin/../lib/gcc/i686-linux-android/4.9.x/../../../../i686-linux-android/bin/ld: error: /Users/vxtl/projects/themis-test/mainfork/jni/../third_party/boringssl/.externalNativeBuild/cmake/debug/x86/crypto/libcrypto.a(bcm.c.o): incompatible target

How to explicitly set boringssl architecture omitting mips64?

Should I have several cmake statements for each architecture?

    defaultConfig {
        minSdkVersion 16
        targetSdkVersion 16
        externalNativeBuild {
		    cmake {
		        arguments "-DCMAKE_TOOLCHAIN_FILE=" + android.ndkDirectory + "/build/cmake/android.toolchain.cmake",
		                  "-DANDROID_NATIVE_API_LEVEL=21",
		                  "-DANDROID_TOOLCHAIN=gcc",
		                  "-DCMAKE_BUILD_TYPE=Release",
                          "-DANDROID_ABI=armeabi-v7a",
		                  "-GNinja"
		    }
            cmake {
                arguments "-DCMAKE_TOOLCHAIN_FILE=" + android.ndkDirectory + "/build/cmake/android.toolchain.cmake",
                          "-DANDROID_NATIVE_API_LEVEL=21",
                          "-DANDROID_TOOLCHAIN=gcc",
                          "-DCMAKE_BUILD_TYPE=Release",
                          "-DANDROID_ABI=x86",
                          "-GNinja"
            }
            cmake {
                arguments "-DCMAKE_TOOLCHAIN_FILE=" + android.ndkDirectory + "/build/cmake/android.toolchain.cmake",
                          "-DANDROID_NATIVE_API_LEVEL=21",
                          "-DANDROID_TOOLCHAIN=gcc",
                          "-DCMAKE_BUILD_TYPE=Release",
                          "-DANDROID_ABI=arm64-v8a",
                          "-GNinja"
            }
            cmake {
                arguments "-DCMAKE_TOOLCHAIN_FILE=" + android.ndkDirectory + "/build/cmake/android.toolchain.cmake",
                          "-DANDROID_NATIVE_API_LEVEL=21",
                          "-DANDROID_TOOLCHAIN=gcc",
                          "-DCMAKE_BUILD_TYPE=Release",
                          "-DANDROID_ABI=x86_64",
                          "-GNinja"
            }
	    }
    }
@ignatk

This comment has been minimized.

Copy link
Contributor Author

ignatk commented Jan 2, 2018

I would prefer to rely on defaults - as I mentioned above, check your build environment why is it building mips64. Maybe, try to upgrade Android NDK to the latest version?

See https://developer.android.com/ndk/guides/abis.html
mips and mips64 are deprecated in NDK r16, so probably you are using an older one.

@vixentael

This comment has been minimized.

Copy link
Member

vixentael commented Jan 2, 2018

Thank you, currently I'm waiting untill local build finishes, then I'll try to update NDK.

According to the documentation:

ANDROID_ABI - specifies the target Application Binary Interface (ABI). 
This option nearly matches to the APP_ABI variable used by ndk-build tool from Android NDK. 
If not specified then set to armeabi-v7a.
@ignatk

This comment has been minimized.

Copy link
Contributor Author

ignatk commented Jan 2, 2018

That variable is set by Android gradle build system (depending on the project config), so no need to specify it manually.

@vixentael

This comment has been minimized.

Copy link
Member

vixentael commented Jan 5, 2018

I've updated NDK, it helped to solve the issue with building on mips64, however, I still see various error while building. I'll clean environment, themis folder and put here error log.

@vixentael

This comment has been minimized.

Copy link
Member

vixentael commented Jan 5, 2018

Good news, I've built .aar archives successfully.

I'll run tests using connectedAndroidTest, run our example code, and merge PR after that.

@vixentael

This comment has been minimized.

Copy link
Member

vixentael commented Jan 5, 2018

@secumod can you please review build instructions on wiki page?

I'm planning to add requirements (like installing SDK Platform-Tools 27, SDK Build Tools 27, downloading cmake via Android Studio) and short note about using submodules (like git submodule update --remote).

Please add notes about building themis_jni.dylib with new BoringSSL location

What else you would add?

@vixentael vixentael merged commit c42c452 into cossacklabs:master Jan 5, 2018
2 checks passed
2 checks passed
ci/bitrise/b32b4ea8bffedad7/pr Passed - themis
Details
ci/circleci Your tests passed on CircleCI!
Details
@vixentael

This comment has been minimized.

Copy link
Member

vixentael commented Jan 5, 2018

merged.

@secumod please update docs

@vixentael

This comment has been minimized.

Copy link
Member

vixentael commented Jan 6, 2018

@secumod if I understand correctly, we run tests on Android 22. However latest version is Android 27.

Test case are written using AndroidTestCase which is deprecated since Android 24.
https://developer.android.com/reference/android/test/AndroidTestCase.html

Trying to run tests on emulator 24+, I constantly receive error:

com.android.builder.testing.ConnectedDevice > No tests found.[Nexus_27(AVD) - 8.1.0] FAILED 
No tests found. This usually means that your test classes are not in the form that your test runner expects (e.g. don't inherit from TestCase or lack @Test annotations).

I believe we should update Android Tests to target to the latest Android version. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.