Update check_reqs.js - allow JDK > 1.8

dkotrada commented 4 years ago

Checking JDK should not fail when there is another version installed like 11.0.6 for example. Redme file declares clearly

Requires

Java JDK 1.8 or greater

Motivation and Context

I have another verison installed. It fails but shouldn't.

breautek commented 4 years ago

Java JDK 1.8 or greater

Is actually wrong. Android requires 1.8, not anything lower, not anything greater. So if the readme states this, it probably should be changed. But I would favour the README change in a separate PR.

jcesarmobile commented 4 years ago

As Norman said, Android requires Java 8, so the docs should be changed, not the code.

dkotrada commented 4 years ago

Java JDK 1.8 or greater

Is actually wrong. Android requires 1.8, not anything lower, not anything greater. So if the readme states this, it probably should be changed. But I would favour the README change in a separate PR.

I am not native speaker so you may not understood me what I mean with this PR.

With my code you can use any java version. Who has today lower than 1.8? Next LTS version is 11. So my code works well with both 1.8 and greather current code is wrong because it checks only for 1.8. But cordova works with greather versions.

But you can do what ever you want. Have a nice day.

jcesarmobile commented 4 years ago

I'm reopening so can be tested, but supposedly Android only works with Java 8. But maybe, as long as we don't use newer Java API features, it could still compile with newer SDK versions.

breautek commented 4 years ago

But maybe, as long as we don't use newer Java API features, it could still compile with newer SDK versions.

I'm pretty sure this is the case. The android documentation says that the target compile needs to be set to jdk 8, which is what we define in gradle.

https://developer.android.com/studio/write/java8-support

So we cannot use features that only appear in later versions. I still think it's safer to stick with java8, but perhaps the checks can be less restrictive, which I believe is what this PR tries to do.

I would like to see some tests added to ensure the checks returns appropriately when it finds older versions of java and when it finds newer versions of java.

And if we go that route, then I guess this will make https://github.com/apache/cordova-android/pull/929 obsolete.

breautek commented 4 years ago

I just tested openJDK 11 on ubuntu and the tests was passed.. so I think was indeed wrong about the android sdk strictly requiring java 8.

If we are going to open up that requirements check though, I think we should have a suite of tests for both java8 and java11 in the github actions.

GuilleW commented 4 years ago

Hi ! Same problem here. I'm on Debian GNU/Linux bullseye/sid with openjdk-11-jdk and openjdk-8-jdk installed. In my .bashrc, I have : export JAVA_HOME=/usr/lib/jvm/java-1.8.0-openjdk-amd64 and my JDK 8 is not used by cordova-android build.

Actually, I patch this file manually to [197] return Q.denodeify(child_process.exec)(process.env['JAVA_HOME'] + '/bin/javac -version')

Or ... can we set a ['CORDOVA_JAVA'] env variable so everyone can use last update of java, and a specific version for cordova ?

GuilleW commented 4 years ago

Sorry, I just saw that the NPM package cordova-android : 8.1.0 is not updated since 8 months, so ... my suggestion is already outdated.

breautek commented 4 years ago

Nope, i dont think you're suggestion is out dated.

I'm not sure which route we should take, but there are two different routes imo:

1) this route this PR addresses, which opens up the check to allow java 8+, including java11. Android sdk specifically requires 8, but I don't think that means you need jdk8... because we actually do explicitly say to target/compile against java 8 in the gradle configs. In this route, I think we should have tests with both jdk8 and jdk11 environments. I'm personally not aware of any cons of using the jdk11.

2) we keep the restrictive jdk check (that is, must require jdk 8), but provide a cordova environment variable like you suggested so users can have both jdk versions installed at the same time. Cordova will check for the cordova java home variable, then fall back to the generic JAVA_HOME variable. This route is probably the safer option.

GuilleW commented 4 years ago

IMO, option 2 is safer and doesn't introdruce new bugs if it works great with JDK8. With this option, we have time to test JDK11.

breautek commented 3 years ago

On this note, I did add java 11 to the test CI on my fork, and it all appears to pass: https://github.com/breautek/cordova-android/runs/676508506?check_suite_focus=true

Laubeee commented 3 years ago

is there any quick fix to this? I have multiple JREs/JDKs and even if my JAVA_HOME points to jdk1.8, java -version gives me 1.8, I still get the error

Requirements check failed for JDK 8 ('1.8.*')! Detected version: 11.0.4

breautek commented 3 years ago

even if my JAVA_HOME points to jdk1.8

Improvements were made with how java is detected, so this may be fixed in cordova-android@nightly. Can you test? We are looking to release cordova-android@9 soon, but I can't give any specific ETA.

GuilleW commented 3 years ago

I can't build with nightly. TL;DR look at the last $ cordova build

See below...

`$ cordova create hello com.example.hello HelloWorld`

Creating a new cordova project.

`$ cd hello`

`$ cordova platform add android`

Using cordova-fetch for cordova-android@^8.0.0
Adding android project...
Creating Cordova project for the Android platform:
    Path: platforms/android
    Package: com.example.hello
    Name: HelloWorld
    Activity: MainActivity
    Android target: android-28
Subproject Path: CordovaLib
Subproject Path: app
Android project created with cordova-android@8.1.0
Plugin 'cordova-plugin-whitelist' found in config.xml... Migrating it to package.json
Discovered saved plugin "cordova-plugin-whitelist". Adding it to the project
Installing "cordova-plugin-whitelist" for android
Adding cordova-plugin-whitelist to package.json

`$ cordova requirements`

Requirements check results for android:
Java JDK: installed 11.0.7
Android SDK: installed true
Android target: installed android-28
Gradle: installed /home/guillew/App/gradle/gradle-6.4/bin/gradle

`$ cordova build`

Checking Java JDK and Android SDK versions
ANDROID_SDK_ROOT=/home/guillew/App/android-sdk (recommended setting)
ANDROID_HOME=/home/guillew/App/android-sdk (DEPRECATED)
Requirements check failed for JDK 8 ('1.8.*')! Detected version: 11.0.7
Check your ANDROID_SDK_ROOT / JAVA_HOME / PATH environment variables.

Update to nightly

`$ npm i cordova-android@nightly`

npm WARN com.example.hello@1.0.0 No repository field.

+ cordova-android@9.0.0-nightly.2020.5.29.a830145f
added 39 packages from 40 contributors, removed 6 packages, updated 12 packages and audited 88 packages in 6.784s

2 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

`$ cordova build`

Unable to load PlatformApi from platform. Error: Cannot find module 'shelljs'
Require stack:
- /home/guillew/hello/platforms/android/cordova/lib/pluginHandlers.js
- /home/guillew/hello/platforms/android/cordova/lib/AndroidProject.js
- /home/guillew/hello/platforms/android/cordova/Api.js
- /usr/lib/node_modules/cordova/node_modules/cordova-lib/src/cordova/util.js
- /usr/lib/node_modules/cordova/node_modules/cordova-lib/src/platforms/platforms.js
- /usr/lib/node_modules/cordova/node_modules/cordova-lib/src/plugman/install.js
- /usr/lib/node_modules/cordova/node_modules/cordova-lib/src/plugman/plugman.js
- /usr/lib/node_modules/cordova/node_modules/cordova-lib/cordova-lib.js
- /usr/lib/node_modules/cordova/src/help.js
- /usr/lib/node_modules/cordova/src/cli.js
- /usr/lib/node_modules/cordova/bin/cordova
Unhandled error. ('The platform "android" does not appear to be a valid cordova platform. It is missing API.js. android not supported.')

Laubeee commented 3 years ago

even if my JAVA_HOME points to jdk1.8

Improvements were made with how java is detected, so this may be fixed in cordova-android@nightly. Can you test? We are looking to release cordova-android@9 soon, but I can't give any specific ETA.

Hey, sorry, I started editing my original post but it seems I forgot to actually post it (or didnt work)

I got it to work. there was still a ref to javac 11 in the path before 1.8 (but not for java), so when I rearranged some more path vars it worked. Looking forward to see this handled in a better way in the next release! thx

brodybits commented 3 years ago

I would like to get this fixed, would definitely agree with @timbru31 to check for JDK < 8.

raphinesse commented 3 years ago

It would be great if we could finally allow builds using the latest SDKs!

I remember that I thought this was possible quite some time ago. But when I did some tests in preparation of a PR it appeared as if the newer SDKs did break the build after all. Maybe I just messed up something else instead. I really hope so.

brodybits commented 3 years ago

As I said in this thread on the mailing list([1]), I was able to use OpenJDK 13 to build and install a couple of non-Cordova Android apps from the command line, including a React Native project. (OpenJDK 13 also worked for me with building and installing https://github.com/sqlcipher/sqlcipher-android-tests from the command line.)

A side point is that we are using Gradle a little differently than most other Android projects, as discussed in the same thread ([1]).

So yes, I think we should update to allow JDK > 1.8, properly, with proper review and testing. I took the liberty to update the title again to better reflect the actual change proposed here.

[1] https://lists.apache.org/thread.html/ra54447f23a6df77128dbec69ad3130281221e8a1b724c5ceb02052db%40%3Cdev.cordova.apache.org%3E

raphinesse commented 3 years ago

Here's the problem I encountered and still do when using Java > 8:

$ echo $JAVA_HOME
/usr/lib/jvm/java-11-openjdk-amd64

$ sdkmanager --version # same Exception for avdmanager and android
Exception in thread "main" java.lang.NoClassDefFoundError: javax/xml/bind/annotation/XmlSchema
    at com.android.repository.api.SchemaModule$SchemaModuleVersion.<init>(SchemaModule.java:156)
    at com.android.repository.api.SchemaModule.<init>(SchemaModule.java:75)
    at com.android.sdklib.repository.AndroidSdkHandler.<clinit>(AndroidSdkHandler.java:81)
    at com.android.sdklib.tool.sdkmanager.SdkManagerCli.main(SdkManagerCli.java:73)
    at com.android.sdklib.tool.sdkmanager.SdkManagerCli.main(SdkManagerCli.java:48)
Caused by: java.lang.ClassNotFoundException: javax.xml.bind.annotation.XmlSchema
    at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581)
    at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
    at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
    ... 5 more

$ export JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64

$ sdkmanager --version
26.1.1

An explanation of the Exception can be found at stackoverflow. Unfortunately there seems to be no workaround for Java 11. It might work with later Android SDK tools. But I don't know what we support exactly.

This issue does not have to block this PR, since users should be able to use latest JDK if it works for them and the SDKs they are using. But we should maybe have an idea of what will and what will not work together, so we can estimate the impact of allowing newer JDK versions and maybe warn users or catch errors appropriately.

breautek commented 3 years ago

Strange. Is it possible you're still using the old command line tools?

Android now offers command line tools as a separate package that you need to install. Perhaps this is updated for java 11+ support.

https://stackoverflow.com/a/58652345/4685664

raphinesse commented 3 years ago

@breautek Yeah, it's been some time since I did Android development, so I still have the tools that come with the SDK (or you install via the sdkmanager?). I did not know there were new tools. If this is an unsupported or deprecated setup we don't have to worry. Hopefully we call the right tools from cordova-android if there are multiple versions. Probably depends on the user's PATH setup etc.

breautek commented 3 years ago

I haven't been doing a whole lot of mobile development stuff at my workplace lately, but I'll install JDK 11 and see what kind of issues I run into with. My SDK environment is relatively up to date with everything as well.

Edit: Actually I'm seeing similar issues running JDK 11 as well, even Android Studio GUI doesn't work properly for me when I have JAVA_HOME pointing to JDK 11.

Not sure why my fork had all the tests passing, perhaps most of these android studio calls are mocked...?

https://github.com/breautek/cordova-android/runs/676508506?check_suite_focus=true

Edit 2:

Yah, quick glance at the unit tests, it looks like most android sdk calls are mocked. So it looks like my original interpretation of Android SDK strictly supports java 8 is correct I think...

breautek commented 3 years ago

Thank you for your PR, but I'm certain that Android SDK tools doesn't play nicely with any java version above 8.

You can get Java11 working with the android tools, but it requires some hackery. Depending on this is not an acceptable solution, it should work out of the box.

So unfortunately, I don't see supporting java11 in the foreseeable future. This is something we can return to once Android Studio/Android SDK Tools supports java11, or greater.

raphinesse commented 3 years ago

Just some more details from my side:

Even the latest (6609375) Android Command Line Tools did not work properly for me using Java 11. While avdmanager seemed to work, sdkmanager always only showed the usage help preceded by this error:

Warning: Could not create settings
java.lang.IllegalArgumentException
    at com.android.sdklib.tool.sdkmanager.SdkManagerCliSettings.<init>(SdkManagerCliSettings.java:428)
    at com.android.sdklib.tool.sdkmanager.SdkManagerCliSettings.createSettings(SdkManagerCliSettings.java:152)
    at com.android.sdklib.tool.sdkmanager.SdkManagerCliSettings.createSettings(SdkManagerCliSettings.java:134)
    at com.android.sdklib.tool.sdkmanager.SdkManagerCli.main(SdkManagerCli.java:57)
    at com.android.sdklib.tool.sdkmanager.SdkManagerCli.main(SdkManagerCli.java:48)

Benji1 commented 3 years ago

I wanted to switch from an old docker image to circleci/android:api-28-node but they recently switched to openjdk 11 and in that article there is a link to another article, which claims that:

Even Android Studio and the Android SDK — one of the last holdouts — works with OpenJDK 11 out of the box.

So I wanted to know if maybe @aahlenst (who wrote this article) has a trick up his sleeve and can show us what could be done to make it work.

aahlenst commented 3 years ago

To make things work:

You need the new commandlinetools, 6514223 or newer should work.
You need to place the tool directory inside in /opt/android/cmdline-tools (when /opt/android is the SDK root).
You need to update the paths, especially /opt/android/cmdline-tools/tools/bin (when /opt/android is the SDK root) must come before everything else. This is because there are 2 sdkmanager binaries in the complete SDK and the old one in tools/bin does not work. It produces the stacktrace quoted above.

Install instructions:

$ unzip commandlinetools-linux-6514223_latest.zip 
$ mkdir -p /opt/android/cmdline-tools
$ mv tools /opt/android/cmdline-tools
$ export PATH="/opt/android/cmdline-tools/tools/bin:$PATH"

Then, use sdkmanager to install all required dependencies.

PATH configuration:

export PATH="/opt/android/tools:$PATH"
export PATH="/opt/android/tools/bin:$PATH"
export PATH="/opt/android/platform-tools:$PATH"
export PATH="/opt/android/emulator:$PATH"
export PATH="/opt/android/cmdline-tools/tools/bin:$PATH"

export ANDROID_SDK_ROOT="/opt/android"

Benji1 commented 3 years ago

Thanks for the fast answer!

GuilleW commented 3 years ago

I use a faster way, just sed the check_reqs.js file to really follow the JAVA_HOME env variable:

Fix JAVA_HOME not detected: https://github.com/apache/cordova-android/pull/928

sed -i 's/\x27javac -version/process.env\[\x27JAVA_HOME\x27\] + \x27\/bin\/javac -version/g' src-cordova/platforms/android/cordova/lib/check_reqs.js

I run this command from my VueJS project folder. You should probably fix the path.

apache / cordova-android