-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
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 default value of editor property export/android/android_sdk_path
for Windows, Linux, and macOS
#97992
Conversation
export/android/android_sdk_path
for Windows, Linux, and macOS
platform/linuxbsd/os_linuxbsd.cpp
Outdated
@@ -1180,6 +1180,10 @@ String OS_LinuxBSD::get_system_ca_certificates() { | |||
return f->get_as_text(); | |||
} | |||
|
|||
String OS_LinuxBSD::get_default_android_sdk_path() const { | |||
return OS::get_singleton()->get_environment("HOME") + "/Android/Sdk"; |
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.
Handle trailing slashes if present in the environment variable (very unlikely here, but could technically happen):
return OS::get_singleton()->get_environment("HOME") + "/Android/Sdk"; | |
return OS::get_singleton()->get_environment("HOME").path_join("Android/Sdk"); |
platform/macos/os_macos.mm
Outdated
@@ -763,6 +763,10 @@ | |||
return certs; | |||
} | |||
|
|||
String OS_MacOS::get_default_android_sdk_path() const { | |||
return OS::get_singleton()->get_environment("HOME") + "/Library/Android/Sdk"; |
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.
return OS::get_singleton()->get_environment("HOME") + "/Library/Android/Sdk"; | |
return OS::get_singleton()->get_environment("HOME").path_join("Library/Android/Sdk"); |
platform/windows/os_windows.cpp
Outdated
@@ -2004,6 +2004,10 @@ String OS_Windows::get_system_ca_certificates() { | |||
return certs; | |||
} | |||
|
|||
String OS_Windows::get_default_android_sdk_path() const { | |||
return OS::get_singleton()->get_environment("LOCALAPPDATA") + "\\Android\\Sdk"; |
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.
return OS::get_singleton()->get_environment("LOCALAPPDATA") + "\\Android\\Sdk"; | |
return OS::get_singleton()->get_environment("LOCALAPPDATA").path_join("Android\\Sdk"); |
platform/android/export/export.cpp
Outdated
@@ -52,7 +52,9 @@ void register_android_exporter() { | |||
#ifndef ANDROID_ENABLED | |||
EDITOR_DEF_BASIC("export/android/java_sdk_path", OS::get_singleton()->get_environment("JAVA_HOME")); | |||
EditorSettings::get_singleton()->add_property_hint(PropertyInfo(Variant::STRING, "export/android/java_sdk_path", PROPERTY_HINT_GLOBAL_DIR)); | |||
EDITOR_DEF_BASIC("export/android/android_sdk_path", OS::get_singleton()->get_environment("ANDROID_HOME")); | |||
|
|||
EDITOR_DEF_BASIC("export/android/android_sdk_path", OS::get_singleton()->has_environment("ANDROID_HOME") ? OS::get_singleton()->get_environment("ANDROID_HOME") : OS::get_singleton()->get_default_android_sdk_path()); |
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.
I wonder why the Android installers don't define ANDROID_HOME
to the user environment variables after installing Android Studio. If they consistently did this, then this step wouldn't be needed.
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.
I think it's due to Linux's mess in environment variables. While in windows they are stored compactly and conveniently in one place in registry, in Linux there plenty of places for both user and global variables. And what makes it even worse is the fact that this set of places depends on DE and shell you currently use (and maybe distro, if we're taking about global vars). Yes, we have "/etc/environment", but you need sudo to write to it. So there's very few examples of non system apps which automatically edit variables
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.
Yes, we have '/etc/environment', but you need sudo to write to it.
On macOS, it's not writable at all, there's no way to set global environment variables. ANDROID_HOME
can be set for the shell, but Godot usually is not started from the shell.
platform/android/export/export.cpp
Outdated
@@ -52,7 +52,9 @@ void register_android_exporter() { | |||
#ifndef ANDROID_ENABLED | |||
EDITOR_DEF_BASIC("export/android/java_sdk_path", OS::get_singleton()->get_environment("JAVA_HOME")); | |||
EditorSettings::get_singleton()->add_property_hint(PropertyInfo(Variant::STRING, "export/android/java_sdk_path", PROPERTY_HINT_GLOBAL_DIR)); | |||
EDITOR_DEF_BASIC("export/android/android_sdk_path", OS::get_singleton()->get_environment("ANDROID_HOME")); | |||
|
|||
EDITOR_DEF_BASIC("export/android/android_sdk_path", OS::get_singleton()->has_environment("ANDROID_HOME") ? OS::get_singleton()->get_environment("ANDROID_HOME") : OS::get_singleton()->get_default_android_sdk_path()); |
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.
Yes, we have '/etc/environment', but you need sudo to write to it.
On macOS, it's not writable at all, there's no way to set global environment variables. ANDROID_HOME
can be set for the shell, but Godot usually is not started from the shell.
|
||
String get_default_android_sdk_path() { | ||
#ifdef WINDOWS_ENABLED | ||
return OS::get_singleton()->get_environment("LOCALAPPDATA").path_join("Android/Sdk"); |
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.
I replaced backslash with forward slash because String::path_join()
method uses these slashes as well (So in any case sdk path on Windows will contain both back and forward slashes, because Windows' environment variables always use backslashes.
@@ -37,6 +37,8 @@ | |||
#include "editor/editor_settings.h" | |||
#include "editor/export/editor_export.h" | |||
|
|||
String get_default_android_sdk_path(); |
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.
Can you move this method to EditorPaths
for consistency.
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.
I actually think this should be in platform/android/export/export.cpp
as it's specific to the Android export pipeline and we should aim to keep solutions local.
platform/macos/os_macos.h
Outdated
@@ -126,6 +126,7 @@ class OS_MacOS : public OS_Unix { | |||
virtual Error move_to_trash(const String &p_path) override; | |||
|
|||
virtual String get_system_ca_certificates() override; | |||
|
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.
Can be reverted?
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!
Can you squash the commits.
2f51dc1
to
3411a26
Compare
export/android/android_sdk_path
for Windows, Linux, and macOSexport/android/android_sdk_path
for Windows, Linux, and macOS
editor/editor_paths.cpp
Outdated
#elif LINUXBSD_ENABLED | ||
return OS::get_singleton()->get_environment("HOME").path_join("Android/Sdk"); |
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.
Could you clarify what installation process leads to the Android SDK being in this path on Linux?
In my case I install the Android SDK from the command line tools from Google and thus the path is whatever I unzip the tools at, and pass as --sdk_root
to sdkmanager
.
Is ~/Android/Sdk
the default path used by Android Studio on Linux?
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.
Yes, in my case (and according to web search, not only in mine) Android Studio (no matter if it's snap, flatpak or official version) always use ~/Android
location for its files, including the SDK.
Sorry for the back and forth on this but I think the helper method to get the default SDK path should be in the Android |
…" for Windows, Linux, and macOS
3411a26
to
c6f4228
Compare
Thanks! Congratulations on your first contribution! 🎉 |
[It is reopened #97978]
It was always annoying to me to start android development in Godot because it doesn't work out of the box, despite the fact that it's completely possible (Android Studio is a prove).
In particular, for some reason, I always need to manually set Android SDK path, which is almost (if not completely) always the same within each OS family when installed from Android Studio with default settings.
So now Godot has default value of Android SDK path for every supported OS:
Tested on Linux and Windows (through wine).