-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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_SDK_ROOT variable #951
Conversation
Not sure why it's failling on mac, NodeJS on mac must behaves slightly different, will investigate tomorrow. Also pondering forcing
https://developer.android.com/studio/command-line/variables#android_sdk_root |
be7cfa8
to
7634087
Compare
Codecov Report
@@ Coverage Diff @@
## master #951 +/- ##
==========================================
+ Coverage 65.53% 66.10% +0.56%
==========================================
Files 21 21
Lines 1828 1838 +10
==========================================
+ Hits 1198 1215 +17
+ Misses 630 623 -7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Rebase PR
- Re-run the lint after rebasing. I believe there are some cases that will be caught with the new lint version.
- Revert all gradle changes and move those changes into its own PR
2b009d8
to
c715ca4
Compare
Putting this PR back to "WIP" since the latest rebase, this branch specifically does not pass a test for some reason... the test that is failing seems unrelated, so will need to investigate... Oddly enough, it doesn't fail in the other 2 branches that this PR depends on, so it is something specific to this branch. |
This commit does the following: - Makes ANDROID_SDK_ROOT the primary variable to look for the Android SDK location. - Makes ANDROID_HOME the fallback variable, if ANDROID_SDK_ROOT is not present/valid. Gradle updates: Note that the following gradle updates were required, otherwise the android gradle plugin did not honour the ANDROID_SDK_ROOT variable. - Updates the framework's android studio's gradle plugin from version 3.3.0 to 3.5.3. Not only this is required for android's gradle to obey ANDROID_SDK_ROOT, it is now in sync with the Android test project/ - Updates the Androidx test project to use gralde plugin from version 3.3.0 to 3.5.3, to match Android Test & framework. - Consequentially, this required to also upgrade AndroidX test project to use Gradle 6.1, which also matches both the Android test project & framework These changes above fixes apache#949 Additionally, since we update the environment variables dynamically, the environment variable printout produced misleading information. The environment variable printout will now print out the variable as defined by the user (before the tooling messes with them). An additional log is printed that tells the user exactly what Cordova is going to use for the Android SDK path. This should fix apache#670
c715ca4
to
28599f4
Compare
Platforms affected
Android
Motivation and Context
Fixes #949 which describes a functional issue
Fixes #670 which describes a cosmetic issue that is loosely related.
Description
This PR depends on two other PRs: #959 #960
This PR does the following:
Gradle updates:Note that the following gradle updates were required, otherwise the android gradle plugin did not honour the ANDROID_SDK_ROOT variable.Updates the framework's android studio's gradle plugin from version 3.3.0 to 3.5.3.Not only this is required for android's gradle to obey ANDROID_SDK_ROOT, it is now in sync with the Android test project/
Updates the Androidx test project to use gralde plugin from version 3.3.0 to 3.5.3, to match Android Test & framework.Consequentially, this required to also upgrade AndroidX test project to use Gradle 6.1, which also matches both the Android test project & frameworkGradle updates have been moved to independent PRs #959 #960
These changes above fixes #949
Additionally, since we update the environment variables dynamically, the environment variable printout produced misleading information.
The environment variable printout will now print out the variable as defined by the user (before the tooling messes with them). An additional log
is printed that tells the user exactly what Cordova is going to use for the Android SDK path. This should fix #670
Tests was also changed to mainly use
ANDROID_SDK_ROOT
instead ofANDROID_HOME
. Additional tests were added to ensure things works with either variable, butANDROID_SDK_ROOT
now takes precedence overANDROID_HOME
, should both be defined.Testing
npm test passes successfully, both old and new.
I've ran npm test in every combination of:
ANDROID_SDK_ROOT
/ withANDROID_HOME
ANDROID_SDK_ROOT
/ withoutANDROID_HOME
ANDROID_SDK_ROOT
/ withANDROID_HOME
ANDROID_SDK_ROOT
/ withoutANDROID_HOME
Additionally, I've tested each of these combinations with a hello world cordova test app, to ensure
build
runs successfully.This PR should be accompanied with a cordova-docs PR to update the Environment Variables documentation, which has not been done yet.
Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)