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

Remove ARFLAGS hack for Windows, replace with TEMPFILE #96407

Merged

Conversation

alvinhochun
Copy link
Contributor

TEMPFILE is the built-in way of SCons to use a response file for command lines that are too long.

@bruvzg This can replace #96220.

TEMPFILE is the built-in way of SCons to use a response file for command
lines that are too long.
@akien-mga akien-mga changed the title Remove ARFLAGS hack for Windows, replace with TEMPFILE Remove ARFLAGS hack for Windows, replace with TEMPFILE Sep 3, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 02b16d2), it works as expected on Windows 11 23H2 + MSVC 2022 (17.10.0).

@akien-mga akien-mga requested a review from Faless September 17, 2024 15:25
@akien-mga akien-mga merged commit 804d977 into godotengine:master Sep 18, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

This seems to have broken mingw build on Windows (at least with some mingw distribution). I saw this reported on Discord by @noidexe:

[Initial build] Linking Static Library modules\freetype\libfreetype_builtin.windows.editor.x86_64.a ...
ERROR: C:/Users/lisan/scoop/apps/mingw/14.2.0-rt_v12-rev0/bin/../lib/gcc/x86_64-w64-mingw32/14.2.0/../../../../x86_64-w64-mingw32/bin/ar.exe: thirdpartyfreetypesrcautofitautofit.windows.editor.x86_64.o: No such file or directory

scons: *** [modules\freetype\libfreetype_builtin.windows.editor.x86_64.a] Error 1
scons: building terminated because of errors.

@AThousandShips
Copy link
Member

AThousandShips commented Sep 19, 2024

Was just about to post on this but I get a different error, involving prefixed @ on tempfiles:

x86_64-w64-mingw32-gcc-ar @C:\Users\XXXX\AppData\Local\Temp\tmpzcvaw526.rsp
C:/Users/XXX/scoop/apps/mingw/13.2.0-rt_v11-rev0/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ar.exe: unknown option -- @

This was resolved by updating to mingw 14.2

@alvinhochun
Copy link
Contributor Author

This seems to have broken mingw build on Windows (at least with some mingw distribution). I saw this reported on Discord by @noidexe:

[Initial build] Linking Static Library modules\freetype\libfreetype_builtin.windows.editor.x86_64.a ...
ERROR: C:/Users/lisan/scoop/apps/mingw/14.2.0-rt_v12-rev0/bin/../lib/gcc/x86_64-w64-mingw32/14.2.0/../../../../x86_64-w64-mingw32/bin/ar.exe: thirdpartyfreetypesrcautofitautofit.windows.editor.x86_64.o: No such file or directory

scons: *** [modules\freetype\libfreetype_builtin.windows.editor.x86_64.a] Error 1
scons: building terminated because of errors.

I see, so it appears we will have to set TEMPFILEARGESCFUNC to replace backslashes to forward slashes.

Was just about to post on this but I get a different error, involving prefixed @ on tempfiles:

x86_64-w64-mingw32-gcc-ar @C:\Users\XXXX\AppData\Local\Temp\tmpzcvaw526.rsp
C:/Users/XXX/scoop/apps/mingw/13.2.0-rt_v11-rev0/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ar.exe: unknown option -- @

Hmm, Binutils ar should support response file. This doesn't look right.

@AThousandShips
Copy link
Member

My above issue was solved by updating to mingw 14.2, unsure when the relevant functionality was introduced or exactly what was different but might be worth looking at versions supporting this

@Repiteo
Copy link
Contributor

Repiteo commented Sep 28, 2024

Can confirm this caused me issues with adding GCC to the Windows CI in #97446; reverting the change allowed the build to work as expected

@alvinhochun
Copy link
Contributor Author

Was just about to post on this but I get a different error, involving prefixed @ on tempfiles:

x86_64-w64-mingw32-gcc-ar @C:\Users\XXXX\AppData\Local\Temp\tmpzcvaw526.rsp
C:/Users/XXX/scoop/apps/mingw/13.2.0-rt_v11-rev0/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ar.exe: unknown option -- @

This was resolved by updating to mingw 14.2

Can confirm this caused me issues with adding GCC to the Windows CI in #97446; reverting the change allowed the build to work as expected

This took me a while to diagnose, but turns out it was a known upstream GCC bug with calling gcc-ar (instead of plain ar) until quite recently, which inserts a dash in front of the first argument even if it specifies a response file: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77576
Looks like the fix landed on GCC 14.1.0.

Some ideas to accommodate older versions:

  • Don't use gcc-ar, just use ar directly?
    • From the comments in the bug report it seems ar should be able to find the LTO plugin automatically, so gcc-ar should not be necessary these days. This needs testing.
  • Override TempFileMunge to leave the ARFLAGS outside of the response file (i.e. the command becomes gcc-ar.exe rc --thin @C:\temp\tmp123.rsp)
    • Seems non-trivial
  • Add a hack to use_windows_spawn_fix to insert -- before the response file command line argument (i.e. the command becomes gcc-ar.exe -- @C:\temp\tmp123.rsp)
    • Does not fix cross-compilation from Linux, though with the long command line limit on Linux it is less of an issue there.

The first option may be the best if it works. Can someone test it out? (I very much doubt my system can handle an LTO build of Godot.)

@AThousandShips
Copy link
Member

If you have some diffs for the suggested changes I can try them out

@alvinhochun
Copy link
Contributor Author

If you have some diffs for the suggested changes I can try them out

Probably like

diff --git a/platform/windows/detect.py b/platform/windows/detect.py
index db4c743595..7fbcd8a0df 100644
--- a/platform/windows/detect.py
+++ b/platform/windows/detect.py
@@ -792,8 +792,6 @@ def configure_mingw(env: "SConsEnvironment"):
         env["CXX"] = mingw_bin_prefix + "g++"
         if try_cmd("as --version", env["mingw_prefix"], env["arch"]):
             env["AS"] = mingw_bin_prefix + "as"
-        if try_cmd("gcc-ar --version", env["mingw_prefix"], env["arch"]):
-            env["AR"] = mingw_bin_prefix + "gcc-ar"
         if try_cmd("gcc-ranlib --version", env["mingw_prefix"], env["arch"]):
             env["RANLIB"] = mingw_bin_prefix + "gcc-ranlib"

(I do also wonder if gcc-ranlib is needed in a similar sense...)

@Repiteo
Copy link
Contributor

Repiteo commented Sep 28, 2024

I can confirm that commenting out the gcc-ar code fixed the issue for my PR!

@Faless
Copy link
Collaborator

Faless commented Sep 28, 2024

I can confirm that commenting out the gcc-ar code fixed the issue for my PR!

Doesn't that risk breaking creoss-compilation from other platforms though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants