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

Issues using via CMake's FetchContent or add_subdirectory #3509

Open
AnyOldName3 opened this issue Feb 12, 2024 · 2 comments
Open

Issues using via CMake's FetchContent or add_subdirectory #3509

AnyOldName3 opened this issue Feb 12, 2024 · 2 comments

Comments

@AnyOldName3
Copy link
Contributor

AnyOldName3 commented Feb 12, 2024

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:

  • ~Fix the warnings, like -Wshadows fixes #3143 attempted for -Wshadow.~~
  • Explicitly disable warnings on the glslang targets. This could be a hassle as just adding -Wno-shadow -Wno-implicit-fallthrough and -Wno-missing-field-initializers (plus equivalents for other compilers) depends on the order of arguments to work, so it would also need various CMake variables and properties to be scanned and have flags enabling these warnings replaced.

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 #3439, glslang can only be installed if it's the top-level project.

This is okay for:

  • applications using glslang with BUILD_SHARED_LIBS off - the static library will be linked so nothing from glslang will need to be installed.
  • applications using glslang with BUILD_SHARED_LIBS on - (presumably - I haven't checked this, so could be wrong) one of the CMake features that installs runtime dependencies for a target would install the shared libraries.
  • shared libraries - they'd work the same as applications.

This isn't okay for:

  • static libraries using glslang with BUILD_SHARED_LIBS off - nothing's been linked, so the glslang static library needs to be installed, and CMake hasn't been told how.
  • static libraries using glslang with BUILD_SHARED_LIBS on - nothing's been linked, so the glslang shared library (import library with MSVC) needs to be installed, and CMake hasn't been told how, although the runtime part could presumably be installed.

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
Copy link
Contributor Author

Looks like #3508 will resolve the installation-related problems here.

@AnyOldName3
Copy link
Contributor Author

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.

AnyOldName3 added a commit to AnyOldName3/glslang that referenced this issue Feb 14, 2024
This makes things consistent between when glslang is installed and imported versus when it's included as nested CMake project.
You can now use `glslang::glslang` in both cases instead of needing the non-namespaced version sometimes and the namespaced one other times.

Resolves one of the problems discussed in KhronosGroup#3509
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

No branches or pull requests

1 participant