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

Fix detecting when Gradle is invoked from Studio #92176

Merged
merged 1 commit into from
May 21, 2024

Conversation

emrekultursay
Copy link
Contributor

The existing idea.platform.prefix system-property approach only worked because of a Android Studio bug that leaks the system properties from Android Studio into Gradle build.

This bug was fixed in Android Studio 2023.3.1 (Jellyfish), which is why Godot fails to build on Jellyfish without this fix.

The correct way of identifying builds from Android Studio is to use the following project property (not system property):

  • android.injected.invoked.from.ide

Fixes #92122

@emrekultursay emrekultursay requested a review from a team as a code owner May 20, 2024 21:55
@m4gr3d m4gr3d added bug platform:android topic:buildsystem cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels May 20, 2024
@m4gr3d m4gr3d added this to the 4.3 milestone May 20, 2024
@@ -233,8 +233,7 @@ def generateBuildTasks(String flavor = "template") {
}

def isAndroidStudio() {
def sysProps = System.getProperties()
return sysProps != null && sysProps['idea.platform.prefix'] != null
return project.hasProperty('android.injected.invoked.from.ide')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you apply the fix in config.gradle as well.

Is this approach supported by past releases of Android Studio?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually looking at the code, the definition of isAndroidStudio() defined in config.gradle is visible here as well, so you can just make the change in the version in config.gradle and delete the version of isAndroidStudio() that's in this build.gradle file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can just make the change in the version in config.gradle and delete the version of isAndroidStudio() that's in this build.gradle file.

Done.

Is this approach supported by past releases of Android Studio?

This property has been available since 2014: https://android.googlesource.com/platform/tools/base/+/45573717b3f70c58b9e389ada306b25d477af6d9

@m4gr3d
Copy link
Contributor

m4gr3d commented May 20, 2024

Validated the fix works as expected!

@emrekultursay
Copy link
Contributor Author

PTAL.

Copy link
Contributor

@m4gr3d m4gr3d left a 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 into one so the PR is ready for merging (https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase)

The existing 'idea.platform.prefix' system-property approach
only worked because of a Android Studio bug that leaks the
system properties from Android Studio into Gradle build:
  - https://issuetracker.google.com/201075423

This bug was fixed in Android Studio 2023.3.1 (Jellyfish).

The correct way of identifying builds from Android Studio is to
use the following project property (not system property):
 - android.injected.invoked.from.ide
@emrekultursay
Copy link
Contributor Author

Squashed commits. PTAL.

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks good!

@akien-mga akien-mga added the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label May 21, 2024
@akien-mga akien-mga merged commit 0e39ac6 into godotengine:master May 21, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release platform:android topic:buildsystem
Projects
Status: Bad
Development

Successfully merging this pull request may close these issues.

Android Studio Jellyfish (2023.3.1) doesn't build Godot project
3 participants