-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 for Android build on Windows #91339
Fix for Android build on Windows #91339
Conversation
5bd6856
to
df9288a
Compare
df9288a
to
cc0d5a4
Compare
"gradlew.bat", | ||
] | ||
else: | ||
gradle_process = ["./gradlew"] |
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.
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.
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 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.
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.
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!
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.
Answers all questions in this comment here: #91339 (comment)
Does this fail on Windows specifically? Or on other platforms? If it's not on windows try making it |
It works on my POP OS Linux machine but does not work on my Windows machine. I have not tested my Mac, but I suspect the Mac will work fine since it invokes shell scripts. Is this what you are asking or am I misunderstanding the question? |
What I asked is does adding |
Oh I think I understand the question. Yes it fails on Windows specifically. I have not encountered this on any of my other machines. |
I originally tried Windows:
I'm going to double check this later today to confirm. But this was my experience with it last week when I tested. |
Can't tell exactly how to solve this since I don't work with android, so I've got no further comments here |
@AThousandShips @m4gr3d I just tested your suggestions and these were the results: gradlew shell script = fail ❌ I'm guessing python treats the same CVE as Rust above similarly and that is why it requires cmd. But that is just a guess. What is certain is Windows (or at least my Windows + Python setup) requires If you guys are happy with it, I think the PR is ready to be merged :) |
Thanks! |
Fixes: #91195
Copying in issue description for ease of reading:
Tested versions
Godot v4.3.dev.mono (463ede1f7)
System information
Windows 10
Issue description
Scons currently errors when building an apk during the template creation process. This is due to this line here:
godot/platform/android/SCsub
Line 87 in 11d3768
There are 2 issues with this line.
subprocess.call
as can be seen in this stack overflow: https://stackoverflow.com/a/4616867/9733262That particular solution suggests passing "Shell=true", but when testing that also causes its own issues due to trying to treat the
.
operator as a program itself.This solution appears to actually fix it on my local test: https://stackoverflow.com/a/39474235/9733262
This was discovered when adding windows support to my
godot-src
project over here: https://github.com/Lange-Studios/godot-src.git.