SCons: Implement get_mingw_tool to fix mingw prefix ambiguity (reverted)

[Repiteo]

Expands upon the #79871 fix. Instead of separately checking for if a tool exists with/without a prefix, and using that to manually check for a tool, this PR consolidates that logic into a single function: get_mingw_tool. Now it returns a path outright to whatever tool it finds, or an empty path if no tool was found whatsoever. Because an empty string will function as False in conditionals, this was sufficent to remove the need for get_mingw_bin_prefix in isolation & replace try_cmd outright

The primary change compared to try_cmd is that only the leading argument needs to be supplied; as such, the argument was renamed from "test" to "tool". --version is now implicitly what gets tested, but can be changed if needed via a new argument. This new function was applied to all the areas where get_mingw_bin_prefix and/or try_cmd were directly called, consolidating the relevant code in the process

[saierXP]

By the way I’d like you to add the example of mingw_prefix when mingw is not found. Last month when I was using mingw to compile a third party module, I ran into the problem of not being able to find the compiler when mingw_prefix was set to mingw\bin.

Note: MINGW_PREFIX should point to your MinGW installation, if you installed MSYS2 with default settings this directory will be “C:/msys64/mingw64”1¹.

godot/platform/windows/detect.py

Lines 324 to 328 in 1ba920f

	print(
	"""
	No valid compilers found, use MINGW_PREFIX environment variable to set MinGW path.
	"""
	)

Footnotes

1. https://devblogs.microsoft.com/cppblog/using-mingw-and-cygwin-with-visual-cpp-and-open-folder/ ↩

[akien-mga]

Looks good to me overall. Haven't tested yet.

[akien-mga]

Thanks!

[TokageItLab]

After this fix, I can't build Godot on windows... Am I missing something in MinGW? It was buildable until one of the commits prior to this PR, so does this PR cause it to check and block anything?

 *  Executing task: scons platform=windows target=editor -j 16 

scons: Reading SConscript files ...
Using MinGW, arch x86_64
Building for platform "windows", architecture "x86_64", target "editor".
Checking for C header file mntent.h... (cached) no
scons: done reading SConscript files.
scons: Building targets ...
[Initial build] build_res_file(["platform\windows\godot_res_wrap.windows.editor.x86_64.o"], ["platform\windows\godot_res_wrap.rc"])
[Initial build] Compiling platform\windows\os_windows.cpp ...
[Initial build] Compiling platform\windows\display_server_windows.cpp ...
[Initial build] Compiling platform\windows\native_menu_windows.cpp ...
[Initial build] build_res_file(["platform\windows\godot_res.windows.editor.x86_64.o"], ["platform\windows\godot_res.rc"])
[Initial build] Compiling main\main.cpp ...
[Initial build] Compiling main\performance.cpp ...
[Initial build] Compiling modules\ktx\register_types.cpp ...
[Initial build] Compiling modules\ktx\texture_loader_ktx.cpp ...
[Initial build] Compiling modules\theora\register_types.cpp ...
[Initial build] Compiling modules\theora\video_stream_theora.cpp ...
[Initial build] Compiling modules\vorbis\audio_stream_ogg_vorbis.cpp ...
[Initial build] Compiling modules\vorbis\register_types.cpp ...
[Initial build] Compiling modules\vorbis\resource_importer_ogg_vorbis.cpp ...
[Initial build] Compiling modules\astcenc\image_compress_astcenc.cpp ...
[Initial build] scons: *** [platform\windows\godot_res_wrap.windows.editor.x86_64.o] Error -1
Compiling modules\astcenc\register_types.cpp ...
scons: *** [platform\windows\godot_res.windows.editor.x86_64.o] Error -1
scons: building terminated because of errors.

[bruvzg]

This probably should be reverted since it's based on wrong assumption about prefixes (it's checking for empty arch, but always adding path part of the prefix), a lot of tool chains have some prefixed (compilers, etc.) and some non-prefixed tools (generic stuff like windres), and have them in the different locations.

[bruvzg]

This probably should be reverted

Or I guess an extra case can be added to the get_mingw_tool to keep the cleanup part, something like (not tested):

diff --git a/platform/windows/detect.py b/platform/windows/detect.py
index ccf889f1a3..22b2d90fdb 100644
--- a/platform/windows/detect.py
+++ b/platform/windows/detect.py
@@ -40,6 +40,19 @@ def get_mingw_tool(tool, prefix="", arch="", test="--version"):
                 return path
         except Exception:
             pass
+    try:
+        path = f"{tool}"
+        out = subprocess.Popen(
+            f"{path} {test}",
+            shell=True,
+            stderr=subprocess.PIPE,
+            stdout=subprocess.PIPE,
+        )
+        out.communicate()
+        if out.returncode == 0:
+            return path
+    except Exception:
+        pass
 
     return ""
 

[akien-mga]

This probably should be reverted

Or I guess an extra case can be added to the get_mingw_tool to keep the cleanup part.

My understanding from the PR is that it tries first arch-prefixed binaries, and then the tool without prefix as final fallback. So I would expect it to still be able to call a windres in PATH.

[bruvzg]

Currently, if prefix is set to something like /mingw32, it will try /mingw32/bin/i686-w64-mingw32-windres (usual for cross building) and /mingw32/bin/windres (unlikely to be valid), but it might be just /bin/windres (usual for MSYS), so it should have an extra step for windres without any prefix to look in PATH.

[bruvzg]

But checking for all tools, like gcc and clang in PATH will cause it to wrongly report Windows as a supported platform when it's not. So it should be applied only to extra tools like windres, strip, and objdump as it was before.