refactor: java checks / get java version

[breautek]

Platforms affected

Android

Motivation and Context

Prerequisite PR for #1060

This PR abstracts the Java checks and java version retrieval so that it can be reused.

Description

Created a new Java class with static methods that provides an API to get the version. It also contains the code that finds the java executable that used to live inside check_reqs.

Java.js is contains 100% line coverage, which is an upgrade from when the code lived in check_reqs.

Testing

npm test

Checklist

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
• I've updated the documentation if necessary

[erisu]

Do we really want the name of the file as Java.js and not java.js? I believe in almost every repo our filenames are lowercased.

Since this is introducing a new coding style, I think you should document when to use uppercase vs. lowercase and put a document somewhere like the cordova-contribute so we can understand these new cases.

[raphinesse]

Do we really want the name of the file as Java.js and not java.js? I believe in almost every repo our filenames are lowercased.

We actually have quite a few modules with non-lowercase filenames in this repo that exactly match the exported class' name: AndroidManifest.js, AndroidProject.js, ...

That being said, if we change this module from a class to a plain object, java.js would be fine with me as well. In fact, maybe the name could be more descriptive yet. Something like env/java.js, for example... 🤷‍♂️

[codecov-io]

Codecov Report

Merging #1130 (c645ca3) into master (3081e5e) will increase coverage by 1.46%.
The diff coverage is 100.00%.

❗ Current head c645ca3 differs from pull request most recent head a3d2f6c. Consider uploading reports for the commit a3d2f6c to get more accurate results

@@            Coverage Diff             @@
##           master    #1130      +/-   ##
==========================================
+ Coverage   71.80%   73.27%   +1.46%     
==========================================
  Files          21       22       +1     
  Lines        1745     1766      +21     
==========================================
+ Hits         1253     1294      +41     
+ Misses        492      472      -20     

Impacted Files	Coverage Δ
bin/templates/cordova/lib/check_reqs.js	75.73% <100.00%> (+5.49%)	⬆️
bin/templates/cordova/lib/env/java.js	100.00% <100.00%> (ø)
bin/templates/cordova/lib/utils.js	95.23% <100.00%> (+2.38%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3081e5e...a3d2f6c. Read the comment docs.

[raphinesse]

Great to see improvements to the check_reqs module! Especially the separation of environment checking and inference is something I thought should have been done for a long time.

I made a quick pass over the code, left a few comments and pushed a fix for one of the new tests. Overall, this looks quite good to me. I hope I can take a closer look at it soon.

[raphinesse]

I added some more comments. I hope I didn't go overboard 😅

[breautek]

That being said, if we change this module from a class to a plain object, java.js would be fine with me as well. In fact, maybe the name could be more descriptive yet. Something like env/java.js, for example...

Coding habits from my own projects. I can convert this into a simple JS object and move the file to an env/ folder. We could follow a similar pattern going forward with android studio, adb, bundle tools, etc...

I would want to keep the module exporting an object containing methods though as this makes it easy to unit test/spy functions without the use of rewire, and I personally want to try to limit the use of rewire as much as possible since it's not a package that is actually maintained.

[raphinesse]

This PR looks pretty good to me 👍

My suggestions and comments are mainly cosmetic. Feel free to implement or discard those as you see fit.

The few non-cosmetic remarks are not a deal breaker either for me. So if there's no time to address them, that's fine with me too.

Sorry for taking so long with the review but I'm pretty swamped with work currently.

[breautek]

@erisu @raphinesse

Finally got around into finishing this up, I believe this could be incorporated into 9.1.x release, or do you think it should wait until 10.x?

[erisu]

IMO, I think this can be released with the 9.1.0 minor release.

[DavidStrausz]

Hi @breautek, I hope it's okay if I add this here, but I can also open a new issue if you need one.
This causes the requirements check to fail on macOS Big Sur 11.2.3 if custom java options (_JAVA_OPTIONS) are set.

For reference in my case I set them in .zshrc like this:

export _JAVA_OPTIONS="-Xms2048M -Xmx4096M"

And this is the exact error:

Checking Java JDK and Android SDK versions
ANDROID_SDK_ROOT=/Users/username/Library/Android/sdk (recommended setting)
ANDROID_HOME=undefined (DEPRECATED)
Requirements check failed for JDK 1.8.x! Detected version: 2048.0.0
Check your ANDROID_SDK_ROOT / JAVA_HOME / PATH environment variables.

The reason for this is happening is because in the new implementation you are using javac -version to get the version, this prints the java options before printing the version like this:

Picked up _JAVA_OPTIONS: -Xms2048M -Xmx4096M
javac 1.8.0_271

The result is directly checked via semver without sanitizing the output first. In the old implementation a regular expression was used to only check the relevant parts:

const match = /javac\s+([\d.]+)/i.exec(output);

https://github.com/apache/cordova-android/pull/1130/files#diff-0d1afa0d5c88bdb18fbc6ab9ef7fc4bbe10f7047d387bb2964932dbb6eb8c4b7L198

[raphinesse]

@DavidStrausz thanks for letting us know. Would you care to try if this problem can be resolved by adding the options env: {}, extendEnv: false to the execa call in getVersion?

If so, we surely would appreciate a pull request with that change.

[DavidStrausz]

@raphinesse Thanks for your response! Interesting, didn't think of that way of fixing it yet, I will take a look sometime next week and will open a PR if this indeed resolves the issue!
If it does not work, I will readd the regex in my PR.