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.

Sign up for GitHub

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 for Android build on Windows #91339

Merged
merged 1 commit into from
May 1, 2024
+16 −4
Merged

Fix for Android build on Windows #91339

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions platform/android/SCsub
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#!/usr/bin/env python

import sys
import subprocess

Import("env")
Expand Down Expand Up @@ -81,10 +82,21 @@ if lib_arch_dir != "":
env_android.Command(out_dir + "/libc++_shared.so", stl_lib_path, Copy("$TARGET", "$SOURCE"))

def generate_apk(target, source, env):
gradle_process = []

if sys.platform.startswith("win"):
gradle_process = [
"cmd",
"/c",
"gradlew.bat",
]
else:
gradle_process = ["./gradlew"]
Copy link
Contributor

Choose a reason for hiding this comment

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

It is executing the bash script instead of the batch script

Would replacing ./gradlew with ./gradlew.bat on windows fix the issue? That's the approach we usually follow in the codebase.

It appears that Windows has some special cases with subprocess.call as can be seen in this stack overflow: https://stackoverflow.com/a/4616867/9733262

The link you refer to mentions that this is only needed for commands built into the Shell and not required for executing a batch file.

Copy link
Contributor Author

@TCROC TCROC Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I originally tested with ./gradlew.bat as that seemed like the obvious approach. I can confirm, that does not work on Windows. I did not do any research as to why it didn't work on Windows BUT I am aware that the Rust programming language had a similar CVE regarding .bat files: https://blog.rust-lang.org/2024/04/09/cve-2024-24576.html

This may be why (at least my version of) python prevents executing the .bat file directly.

The recommended approach both in that Stack Overflow and Rust's CVE seems to be going through cmd /c.

I see you want me to test shell=(os.name == "nt") below so I can grab the exact error message when trying to invoke the .bat file directly as well. Then we can discuss which approach we want to take.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking!

Based on your comments, the approach looks good to me so I should be able to approve once you address @AThousandShips's comments.

I won't have access to my windows machine for a week, so I'm unable to validate the fix myself. As such, additional testing is welcomed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answers all questions in this comment here: #91339 (comment)


if env["target"] != "editor" and env["dev_build"]:
subprocess.run(
[
"./gradlew",
gradle_process
+ [
"generateDevTemplate",
"--quiet",
],
Expand All @@ -93,8 +105,8 @@ if lib_arch_dir != "":
else:
# Android editor with `dev_build=yes` is handled by the `generateGodotEditor` task.
subprocess.run(
[
"./gradlew",
gradle_process
+ [
"generateGodotEditor" if env["target"] == "editor" else "generateGodotTemplates",
"--quiet",
],
Expand Down
Loading