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

Fix requirements error messages for JDK 8 #620

Merged
merged 3 commits into from
Jan 17, 2019

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Jan 16, 2019

Fixing the error message in response to discussion in #618.

I am not a super JDK 8 expert, would appreciate some feedback if my proposal looks good or not.

P.S. Fixed in another place in response to #619 (comment).

@codecov-io
Copy link

codecov-io commented Jan 16, 2019

Codecov Report

Merging #620 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #620   +/-   ##
======================================
  Coverage    64.7%   64.7%           
======================================
  Files          18      18           
  Lines        1816    1816           
======================================
  Hits         1175    1175           
  Misses        641     641
Impacted Files Coverage Δ
bin/templates/cordova/lib/check_reqs.js 50% <0%> (ø) ⬆️

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 634f92e...b280494. Read the comment docs.

@brodycj brodycj changed the title Fix requirements error message for JDK 8 Fix requirements error messages for JDK 8 Jan 16, 2019
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.

LGTM

@brodycj brodycj merged commit d9c08f1 into apache:master Jan 17, 2019
@brodycj brodycj deleted the fix-jdk-8-error-message branch January 17, 2019 15:13
rtpHarry added a commit to rtpHarry/ionic-docs that referenced this pull request Jan 24, 2019
It's not just recommended, it's required. I tried this today and there is a flat out version check in the source code that locks it to JDK8.

Also, added https to the download link as the site runs on that now.

FYI if you don't have much time to research this then here is some help:

I also worked with the cordova-android team to clarify this in their error messages and it's been accepted:

 - apache/cordova-android#619
 - apache/cordova-android#620

If you look in the files changed the version check is just around those changed lines.
mhartington pushed a commit to ionic-team/ionic-docs that referenced this pull request Feb 4, 2019
)

It's not just recommended, it's required. I tried this today and there is a flat out version check in the source code that locks it to JDK8.

Also, added https to the download link as the site runs on that now.

FYI if you don't have much time to research this then here is some help:

I also worked with the cordova-android team to clarify this in their error messages and it's been accepted:

 - apache/cordova-android#619
 - apache/cordova-android#620

If you look in the files changed the version check is just around those changed lines.
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