KhronosGroup / glslang

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

Issues using via CMake's FetchContent or add_subdirectory #3509

Open AnyOldName3 opened 4 months ago

AnyOldName3 commented 4 months ago

Not all platforms provide a new enough version of glslang that the proper CMake config files are present to just import the system installation. That means that some projects will still need to pull in the source and build it to support some platforms.

There are some problems with this, though.

Warnings

glslang has code that triggers many commonly-used warnings, and by default will inherit the parent project's warnings. This could be resolved in a a couple of ways:

This is really a problem with how the downstream project's setting warning flags. You're not supposed to set warning flags globally with add_compile_options as that makes your warning flags affect third-party code that wasn't written with these warnings in mind.

Alias

When imported through find_package, glslang is available as glslang::glslang, and ancillary libraries are also namespaced like glslang::SPIRV. When included via FetchContent or add_subdirectory, the namespaced versions don't exist, just glslang and SPIRV.

This can easily be resolved by adding a library alias with

add_library(glslang::glslang ALIAS glslang)

which would make it always available in the namespaced form.

Installing

Since https://github.com/KhronosGroup/glslang/pull/3439, glslang can only be installed if it's the top-level project.

This is okay for:

This isn't okay for:

In cases where the headers are needed because a library uses glslang headers in its public headers, which could be the case for shared or static libraries, they simply aren't installed.

Right now, the only options for downstream libraries are to duplicate a lot of glslang's CMake, or to use a version before #3439 was merged.

It might be enough to change the condition so that it's also controlled by an option, so the default behaviour is the same as now (install is enabled if glslang is the top-level project and disabled otherwise), but can be overridden if the consuming project needs something different.

Something else that would solve the particular problem I'm hitting would be if glslang could be compiled as an object library. That way, the archiver would put glslang's object files into any static libraries depending on glslang, and a linker would link them into any shared libraries or applications. The LIB_TYPE CMake variable could be turned into an option to enable this, but some of the checks that currently use BUILD_SHARED_LIBS would need to be changed to check LIB_TYPE instead. This isn't a particularly standard way to solve this kind of problem, and only solves the specific problem I'm personally hitting rather than the related ones, so it's probably not the sensible approach to take.

AnyOldName3 commented 4 months ago

Looks like https://github.com/KhronosGroup/glslang/pull/3508 will resolve the installation-related problems here.

AnyOldName3 commented 4 months ago

Including GlslangToSpv.h

When installed, this header lives at <glslang/SPIRV/GlslangToSpv.h>, which is a perfectly reasonable location. When incorporated into an existing project, though, the header's at SPIRV/GlslangToSpv.h relative to the repo root.

The include directory is set as

target_include_directories(SPIRV PUBLIC
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/..>
    $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)

This means that a downstream project consuming SPIRV will either need to include the header as <glslang/SPIRV/GlslangToSpv.h> or <SPIRV/GlslangToSpv.h> depending on whether it's imported.

There have been discussions elsewhere about potentially moving the SPIRV directory to be a subdirectory of the glslang directory. This would be a breaking change for projects setting up the include directory manually instead of having CMake handle it, but disruption could be potentially reduced by saying that including it as <SPIRV/GlslangToSpv.h> is deprecated, and temporarily setting the glslang directory as an include directory on the SPIRV target so it'd still work for a while.