Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

Update README and build.sh script to use latest env var naming. #34

Merged
merged 1 commit into from Mar 21, 2022

Conversation

kirillzh
Copy link
Contributor

Description

ANDROID_HOME, ANDROID_SDK_HOME, and ANDROID_NDK_HOME are not used by the Android platform and SDK tools so these are considered deprecated/invalid.

Google recommends using ANDROID_SDK_ROOT and ANDROID_NDK_ROOT instead:

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

ANDROID_HOME, ANDROID_SDK_HOME, and ANDROID_NDK_HOME are not used by the Android platform and SDK tools so these are considered deprecated/invalid.

Google recommends using ANDROID_SDK_ROOT and ANDROID_NDK_ROOT instead:
- https://groups.google.com/g/android-ndk/c/qZjhOaynHXc/m/2ux2ZZdxy2MJ
- https://developer.android.com/studio/command-line/variables
@artfuldev
Copy link
Contributor

I'd suggest raising an issue before raising a PR (in general to any open source project).

Here's a document that talks about ANDROID_NDK_HOME and why/how it's used in the Android Gradle Plugin (AGP):
https://github.com/android/ndk-samples/wiki/Configure-NDK-Path#android_ndk_home

Here's a small table listing versions where ANDROID_NDK_HOME was available, then deprecated, and finally removed (from the same page):
image

ndkPath configuration is the new way (since AGP 4.1, it used to be ndk.dir before then) - but since we're using CLI tools and installers, when we didn't set these variables, we used to get these errors that these specific variables were not defined, which is why it's present in the build script and the README. We do not want to force people on to new versions of build tools unless required.

Just adding a little bit of context. 🙂

@thunderbiscuit
Copy link
Collaborator

thunderbiscuit commented Mar 21, 2022

Thanks for the links @artfuldev, good stuff.

I'm trying to understand your comment though: are you saying the ANDROID_NDK_HOME should stay because it's required when building from the command line?

I'm looking at the docs you linked to:

The replacement for ANDROID_NDK_HOME is the modern_ ndkVersion way_: configure NDK in your modules, and install NDK to $SDK/ndk/$version

For CI purposes, the environmental variable ANDROID_NDK_HOME(deprecated) might be useful when building on the command line

Gradle does check ANDROID_NDK_HOME to find NDK if no local.properties file exists. You could make ANDROID_NDK_HOME into your project’s ndkPath, for example, in module gradle file: ndkPath = System.getenv("ANDROID_NDK_HOME")

When testing locally the build works well using the new variables defined in this PR (ROOT instead of HOME), but I'm not sure if that will be a problem for the CI. Is that what you were implying?

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 16e6a4b

I can confirm what @artfuldev mentioned that with older AGP we were getting a warning if we didn't have the ANDROID_NDK_HOME set, but at this point I think it's safe to assume anyone building this will have the latest tools. So we should use the latest correct and consistent env var naming.

@notmandatory
Copy link
Member

Also related, GitHub hosted runners set both old and new env variable names but uses the 4.0 version of the CLI tools so should also not show any warnings.

https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#environment-variables-3

@artfuldev artfuldev merged commit fa1b94d into bitcoindevkit:master Mar 21, 2022
@artfuldev
Copy link
Contributor

Thank you @kirillzh !

@kirillzh kirillzh deleted the update-android-env-vars branch March 24, 2022 00:13
@notmandatory notmandatory added this to the Release 0.6.0 milestone Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants