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

feat: remove java 1.8 version check #1241

Merged
merged 2 commits into from Jul 7, 2021

Conversation

jcesarmobile
Copy link
Member

This is another approach to the java versions handling/requirement proposed here #1230

At the moment we require java 8, the linked PR will require java 11, but I think we don't really need to require any specific version (or well, maybe >= java 8, but I don't think people is using java 7 or older at this point).

So this PR just removes the java version check (it still checks that java is installed, but just "ignores" which version, as long as java is installed any version should be fine)

At the moment I have java 11 installed because I use the one that Android Studio includes and Android Studio 4.2 started using java 11 instead of java 8, but I can still run cordova apps from the CLI that supposedly require java 8, and I think that java versions are backward compatible and if an user had java 16 it should also work (but didn't try).

Even if we want to require java 11 and go with the linked PR, I think we should change the version check to >= 11 and not a strict 11, because some people work with java for server/desktop apps and use newer java versions and are forced to downgrade to use Cordova, when it looks like it's not really necessary.

@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2021

Codecov Report

Merging #1241 (9a5165d) into master (422ce4f) will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1241      +/-   ##
==========================================
- Coverage   71.62%   71.55%   -0.07%     
==========================================
  Files          21       21              
  Lines        1653     1649       -4     
==========================================
- Hits         1184     1180       -4     
  Misses        469      469              
Impacted Files Coverage Δ
bin/templates/cordova/lib/check_reqs.js 70.58% <ø> (-0.85%) ⬇️

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 422ce4f...9a5165d. Read the comment docs.

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR will require a rebase, but I had confirmed that the changes will work with current main branch.

I created a seperate branch & cherry-picked this commit for testing: https://github.com/erisu/cordova-android/tree/feat/java-11

Build Output:
https://gist.github.com/erisu/d6ee1859d3fecd1b9e8af1e5769b58e3

Gradle Wrapper Version Output:

$ ./platforms/android/gradlew -v

------------------------------------------------------------
Gradle 7.1.1
------------------------------------------------------------

Build time:   2021-07-02 12:16:43 UTC
Revision:     774525a055494e0ece39f522ac7ad17498ce032c

Kotlin:       1.4.31
Groovy:       3.0.7
Ant:          Apache Ant(TM) version 1.10.9 compiled on September 27 2020
JVM:          11.0.11 (AdoptOpenJDK 11.0.11+9)
OS:           Mac OS X 11.4 x86_64

I also confirm that JDK 1.8 is not affected from the changes.

@erisu erisu changed the title Remove java version check feat: remove java 1.8 version check Jul 7, 2021
@jcesarmobile jcesarmobile marked this pull request as ready for review July 7, 2021 09:59
@jcesarmobile jcesarmobile merged commit 8a9cb8f into apache:master Jul 7, 2021
@jcesarmobile jcesarmobile deleted the no-java-version branch July 7, 2021 11:19
wedgberto pushed a commit to wedgberto/cordova-android that referenced this pull request May 17, 2022
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.

None yet

3 participants