-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
chore(android): Update projects to use Java 11 #8543
Conversation
User Test ResultsTest specification and instructions
Test Artifacts
|
I assume this is in order to continue maintenance of Keyman for Android 16.0? Any other reasons of note or causes to be aware of before we fully convert over at some point in the future? |
sdk=31 |
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.
TODO: update the comment (since we can use something newer now - yay!)
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.
Done. Also bumped sdk higher to 33.
android/build.sh
Outdated
@@ -37,6 +37,10 @@ builder_describe \ | |||
|
|||
builder_parse "$@" | |||
|
|||
# Override JAVA_HOME to OpenJDK 11 | |||
builder_heading "Setting JAVA_HOME to OpenJDK 11" | |||
export JAVA_HOME=${JAVA_HOME_11} |
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.
So, a problem here:
- If the build fails, I doubt that
JAVA_HOME
will be restored properly given the current implementation of this build script.- To be extra-explicit, this is the requested change.
It may be wise to extend the existing builder trap (because only one may be set...) and add functionality there to ensure that JAVA_HOME
is fixed before re-throwing an exit-triggering exit code.
For the in-repo reference, refer to _builder_failure_trap
via CTRL-F. It'd probably be best to simply write your own trap and have it call this one, and then replace this one with your version.
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.
JAVA_HOME
doesn't need to be reset -- you don't modify the environment for the caller of the script with export
, only with child processes. (You can only modify 'parent' environment with source
d scripts -- which run in the same process not a child process.)
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.
Ah, good to hear. I inferred that it did based on the implementation I reviewed, but didn't investigate it with any serious depth. Glad to hear it's not actually going to be an issue.
Right - 16.0 is still building with JDK 8. |
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.
This is good but I think we should tweak the JAVA_HOME_11
usage a bit.
android/KMAPro/build.sh
Outdated
@@ -81,7 +81,7 @@ fi | |||
if builder_start_action configure; then | |||
|
|||
# Copy Keyman Engine for Android | |||
cp "$KEYMAN_ROOT/android/KMEA/app/build/outputs/aar/${CONFIG}/keyman-engine.aar" "$KEYMAN_ROOT/android/KMAPro/kMAPro/libs/keyman-engine.aar" | |||
cp "$KEYMAN_ROOT/android/KMEA/app/build/outputs/aar/keyman-engine-${CONFIG}.aar" "$KEYMAN_ROOT/android/KMAPro/kMAPro/libs/keyman-engine.aar" |
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.
Per earlier discussions (#8542), this should be done in build
action.
android/build-publish.sh
Outdated
if [ -f .build-builder ]; then | ||
builder_heading "Setting JAVA_HOME to OpenJDK 11" | ||
export JAVA_HOME=${JAVA_HOME_11} | ||
fi |
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.
Can we move this to a function in build-utils.sh?
@@ -341,6 +346,17 @@ To include UserDefines.mak in the build, use the command line parameter | |||
`-DUSERDEFINES`. You can also set an environment variable `USERDEFINES=1` to get | |||
the same result. | |||
|
|||
### JAVA_HOME |
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 wonder if this whole section should be in a non-platform-specific doc?
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.
Per @mcdurdin's remarks about build-script environment variable handling, my concerns re JAVA_HOME
are satisfied now.
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.
LGTM.
Changes in this pull request will be available for download in Keyman version 17.0.84-alpha |
Fixes #2293 (Updating to Java 11)
Also fixes #8542 by moving some configure steps to build actions
After deferring multiple releases, this updates the Android projects to use Java 11 instead of Java 8.
For CI compatibility, the build.sh will override JAVA_HOME for to JAVA_HOME_11 and then revert to JAVA_HOME_8 when the build is finished.
One notable Gradle change is the Keyman Engine for Android artifact name changes
from KMEA/app/build/outputs/aar/${CONFIG}/keyman-engine.aar
to KMEA/app/build/outputs/aar/keyman-engine-${CONFIG}.aar"
The build also flagged
notificationManager.notify
needing to catch SecurityException.CI Test configs updated to use the top level Android build.sh script (handles updating JAVA_HOME to JDK 11 for the build).
TODO:
User Testing
Setup - Install the PR builds of "Keyman for Android" and "FirstVoices for Android"