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 a warning about the using of android.bat #595

Closed
wants to merge 3 commits into from

Conversation

Songbird0
Copy link

@Songbird0 Songbird0 commented Dec 2, 2018

Hi,

Platforms affected

Android.

Like it said:

**************************************************************************
The "android" command is deprecated.
For manual SDK, AVD, and project management, please use Android Studio.
For command-line tools, use tools\bin\sdkmanager.bat
and tools\bin\avdmanager.bat
**************************************************************************

What does this PR do?

The conditionnal structure has been removed to use systematically sdkmanager instead of android.bat.

See this issue for more.

What testing has been done on this change?

None. The method behaviour hasn't been significantly altered, actually.

Feel free to add/remove what you want. :)

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

Hi,

Like said:

```
**************************************************************************
The "android" command is deprecated.
For manual SDK, AVD, and project management, please use Android Studio.
For command-line tools, use tools\bin\sdkmanager.bat
and tools\bin\avdmanager.bat
**************************************************************************
```

The conditionnal has been removed to use systematically `sdkmanager` instead of `android.bat`.

See [this issue](https://github.com/apache/cordova/issues/61).
@Songbird0
Copy link
Author

{ CordovaError: Failed to find 'ANDROID_HOME' environment variable. Try setting it manually.
Failed to find 'android' command in your 'PATH'. Try update your 'PATH' to include path to valid SDK directory.
    at /home/travis/build/apache/cordova-android/bin/templates/cordova/lib/check_reqs.js:9:18722
    at _fulfilled (/home/travis/build/apache/cordova-android/node_modules/q/q.js:854:54)
    at self.promiseDispatch.done (/home/travis/build/apache/cordova-android/node_modules/q/q.js:883:30)
    at Promise.promise.promiseDispatch (/home/travis/build/apache/cordova-android/node_modules/q/q.js:816:13)
    at /home/travis/build/apache/cordova-android/node_modules/q/q.js:877:14
    at runSingle (/home/travis/build/apache/cordova-android/node_modules/q/q.js:137:13)
    at flush (/home/travis/build/apache/cordova-android/node_modules/q/q.js:125:13)
    at _combinedTickCallback (internal/process/next_tick.js:73:7)
    at process._tickCallback (internal/process/next_tick.js:104:9)
  name: 'CordovaError',
  message: 'Failed to find \'ANDROID_HOME\' environment variable. Try setting it manually.\nFailed to find \'android\' command in your \'PATH\'. Try update your \'PATH\' to include path to valid SDK directory.',
  code: 0,
  context: undefined }
FF.....
Failures:
1) check_reqs check_android set ANDROID_HOME if not set if some Android tooling exists on the PATH should set ANDROID_HOME based on `android` command if command exists in a SDK-like directory structure

Mh... doesn't sound good. Anyone have an idea of what tools use the android command?

@dpogue
Copy link
Member

dpogue commented Dec 2, 2018

This likely breaks compatibility with older versions of the Android SDK.

My changes were reverted to avoid compatibility breaking. A warning was added into the `cordova requirements` command text message.
@codecov-io
Copy link

codecov-io commented Dec 2, 2018

Codecov Report

Merging #595 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #595   +/-   ##
=======================================
  Coverage   62.24%   62.24%           
=======================================
  Files          17       17           
  Lines        1992     1992           
  Branches      371      371           
=======================================
  Hits         1240     1240           
  Misses        752      752
Impacted Files Coverage Δ
bin/templates/cordova/lib/check_reqs.js 48.57% <ø> (ø) ⬆️

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 8a4ae31...e003f04. Read the comment docs.

@Songbird0
Copy link
Author

If that's sound good for you, want you the branch be manually rebased by me or it will be rebased automatically before merging?

@Songbird0 Songbird0 changed the title Update getAbsoluteAndroidCmd method Add a warning about the using of android.bat Dec 2, 2018
@janpio janpio added this to 🐣 New PR / Untriaged in Apache Cordova: Platform Pull Requests Dec 8, 2018
@breautek
Copy link
Contributor

I don't really understand how useful the notice is.

Wouldn't it be better to remove usages of the android command? I realise this is 2 years old now... does this even apply still?

@brodybits
Copy link
Contributor

Wouldn't it be better to remove usages of the android command?

+1

I realise this is 2 years old now... does this even apply still?

Needs investigation, adding bug & discussion labels for now.

@erisu erisu closed this in #1083 Apr 13, 2021
Apache Cordova: Platform Pull Requests automation moved this from 🐣 New PR / Untriaged to ☠️ Closed/Abandoned Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants