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

SCons: Add proper MinGW support to D3D12 deps install script #87619

Merged

Conversation

akien-mga
Copy link
Member

I'm not super happy about the change in platform/windows/detect.py to figure out whether to modify the env["mesa_libs"] path to add a -mingw suffix...

Open to suggestions to make this less hacky, especially having to pass the detected d3d12 deps path as an option, because we can't easily pass info otherwise from get_opts to the env or configure_mingw (global doesn't seem to work).

Might also work on macOS to cross-compile, worth testing.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Maybe we should update Mesa to the current release - 23.3.4 (GitHub mirror seems to be outdated, and active repo is https://gitlab.freedesktop.org/mesa/mesa/), and make a single release with both MinGW and MSVC files.

@akien-mga
Copy link
Member Author

That would be nice yeah, it should simplify the script so we don't have to make assumptions, if we can just rely on mesa_path being either custom or the target of the install script for both MSVC and MinGW.

@akien-mga akien-mga force-pushed the scons-d3d12-install-fix-linux-support branch from 4b2d798 to c60a926 Compare January 29, 2024 11:06
@akien-mga akien-mga requested a review from a team as a code owner January 29, 2024 11:06
@akien-mga akien-mga force-pushed the scons-d3d12-install-fix-linux-support branch from c60a926 to c5ad9f2 Compare January 29, 2024 12:33
@akien-mga akien-mga force-pushed the scons-d3d12-install-fix-linux-support branch from c5ad9f2 to c747184 Compare February 20, 2024 16:05
@akien-mga
Copy link
Member Author

Redid this PR now that https://github.com/godotengine/godot-nir-static/ has both MSVC and MinGW support in the same tarball.

There's no more command line options, but instead we rely on whether we find gendef and dlltool to see if the .a files should be generated for PIX. Needs to be checked if that doesn't break the script on Windows both when MinGW is installed and not installed/in PATH.

@akien-mga akien-mga force-pushed the scons-d3d12-install-fix-linux-support branch from c747184 to 8ebeec2 Compare February 20, 2024 16:09
@akien-mga akien-mga force-pushed the scons-d3d12-install-fix-linux-support branch from 8ebeec2 to 64d8cb7 Compare February 22, 2024 13:27
@Calinou
Copy link
Member

Calinou commented Feb 23, 2024

Tested locally on Windows 11 with llvm-mingw 20231128, it works as expected. The resulting D3D12 build can be run just fine.

Note that if your llvm-mingw version is older (I had a version from 2022 lying around), I got the following output with it (but the .a generation was still successful):

[3/4] WinPixEventRuntime
Downloading WinPixEventRuntime 1.0.231030001 ...
Extracting WinPixEventRuntime 1.0.231030001 to C:\Users\Hugo\AppData\Local\Godot\build_deps\pix ...
Converting WinPixEventRuntime.dll for MinGW.
 * [./bin/x64/WinPixEventRuntime.dll] Found PE+ image
ignoring unknown argument: --no-leading-underscore
 * [./bin/ARM64/WinPixEventRuntime.dll] Found PE+ image
ignoring unknown argument: --no-leading-underscore
WinPixEventRuntime 1.0.231030001 installed successfully.

The script still works successfully if I rename my llvm-mingw folder so it's no longer in PATH.

@akien-mga akien-mga force-pushed the scons-d3d12-install-fix-linux-support branch from 64d8cb7 to 5fd9d08 Compare February 23, 2024 21:13
@akien-mga akien-mga merged commit d71ee02 into godotengine:master Feb 23, 2024
15 checks passed
@akien-mga akien-mga deleted the scons-d3d12-install-fix-linux-support branch February 23, 2024 21:26
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.

4 participants