search

KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
3.01k stars 829 forks source link

14.0.0 broke FindVulkan in CMake #3480

Closed moritz-h closed 8 months ago

moritz-h commented 8 months ago

There is a FindVulkan Module in CMake supporting users to do find_package(Vulkan COMPONENTS glslang). As this module is explicitly checking for the existence of every single glslang target, this broke as the 14.0.0 release removed the OGLCompiler target.

I started a try to fix this in upstream CMake here https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9158

May you can help with defining what the public interface of glslang is that should be checked for. I saw this comment:

Yes, OSDependent, MachineIndependent, and GenericCodeGen are not for end user use and should be object libraries.

Originally posted by @arcady-lunarg in https://github.com/KhronosGroup/glslang/issues/3462#issuecomment-1871369876

With that, it may makes sense to also remove checks for the existence for these targets, if you as official source can confirm, that these are not part of the glslang CMake interface. Otherwise the find vulkan module will probably break again when these libs will be converted to object libraries which are not installed anymore.

Additional reference: The issue was first detected while upgrading glslang in vcpkg https://github.com/microsoft/vcpkg/pull/36111

arcady-lunarg commented 8 months ago

I believe the correct way to do this in cmake is to simply use find_package(glslang) rather than looking at individual targets. Sounds like we should probably update various other places that use that. @jpr42 could you confirm?

moritz-h commented 8 months ago

Of course when using glslang stand alone find_package(glslang) seems the best option.

I just noted, that the FindVulkan CMake module probably is written against the Vulkan SDK Release which only contains the headers and library binaries of glslang, but no CMake target information. At least with this it would make sense why glslang is seen as component of Vulkan instead of an own package and why the FindVulkan module manually creates all the CMake targets instead of using the ones provided by glslang.

So probably if there are CMake users of the Vulkan SDK trying to do find_package(Vulkan COMPONENTS glslang), this issue did just not came up yet because there was no new SDK release since the removal of OGLCompiler. It just now first came up in vcpkg because the glslang stand alone release numbers are used to build the Vulkan environment.

moritz-h commented 8 months ago

I saw this CMake discussion about deprecating the FindVulkan module: https://gitlab.kitware.com/cmake/cmake/-/issues/25384#note_1465932 Also resulting in this request to include CMake configs of all libraries in the VulkanSDK: https://vulkan.lunarg.com/issue/view/6598781b5df1125b58afbb3c

Having the Vulkan SDK also include CMake Configs and then CMake replacing the find module sounds like the best solution long term. So it may be better to move the discussion to the linked issues, as it is more of a CMake and Vulkan SDK problem and not really related to glslang anymore.

jpr42 commented 8 months ago

I believe the correct way to do this in cmake is to simply use find_package(glslang) rather than looking at individual targets.

Correct. find_package(glslang CONFIG) should be preferred. However, the Vulkan SDK doesn't currently ship the necessary *.cmake files on all platforms.

Sounds like we should probably update various other places that use that. @jpr42 could you confirm?

Repositories inside the KhronosGroup organization don't use FindVulkan.cmake the problem is other projects that do use it.

I just noted, that the FindVulkan CMake module probably is written against the Vulkan SDK Release which only contains the headers and library binaries of glslang, but no CMake target information. At least with this it would make sense why glslang is seen as component of Vulkan instead of an own package and why the FindVulkan module manually creates all the CMake targets instead of using the ones provided by glslang.

Yes the FindVulkan.cmake was originally written against the Vulkan SDK released by LunarG. At the time it was written the projects didn't provide CMake configuration files. Now most projects do provide configuration files.

NOTE: FindVulkan.cmake has been updated to also work with other system package managers. I see that MinGW folks have contributed to it a fair amount for example.

So probably if there are CMake users of the Vulkan SDK trying to do find_package(Vulkan COMPONENTS glslang), this issue did just not came up yet because there was no new SDK release since the removal of OGLCompiler. It just now first came up in vcpkg because the glslang stand alone release numbers are used to build the Vulkan environment.

Ideally FindVulkan.cmake would be deprecated and the SDK shipped the necessary CMake configuration files.

moritz-h commented 8 months ago

Ideally FindVulkan.cmake would be deprecated and the SDK shipped the necessary CMake configuration files.

That sounds really great and with CMake discussing the same (https://gitlab.kitware.com/cmake/cmake/-/issues/25384#note_1465932) hopefully things find together somehow. So I will close this issue here as it is not a glslang bug and the linked CMake issue or LunarG bugtracker are probably better places for this discussion.

ghost commented 8 months ago

I have a fix for FindVulkan.cmake up now: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9225

It will also handle remove of osdependent, machineindependent, and genericcodegen to avoid further disruption.

It's set to be in for CMake 3.29.0