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

Add missing log to Java version check #624

Merged

Conversation

fabiante
Copy link
Contributor

Platforms affected

Every platform

What does this PR do?

Add a log that clarifies the Java validation

What testing has been done on this change?

No testing needed, this just adds a console.log call.

Checklist

@codecov-io
Copy link

codecov-io commented Jan 17, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #624   +/-   ##
=======================================
  Coverage   64.66%   64.66%           
=======================================
  Files          18       18           
  Lines        1817     1817           
=======================================
  Hits         1175     1175           
  Misses        642      642
Impacted Files Coverage Δ
bin/templates/cordova/lib/check_reqs.js 49.74% <0%> (ø) ⬆️
bin/templates/cordova/lib/emulator.js 89.62% <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 d9c08f1...33ffdc4. Read the comment docs.

@janpio
Copy link
Member

janpio commented Jan 17, 2019

Maybe the check_* function should output this information when it checks the actual values?

Copy link
Contributor

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

I liked the first commit (02296b9) better.

I do think it is good to add the statement that the user should check JAVA_HOME, PATH, and ANDROID_HOME settings.

As I said in the comments I am not so happy about not logging the actual environment variable values for the ANDROID_HOME setting. This is information that could really help someone figure out what is not working.

I think some things have been a little mixed up in check_reqs.js. There is module.exports.run that runs a combination of check_java and check_android, which seem to be designed to run independently and are also part of a longer list in module.exports.check_all. I think it would be good to clean this up a bit, but not before the coming Cordova 9 release discussed in apache/cordova#10.

Another thing we need to fix is that we should check ANDROID_SDK_ROOT, as discussed in #617. I think it would be ideal to get that in before Cordova, if possible.

In short, I would really favor keeping the updates simple and keep the existing log statements until we get a chance to make the updates to check ANDROID_SDK_ROOT and cleanup check_reqs.js.

Copy link
Contributor

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

I am now convinced that we should not log JAVA_HOME at this point. The follow other things still need to be fixed as I said before:

  • need to keep log of ANDROID_HOME path
  • error message for JDK 8 check failure needs to be fixed (fix indentation to pass eslint and split into multiple lines)

Copy link
Contributor

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

Thanks @fabiante for making the requested changes, merging now.

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

4 participants