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

Fix the mingw-w64 dll name #998

Merged
merged 3 commits into from Sep 6, 2022

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Aug 30, 2022

Both MSVC and mingw-w64 should produce the same DLL name and their import libraries should target the same DLL as well.

Fixes #997

It should be the same name so mingw-w64 toolchains can generate/use the same
DLLs as the MSVC toolchains. The DLL and import libraries are compatible both
ways.

Fixes KhronosGroup#997
The common name for import libraries in mingw-w64 ends with .dll.a so we need
to use it so the pkg-config file ends up use -lvulkan-1.dll.
@CLAassistant
Copy link

CLAassistant commented Aug 30, 2022

CLA assistant check
All committers have signed the CLA.

@ci-tester-lunarg
Copy link

Author robUx4 not on autobuild list. Waiting for curator authorization before starting CI build.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 22836.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 1467 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 1467 passed.

@charles-lunarg
Copy link
Collaborator

This is not the first time the name of the vulkan-loader library in mingw has been changed. And I would really like some sort of authority (aka, a mingw documentation page stating what the convention should be) before merging any further changes.

#523 is the original PR that drops the lib prefix (afaik).

#606 Reverts PR 523 by adding it back.

Now this PR wants to drop it again. I'm just generally confused as to what the answer should be.

@robUx4
Copy link
Contributor Author

robUx4 commented Aug 31, 2022

Ah, I didn't know about this back and forth. It seems #523 did the proper thing which then broke unexpectedly with #595. #606 was an attempt to fix it but it did not go back to the #523 situation.

I'm not sure there's a definitive authority for mingw-w64 things. If anything it's the few people active on the project and in the mailing list. I have doing occasional work on mingw-w64 to fix various issues for VLC.

mingw-w64 is basically the Windows SDK redone in a separate/independent way (with the help of wine headers). So the idea is to provide the .h files to compile the code and the libraries to link the code so it works on Windows. So the import library have to target the Windows DLL with the right name. They don't actually build the DLLs (that would be wine's job), just generate the import libraries from a lot of .def files.

Given that, the first clear thing is that the import library must match the vulkan-1.dll name that is often found on the system. It is correctly done in the .def file of this project. But it's not used to generate the import library when building with mingw-w64. So to fix that we must tell the mingw-w64 to compile the DLL as vulkan-1.dll. Then the import library will target the right DLL.

Then there's the name of the import library. The mingw-w64 ones don't use the .dll.a suffix to match what is expected when linking with windows libraries. For example -lkernel32 uses libkernel32.a even though it's linking with a DLL. However for projects building with mingw-w64 a static library should be name libthing.a and a dynamic library should have an import library named libthing.dll.a. This allows building both static and dynamic versions. The user can pick which variant to use. In our case we (currently) only build a dynamic libary so it should be name libvulkan-1.dll.a which is expected to use vulkan-1.dll.

Side note: it seems that vulkan-1.dll is on all my windows/system32 with Windows 10 or 11. Not sure if it's installed by Microsoft or each driver (but then they would fight to pick the version they want). If this is from MS, that means it's a good candidate to be handled by mingw-w64 to provide the import library (from a .def file) and the headers (maybe they can be copied from here directly if the license fits).

@charles-lunarg
Copy link
Collaborator

I don't really have a stake in this, as I do not use mingw nor do we (lunarg) directly support it. I'm happy to take PR's, but well, the name of the file isn't something that should be changing every year.

What confuses me is whether a mingw built application needs to 'build' the loader to use it (aka, the loader has to be built in a mingw environment to be useable for mingw apps), or if the mingw app can use the vulkan-1.dll in in Windows/System32.

I can answer your question about vulkan-1.dll being in Windows/System32. That is installed by the drivers, not currently by Windows directly. The logic they follow is to replace it if the one being installed is 'newer' than what is there. In the future, MS could provide it but that isn't the case currently.

@robUx4
Copy link
Contributor Author

robUx4 commented Sep 1, 2022

What confuses me is whether a mingw built application needs to 'build' the loader to use it (aka, the loader has to be built in a mingw environment to be useable for mingw apps), or if the mingw app can use the vulkan-1.dll in in Windows/System32.

A mingw app can use the loader directly. All it needs to link with it is to "compile" the .def (generate the import library). But since there's no target in the project for that we have to build the DLL.

@charles-lunarg
Copy link
Collaborator

From your comments, it appears that vulkan-1.dll is the appropriate name, (aka no lib prefix).

I'm happy to accept this merge request, but do wonder why it was previously using lib at all.

Copy link
Collaborator

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

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

Because of mingw trying to be compatible with windows, it make sense that the dll name should be vulkan-1.dll (libvulkan-1.dll is neither windows style nor linux style). I will accept and merge the PR.

If anyone disagree's with this change, they should open an issue or PR and include proper references & documentation as to why it should change.

@charles-lunarg charles-lunarg merged commit a73cf14 into KhronosGroup:master Sep 6, 2022
@robUx4 robUx4 deleted the mingw-dll-name branch September 7, 2022 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mingw-w64 generate the wrong DLL name
4 participants