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!: ANDROID_HOME is the new default, to check first and give advice #1471

Merged
merged 1 commit into from
Apr 9, 2023

Conversation

ath0mas
Copy link
Contributor

@ath0mas ath0mas commented Jul 31, 2022

Platforms affected

Android

Motivation and Context

cordova-android 11.0.0 (since #1444) reports about Android SDK: undefined while ANDROID_HOME is set and ANDROID_SDK_ROOT is not.

Description

cordova create hello com.example.hello HelloWorld
cd hello/
cordova platform add [email protected]
cordova build

with environment variables set for Java (JAVA_HOME) and Android (ANDROID_HOME, and no more ANDROID_SDK_ROOT)

results in a successful build but with wrong reporting during the first steps:

Checking Java JDK and Android SDK versions
ANDROID_HOME=C:\my\path\to\android-sdk (recommended setting)
ANDROID_SDK_ROOT=undefined (DEPRECATED)
Using Android SDK: undefined

Testing

  • creating and building a blank project using local clone of my fork of cordova-android
  • npm run test

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2022

Codecov Report

Merging #1471 (b06f3db) into master (60e3803) will not change coverage.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master    #1471   +/-   ##
=======================================
  Coverage   72.25%   72.25%           
=======================================
  Files          21       21           
  Lines        1748     1748           
=======================================
  Hits         1263     1263           
  Misses        485      485           
Impacted Files Coverage Δ
lib/check_reqs.js 71.05% <50.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us.

Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

lgtm -- but I'm not sure if this can be considered a patch.

It could be considered a breaking change... particularly if they did had ANDROID_HOME set, but to an invalid value. Therefore, we may have to wait until we prepare a new major release before this can be merged.

@breautek breautek added this to the 12.0.0 milestone Oct 1, 2022
@breautek breautek changed the title fix: ANDROID_HOME is the new default, to check first and give advice fix!: ANDROID_HOME is the new default, to check first and give advice Oct 1, 2022
@breautek breautek requested a review from erisu March 15, 2023 14:30
@breautek
Copy link
Contributor

breautek commented Apr 8, 2023

Final call for review before merging.

@breautek breautek merged commit 841710e into apache:master Apr 9, 2023
@breautek
Copy link
Contributor

breautek commented Apr 9, 2023

Thank you @ath0mas for helping us keep up with Android tooling standards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants