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.

Sign up for GitHub

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

Jump to bottom

Quick fix to support ANDROID_SDK_ROOT #656

Merged
merged 2 commits into from
Feb 12, 2019
Merged

Quick fix to support ANDROID_SDK_ROOT #656

merged 2 commits into from
Feb 12, 2019

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Feb 8, 2019 •
edited
Loading

This is a quick fix. Purpose is that upcoming major release of cordova-android should not depend on deprecated behavior as discussed in #617.

TODO:

  • NEEDS TO BE TESTED now tested on my own dev system
  • review

FUTURE TODO:

  • cleanup
  • more elegant

@codecov-io
Copy link

Codecov Report

Merging #656 into master will decrease coverage by 0.05%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #656      +/-   ##
==========================================
- Coverage   64.66%   64.61%   -0.06%     
==========================================
  Files          18       18              
  Lines        1817     1820       +3     
==========================================
+ Hits         1175     1176       +1     
- Misses        642      644       +2
Impacted Files Coverage Δ
bin/templates/cordova/lib/check_reqs.js 49.5% <25%> (-0.25%) ⬇️

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 73692e6...7f1bfe1. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Feb 8, 2019 •
edited
Loading

Codecov Report

Merging #656 into master will decrease coverage by 0.05%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #656      +/-   ##
==========================================
- Coverage   64.66%   64.61%   -0.06%     
==========================================
  Files          18       18              
  Lines        1817     1820       +3     
==========================================
+ Hits         1175     1176       +1     
- Misses        642      644       +2
Impacted Files Coverage Δ
bin/templates/cordova/lib/check_reqs.js 49.5% <25%> (-0.25%) ⬇️

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 73692e6...bedaf90. Read the comment docs.

janpio
janpio reviewed Feb 9, 2019
View reviewed changes
bin/templates/cordova/lib/check_reqs.js

if (!String(values[0]).startsWith('1.8.')) {
throw new CordovaError(
'Requirements check failed for JDK 8 (\'1.8.*\')! Detected version: ' + values[0] + '\n' +
'Check your JAVA_HOME / ANDROID_HOME / PATH environment variables.'
'Check your ANDROID_SDK_ROOT / JAVA_HOME / PATH environment variables.'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Check your ANDROID_SDK_ROOT / JAVA_HOME / PATH environment variables.'
'Check your `ANDROID_SDK_ROOT` and `JAVA_HOME` PATH environment variables.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not 100% convinced by this suggestion. It is possible that a developer could set a PATH environment variable that already includes the Android SDK bin directory.

Copy link
Member

Choose a reason for hiding this comment

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

ah, then I misread the meaning.

Suggested change
'Check your ANDROID_SDK_ROOT / JAVA_HOME / PATH environment variables.'
'Check your `ANDROID_SDK_ROOT`, `JAVA_HOME` and `PATH` environment variables.'

@janpio
Copy link
Member

janpio commented Feb 9, 2019

Is this WIP or should this be reviewed? Please decide.

@janpio
Update bin/templates/cordova/lib/check_reqs.js
bedaf90 
Co-Authored-By: brodybits <chris.brody@gmail.com>
@brodycj brodycj changed the title [WIP] Quick fix to support ANDROID_SDK_ROOT WIP (NOT TESTED) Quick fix to support ANDROID_SDK_ROOT Feb 11, 2019
@brodycj
Copy link
Contributor Author

brodycj commented Feb 11, 2019

Now tested, with one of the suggestions applied and my comment left on the other one. I would like to request a quick review, hope we can merge it soon.

@brodycj brodycj mentioned this pull request Feb 12, 2019
Merged
1 task
@oliversalzburg
Copy link

LGTM

janpio
janpio approved these changes Feb 12, 2019
View reviewed changes
Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

Looks good in theory, we'll see if it also works with all the different installations out there.

@brodycj brodycj merged commit b4de6f5 into apache:master Feb 12, 2019
@brodycj brodycj deleted the android-sdk-home-quick-fix branch February 12, 2019 14:20
@brodycj brodycj mentioned this pull request Feb 14, 2019
Closed
erisu added a commit to erisu/cordova-android that referenced this pull request Feb 16, 2019
@erisu
Updated ANDROID_HOME Test to Follow apache#656 Change
af99160
erisu added a commit that referenced this pull request Feb 18, 2019
@erisu
Updated ANDROID_HOME Test to Follow #656 Change (#673)
576edb5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janpio janpio janpio approved these changes

@dpogue dpogue Awaiting requested review from dpogue

@erisu erisu Awaiting requested review from erisu

@jcesarmobile jcesarmobile Awaiting requested review from jcesarmobile

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brodycj @codecov-io @janpio @oliversalzburg