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

Android: don't require deprecated tools #21736

Merged

Conversation

anders-nylander
Copy link
Contributor

@anders-nylander anders-nylander commented Mar 17, 2022

Checking for these deprecated is no longer necessary, and infact broken on fresh Android SDK installs. Remove the check.

resolves #21735

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake
force_builders_only=Android armeabi-v7a,docs

@alalek
Copy link
Member

alalek commented Mar 18, 2022

Patch should not introduce regressions of used configurations.

"Android armeabi-v7a" build has the following setup:

--   Android NDK:                   /opt/android/android-ndk-r18b (ver 18.1.5063045)
--     Android ABI:                 armeabi-v7a
--     NDK toolchain:               arm-linux-androideabi-clang
--     STL type:                    c++_static
--     Native API level:            21
--   Android SDK:                   /opt/android/android-sdk.gradle (tools: 26.1.1 build tools: 28.0.3)

(all of these dependencies are likely from 2018-2019)


It would be nice if you could take a look how to support both configurations.

@dkurt dkurt added this to the 4.8.0 milestone May 18, 2023
@dkurt dkurt force-pushed the fix_android_sdk_tools_dependency branch from 87a1c13 to fd69ea8 Compare May 18, 2023 09:46
@dkurt dkurt requested a review from opencv-alalek May 18, 2023 10:10
@dkurt dkurt changed the title Remove check for deprecated Android SDK tools. Add check for Android SDK cmdline-tools May 18, 2023
@dkurt
Copy link
Member

dkurt commented May 23, 2023

@asmorkalov, @opencv-alalek, please take a look at updated changes.

if(DEFINED ANDROID_SDK AND EXISTS "${ANDROID_SDK}/tools")
set(ANDROID_SDK_TOOLS "${ANDROID_SDK}/tools" CACHE INTERNAL "Android SDK Tools path")
if(DEFINED ANDROID_SDK)
if (EXISTS "${ANDROID_SDK}/cmdline-tools")
Copy link
Contributor

Choose a reason for hiding this comment

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

They are not the same and can't be exchanged. Just compare:

cmdline-tools/latest/source.properties:

Pkg.Revision=7.0
Pkg.Path=cmdline-tools;7.0
Pkg.Desc=Android SDK Command-line Tools

vs

tools/source.properties

Pkg.UserSrc=false
Pkg.Revision=26.1.1
Platform.MinPlatformToolsRev=20
Pkg.Dependencies=emulator
Pkg.Path=tools
Pkg.Desc=Android SDK Tools

OpenCV doesn't use "tools" with modern "gradle" flow actually. They are use with legacy "ant" build flow.


Try to comment out #ocv_detect_android_sdk_tools() call.

Copy link
Member

Choose a reason for hiding this comment

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

Replaced to a simple check that tools found.

cmdline-tools tools changed their versioning so the latest one is 9.0. More about my build steps here: #23642

@@ -173,12 +173,12 @@ if(BUILD_ANDROID_PROJECTS)
ocv_detect_android_sdk_tools()
ocv_detect_android_sdk_build_tools()

if(ANDROID_SDK_TOOLS_VERSION VERSION_LESS 14)
if(DEFINED ANDROID_SDK_TOOLS_VERSION AND ANDROID_SDK_TOOLS_VERSION VERSION_LESS 14)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please clean CMake cache and ensure that configuration process doesn't stop on the line 64.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, sorry for confusion. My sdk contained both tools and cmdline-tools. Needed reinstall.

@dkurt dkurt force-pushed the fix_android_sdk_tools_dependency branch from 63bba9f to d7c613c Compare May 24, 2023 20:22
@opencv-alalek opencv-alalek self-requested a review May 25, 2023 08:32
@opencv-alalek opencv-alalek changed the title Add check for Android SDK cmdline-tools Android: don't require deprecated tools May 25, 2023
Copy link
Contributor

@opencv-alalek opencv-alalek left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you!

@asmorkalov asmorkalov merged commit 87331ca into opencv:4.x May 25, 2023
21 checks passed
@asmorkalov asmorkalov mentioned this pull request May 31, 2023
thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024
…tools_dependency

Android: don't require deprecated tools opencv#21736

Checking for these deprecated is no longer necessary, and infact broken on fresh Android SDK installs. Remove the check.

resolves opencv#21735

### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [ ] The feature is well documented and sample code can be built with the project CMake
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.

Building OpenCV for Android using Android cmdline-tools
6 participants