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 support to specify an Android SDK in ANDROID_HOME (fixes #463) #1307

Merged
merged 4 commits into from
Jun 12, 2023

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Jun 11, 2023

Changes

  • Fixes Add support for ANDROID_HOME environment variable #463
  • The file path in ANDROID_HOME now takes precedence over ANDROID_SDK_ROOT
  • The eventual file path for the SDK used is passed both as ANDROID_HOME and ANDROID_SDK_ROOT when Android commands are run in the shell.
  • Behavior summary:
    • No environment variables set
      • Briefcase's SDK is used
    • Only ANDROID_HOME is set
      • If it's an SDK, it is silently used
      • If it's not an SDK, a warning is issued it isn't an SDK and Briefcase's SDK is used
    • Only ANDROID_SDK_ROOT is set
      • If it's an SDK, a warning is issued that ANDROID_SDK_ROOT is deprecated but it is used
      • If it's not an SDK, a warning is issued it isn't an SDK and Briefcase's SDK is used
    • Both ANDROID_HOME and ANDROID_SDK_ROOT are set to the same value
      • Falls back to as if only ANDROID_HOME was set
    • Both ANDROID_HOME and ANDROID_SDK_ROOT are set to different values
      • A warning is issued that ANDROID_HOME takes precedence over ANDROID_SDK_ROOT
      • If ANDROID_HOME is a valid SDK, it is used
      • If ANDROID_HOME is not a valid SDK, a warning is issued it isn't an SDK and Briefcase's SDK is used

Notes

  • I broke with the recommended implementation because I think it can imposition the user too much.
  • For instance, erroring if ANDROID_HOME and ANDROID_SDK_ROOT are set to different file paths.
    • This isn't inherently problematic for Briefcase...at least based on my implementation.
    • However, erroring like this would require users to change their environment just to run Briefcase.
  • As part of this, I am also setting the SDK file path in both ANDROID_HOME and ANDROID_SDK_ROOT for subprocess calls
    • This is primarily because if the user has a bad setting for ANDROID_SDK_ROOT and Briefcase only sets ANDROID_HOME, the ANDROID_SDK_ROOT setting will leak in to subprocess calls.....therefore, the user would need to unset ANDROID_SDK_ROOT to be able to run Briefcase.
    • So, it seems reasonable to me for Briefcase to simply mask this scenario since we already have a known valid SDK.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16 rmartin16 marked this pull request as ready for review June 11, 2023 19:45
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I've pushed some minor language tweaks, but otherwise this looks good.

In terms of the specific behavior you've implemented - those changes both makes sense to me. Setting both environment variables removes all doubt over the environment that is used at runtime; and warning the user about a setting inconsistency but recovering acts as a canary that something isn't right while not getting in the way of progress.

I'll leave the final approval to Malcolm in case I've missed something in my analysis, but as far as I'm concerned, I'm happy.

docs/reference/platforms/android.rst Outdated Show resolved Hide resolved
src/briefcase/integrations/android_sdk.py Outdated Show resolved Hide resolved
@mhsmith mhsmith merged commit 5c25cf9 into beeware:main Jun 12, 2023
@rmartin16 rmartin16 deleted the android-home branch June 13, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for ANDROID_HOME environment variable
3 participants