-
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
refactor!: drop support for android
SDK tool
#1083
refactor!: drop support for android
SDK tool
#1083
Conversation
25c4741
to
605c482
Compare
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.
Looks good to me!
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.
lgtm
Tested adding the platform and building/running on emulator using one of my apps.
Thanks for the review everyone! Since this is a breaking change for next major, how do we want to continue with this PR?
Side note: I will probably need some integration branch for multiple breaking changes anyway |
This might be a good idea for testing PRs against the collection of other breaking changes to be released. |
`emulator.list_images` now always uses the `avdmanager` binary.
`android_sdk.list_targets` now always uses the `avdmanager` binary.
605c482
to
caf8a3e
Compare
Codecov Report
@@ Coverage Diff @@
## master #1083 +/- ##
==========================================
- Coverage 71.41% 70.50% -0.91%
==========================================
Files 20 20
Lines 1770 1719 -51
==========================================
- Hits 1264 1212 -52
- Misses 506 507 +1
Continue to review full report at Codecov.
|
* refactor(emulator)!: remove support for legacy `android` binary `emulator.list_images` now always uses the `avdmanager` binary. * refactor(android_sdk)!: remove support for legacy `android` binary `android_sdk.list_targets` now always uses the `avdmanager` binary. * refactor(check_reqs)!: do not look for legacy `android` binary * refactor: replace installation instructions involving `android` binary
Motivation and Context
We have multiple places where we support both the legacy
android
SDK tool as well asavdmanager
andsdkmanager
from current SDK tools. This makes code more complicated and maintenance more difficult.This PR removes the support for the
android
CLI tool from our code base. After this change, to usecordova-android
users will need either:Closes #595 #741
Description
emulator.list_images
now always uses theavdmanager
binary.android_sdk.list_targets
now always uses theavdmanager
binary.check_reqs.check_android
does not consider legacyandroid
binary for SDK detection anymoreandroid
binary anymoreThere are multiple opportunities for further simplification after the code removal in this PR. I already have seized some of these in a local branch but I would prefer to open new PRs for these after this is merged.
Testing
node -e "require('./bin/templates/cordova/lib/emulator').list_images().then(console.log)"
node -e "require('./bin/templates/cordova/lib/android_sdk').list_targets().then(console.log)"
node -e "require('./bin/templates/cordova/lib/check_reqs').check_android().then(console.log)"