Fix shell argument limit #1731
Fix shell argument limit #1731
Conversation
tools/godotcpp.py
Outdated
| @@ -162,6 +163,23 @@ def scons_generate_bindings(target, source, env): | |||
| return None | |||
|
|
|||
|
|
|||
| def _build_static_lib_with_rsp(target, source, env): | |||
| target_lib = str(target[0]) | |||
| rsp_fd, rsp_path = tempfile.mkstemp(suffix=".rsp") | |||
There was a problem hiding this comment.
Is there a reason you aren't using with TemporaryFile(suffix=".rsp") as rsp_file? Using that would allow you to skip the finally step of clean up because it cleans up after itself.
There was a problem hiding this comment.
It's a left over of a bad example I copy pasted during my experimentations. Will fix, nice catch!
|
aww linux only, i was hoping to test it for msys and mingw on windows see if it helps mingw link times at all(unlikely) |
|
Thanks for starting on a PR for this! This should work on MSYS with mingw, since I think it uses GNU binutils. And, I'm not sure this will work on Linux if using Have you tried building on other platforms? I suspect this is a problem on Windows and MacOS too, and, in fact, my guess would be that Windows has a smaller limit than Linux. Ideally, any solution to this would address all platforms and tool chains - unless this isn't a problem on other platforms and tool chains |
https://llvm.org/docs/CommandGuide/llvm-ar.html |
|
@enetheru Oh, awesome, thanks for looking that up! I guess this means it'll work when building for Android too, because I think the Android tool chain uses clang |
ze2j
force-pushed
the
fix_shell_argument_limit
branch
from
13cf09a to
5458596
Compare
|
I made the suggested fixes and included MinGW. @dsnopek I’ve only ever built my project on Linux, so I’m not sure if this issue can be reproduced on other platforms. Windows seems to have an 8192 limit, so my guess is that there’s no issue. I took a very pragmatic approach here, but if you prefer a more general one, I won’t be offended if you decide to close this PR. I won’t be able to spend time implementing a more general solution, but I can help test it on my project if someone comes up with one. |
|
I can't get the test integration compiling for mingw64 but everything else appears to work. |
|
@enetheru Should I drop the mingw inclusion or there is still some hope to make it work? |
|
I'm not very experienced on the scons side, perhaps someone else can test? but if it's going to block merging I would drop it as it's not required to fix your issue yeah? |
Fix 1729
Tested on Linux only.
Note: I didn't manage to remove the ar command print in non verbose builds.
