-
Notifications
You must be signed in to change notification settings - Fork 144
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
Desktop Java builds with Gradle #633
Conversation
Introduce a subproject ":android" for AndroidThemis builds. We can't continue using top-level build.gradle for everything if we are going to use Gradle for Desktop Java too. This changes the invocation strings for targets which now need to be qualified (by prefixing ":android:..."). Leave the top-level file with some common definitions. This allows to not write them in each subproject and to use "./gradlew" right from the repository root.
Add a subproject ":desktop" for 'Desktop Java' builds of JavaThemis. This will produce a JAR with Java bytecode only, suitable for use by desktop Java projects. You can build it with ./gradlew :desktop:build and collect artifacts from "src/wrappers/themis/java/build/jar". The resulting JAR file is named "java-themis" because the name "themis" is already taked by AndroidThemis.
Debian stable has upgraded its default Java version to 11. Gradle 4.X does not support Java 11. This version is supported only since 5.0. Upgrade to latest Gradle 5.X version. Note that *the latest* version branch is Gradle 6.X. We won't jump versions that fast. Gradle 5.X requires Java 8 (previously it required Java 7). This cuts some older systems that ship with only Java 7: - Debian 8 "Jessie" - oldoldstable, no security support since June 2018, LTS support lapses in July 2020 (Only for JavaThemis, obviously.)
By default Gradle will configure all subprojects of a top-level project. For us that means that we'll force AndroidThemis configuration even if the user is trying to build JavaThemis for desktop. That will require a properly installed and configured Android SDK, completely unnecessary. Thankfully, Gradle also have an experimental "configure on demand" mode which will configure only the projects actually requested and needed for the build. Enable this feature. It is 'experimental' but this means that it might not work for some projects, but Gradle team plans to make this mode the default one. It works for us, so it's okay.
We build JavaThemis with deprecation warnings enabled. Object finalizers are deprecated since Java 9. In our case we genuienly need finalizers to prevent native memory leaks so suppress these warnings.
The "build" target already runs tests if they are set up. Configure JUnit test compilation and runner. Note that JavaThemis requires JNI library to be loaded. By default it is installed into a location that is not present in the Java library search path. We need to explicitly add "/usr/local/lib" to "java.library.path" property. Also note that Gradle will run JUnit stuff in a separate JVM so we need to pass the system property to that JVM from the main one.
Every class having native methods should have a System.loadLibrary() call so that if only that class is loaded by JVM, the native library is also loaded correctly. It kinda worked before because typically users and tests load other classes, but if *only* SymmetricKey is loaded then it failed to locate the native method. (For example, JUnit might run each test in separate JVMs which load only necessary classes.)
I was pleasantly surprised to discover that we have already ported tests to JUnit in #557. It's a quick-and-dirty port, but it kinda works. So I only had to add them to Gradle for desktop Java. Running
will also run tests now. Note that JNI library must be in Java's search path. Since we install it to
|
You know that it was you who ported tests, right? :D |
|
||
buildscript { | ||
dependencies { | ||
classpath 'com.android.tools.build:gradle:3.2.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought, we moved to gradle 5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh... To be honest, I'm not sure how this affects the build. Maybe it tells Gradle to use this compatibility version? This line has been like this before the file got moved, when we were using Gradle 4.6. Running ./gradlew wrapper --gradle-version 5.6
upgraded only wrapper, it did not touch this line.
I guess it's worth looking into. Maybe we're using Gradle 5.6 to be able to run with Java 11, but the build still uses 3.2.1 mode. I'm not sure we need to upgrade that if it works fine, but I guess I can try and see what happens. Maybe we don't even need this line at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither I know what this line indicates.. Maybe minimum gradle version?
If Android studio / IDEA doesn’t warn about this, we can leave it as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's a different thing after all. There is Gradle as a command-line build tool (we use 5.6) and there is Gradle library with various runtime thingies. That line specifies the latter.
The project opens fine in Android Studio:
It builds successfully, runs tests, eats 5+ GB of disk space for SDK & NDK, etc.
Android Studio suggests to update the version to 3.6.3 which is the latest stable one in Google's Maven repo, along with some other updates in Android unit test runner (1.1 → 1.2) and Gradle wrapper version (5.6 → 5.6.4). And for some reason requires an additional property after an update. The strategy “blindly apply whatever Android Studio suggests” has worked fine before 🤷
The build and tests pass, so I guess let's update all of that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The strategy “blindly apply whatever Android Studio suggests” has worked fine before 🤷
I use the same, never failed for me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it does fail now.
It seems that 3.6.X has its issues, suggesting a downgrade to 3.5.X. However, 3.5.X has different issues. FFS, Android ecosystem 🤷 Since this upgrade is discretionary and we don't have any specific reason to upgrade other than being up-to-date, let's keep using 3.2.1 which worked fine before.
(No need to say that on my machine all these versions work fine, but they fail on CI.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, Android ecosystem ¯_(ツ)_/¯
artifact("$buildDir/outputs/aar/projects-release.aar") | ||
groupId 'com.cossacklabs.com' | ||
artifactId 'themis' | ||
version '0.12.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a dream that we moved versions to a single source / constant...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about that because updating it in three places is too close to my personal DRY rule of extracting common code when there are at least three uses.
I guess it should be possible to move the version to Gradle properties (since we have the file now) and set it from there for all subprojects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had same thoughts. It’s not critical, but maybe if on some point we decide to have one version, it would be nice.
I was thinking if java-themis
and android-themis
can potentially have different versions, say 0.13.1 and 0.13.2? Due to different workarounds/hotfixes in each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if java-themis and android-themis can potentially have different versions, say 0.13.1 and 0.13.2?
Valid point. Typically, if we need to change the code then both will get an update, but there's also Android build system to consider. For example, if Android adds a new device architecture then it's a good idea to ship a patch update with the same code built for that new architecture, which only makes sense for Android.
Anyhow, I think it is beneficial to have the versions in one place just so that we don't have to run around the repository during regular releases. We can, for example, have two properties: javaThemisVersion
and androidThemisVersion
.
The properties seem to work (fdc0d23), I'm currently testing if they work for Android too, just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe keep the versions in individual build.gradle files, but extract them into a constant somewhere close to the top so that it's visible and quick to change? IMO, it makes equal sense to keep the version in the “project file”.
At least we won't be duplicating the string in the Android file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having property will increase our UX as maintainers significantly. When we will need different values, we’ll introduces two properties.
Also, pls don’t forget to update internal release guidelines section where we enumerate versions :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add two properties from the get go then.
Maybe in some parallel dimension the VERSION file looks like this:
Core: 0.13.0
GoThemis: 0.13.0
PyThemis: 0.13.1
JavaThemis: 0.13.2
And you don’t have to git grep ${current_minor_version}
to see if anything is missing an update 💤 ☁️ 🌈 🦄
pls don’t forget to update internal release guidelines section where we enumerate versions
Sure, will do.
} | ||
|
||
// Tweak the resulting JAR name so it's not called "java". | ||
archivesBaseName = 'java-themis' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart!
Definitely! Okay... Just to admit it, using “author's we” in commit comments and alike is a (bad?) habit of mine. I want to believe it's not ”royal we“, after all. |
I’m totally fine with your “using we” style, but I think that you deserve extra appreciation 🧡 |
Aww~ 🥺😭 |
Use common properties to have a single place we need to update when doing a release.
Update Gradle support library to the latest stable version. Update Gradle wrapper to the latest patch version too (required for some of others updates). And update the Android test runners as well. New test runners require AndroidX property to be enabled in the properties file so add that as well.
Suggested by Android Studio too.
Android Studio recommends using current latest stable version 3.6.3, but it seems to have some issues with NDK version detection. Since we have different NDK versions in our CI environments, we can't use the version which fails to work with them. Gradle tools v3.6.3 fail the build and require a particular version of NDK, for example: * What went wrong: A problem occurred configuring project ':android'. > No version of NDK matched the requested version 20.0.5594570. Versions available locally: 21.0.6113669 Version 3.5.X does not seem to have such issues, so let's use it then.
It seems that 3.5.3 does not work either. I don't want to spend more time on this discretionary upgrade, so just revert to the version which is known to work and let's keep it pinned that way until we need to upgrade to have some new features or bug fixes.
Make it possible to build JavaThemis for desktop/server environment with Gradle.
will now build a JAR file with Java code in
src/wrappers/themis/java/build/jar
, currently calledjava-themis-0.12.jar
.This is not really used anywhere right now, but it is a stepping stone for properly published JavaThemis.
Note that this builds only Java code. It still requires a JNI library to run which has to be installed separately. Since Themis 0.13 it will be possible to install it as
libthemis-jni
via system package managers.Deprecations and breaking changes
AndroidThemis builds now need to be qualified
Desktop Java build is added to the root Gradle project as a subproject
:desktop
. AndroidThemis build has been moved from the root into the:android
subproject.This means that to build AndroidThemis it is now recommended to use qualified form:
The unqualified form still works:
but it may behave unexpectedly with other targets, acting on both desktop and mobile Themis. The documentation currently recommends this form, but we should revise this once Themis 0.13 is released.
Java 7 is no longer supported
Modern, supported versions of systems have updated their default Java version to 11. Unfortunately, Gradle 4 does not support Java 11 and fails to build on those systems. It is usually possible to install Java 8 there and switch to using it, but we ought to support out-of-the-box builds better.
Embedded Gradle version has been updated to Gradle 5.6 which requires Java 8 to run. This means that systems not having Java 8 are no longer able to build AndroidThemis with embedded Gradle. We no longer consider the following systems supported:
If you still use these systems and need to build AndroidThemis from source, please upgrade your build environments. You can also try installing an older Gradle version (4.X) and use it instead of embedded Gradle wrapper, but that's not guaranteed to work from now on.
Checklist