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

CB-14101 Fix Java version check for Java >= 9 #446

Merged
merged 2 commits into from
Jun 12, 2018

Conversation

raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Jun 3, 2018

Platforms affected

Android

What does this PR do?

  • Fix Java version check for Java >= 9
  • Checks that we have exactly Java 1.8 since nothing else works with the Android SDK
  • Tell the user that we need exactly Java 1.8 if we can't find it

What testing has been done on this change?

The relevant function was not covered by unit tests to begin with, so I only tested this manually with the following code:

Promise.resolve(
  require('./bin/templates/cordova/lib/check_reqs').check_java()
).then(console.log)

Output for Java 8 is 1.8.0, for Java 10 is 10.0.1.

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.

Added automated test coverage as appropriate for this change

@raphinesse
Copy link
Contributor Author

I actually wanted to use semver for this, but with the dependency bundling it was too much of a hassle.

@raphinesse
Copy link
Contributor Author

Added node 4 compatibility so we can release this in a patch release

@codecov-io
Copy link

codecov-io commented Jun 3, 2018

Codecov Report

Merging #446 into master will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
+ Coverage   44.16%   44.21%   +0.04%     
==========================================
  Files          17       17              
  Lines        1698     1694       -4     
  Branches      314      312       -2     
==========================================
- Hits          750      749       -1     
+ Misses        948      945       -3
Impacted Files Coverage Δ
bin/templates/cordova/lib/check_reqs.js 48.57% <0%> (+0.44%) ⬆️

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 02ee925...22d10bb. Read the comment docs.

This also checks that we have exactly 1.8 since nothing else works with
the Android SDK. The user facing error was updated accordingly.
@raphinesse
Copy link
Contributor Author

Actually, there's a bug 🤕

@raphinesse
Copy link
Contributor Author

raphinesse commented Jun 3, 2018

Now it should be fine. Test coverage is a great thing 😅

@raphinesse raphinesse merged commit bf29fe0 into apache:master Jun 12, 2018
@raphinesse raphinesse deleted the check-reqs branch June 12, 2018 20:19
raphinesse added a commit to brodycj/cordova-android that referenced this pull request Jun 20, 2018
This also checks that we have exactly 1.8 since nothing else works with
the Android SDK. The user facing error was updated accordingly.
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Jul 4, 2018
This also checks that we have exactly 1.8 since nothing else works with
the Android SDK. The user facing error was updated accordingly.
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Jul 10, 2018
This also checks that we have exactly 1.8 since nothing else works with
the Android SDK. The user facing error was updated accordingly.
brodycj pushed a commit that referenced this pull request Jul 11, 2018
This also checks that we have exactly 1.8 since nothing else works with
the Android SDK. The user facing error was updated accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants