KhronosGroup
Prefix cmake options and compiler definitions with GLSLANG_
Addresses #1816
Most changes are direct find-replaces with the standard GLSLANG_ prefix. Note that there is some pre-existing inconsistency where some options/definitions were GLSLANG_ENABLE and others were ENABLE_GLSLANG. I opted not to change the latter options to keep what is a sizable commit slimmer and reduce downstream disruption.
Agreed in principle, and that it's the right direction to go.
My concern is breakage with existing usages that have coded the current names. I'd like some idea of how many consumers will break, and if there are any, how we manage the transition to the better way.
That sentiment is understandable. My thought process is that there is already a planned deprecating change in #1964 and perhaps this would be a good opportunity to clean up other aspects of the project. Users that rely on the installed binary would be unaffected.
Another consideration is to not even have the GLSL dedicated option for building shared vs static libraries. Cmake already provides BUILD_SHARED_LIBS and generally it's not recommended to mix static and shared linkage. The breakage I'm envisioning is libraries that depend on glslang but do not know to forward the BUILD_SHARED_LIBS option to glslang, so I'm of the opinion that having GLSLANG_BUILD_SHARED_LIBS is itself an anti-pattern.
edit: An easy backwards-compatible fix for this is to set LIB_TYPE to SHARED if either BUILD_SHARED_LIBS or GLSLANG_BUILD_SHARED_LIBS is set.
I'd recommend accepting both for now. Add an entry to the README that the old versions are deprecated and will be removed in ~6 months. All the glslang internal code should be updated to the new flags and have the CMake files detect the old ones and set the new ones. We then remove the cmake bits when we hit the deprecation date.
This will make it easier to roll out to the ecosystem without causing a lot of breakage.
GLSLANG_BUILD_SHARED_LIBS is itself an anti-pattern.
I'm coming to that belief too. People have said that of Shaderc before, and they've won me over.
Related:
- supporting shared libs "properly" in SPIRV-Tools https://github.com/KhronosGroup/SPIRV-Tools/issues/3214
- supporting shared libs "properly" in Shaderc https://github.com/google/shaderc/issues/994
- prior relevant discussion on this abandoned Shaderc PR https://github.com/google/shaderc/pull/498
I'd really want to solve this problem one way across all these projects (and others I influence). Thanks everybody for your patience.
Just for clarity, should I leave this PR in a holding pattern then until it's decided how to handle the deprecation across the ecosystem? I'm happy to update the PR but would prefer to wait for consensus from the relevant stakeholders
Somewhat unrelated to the discussion above, but there's a pattern I've been using to try and reduce the number of settings that cannot be modified appearing in CMake projects that include other CMake projects.
Consider CMake project Outer that depends on Inner. Inner has a number of option()s that Outer requires to be set to particular values.
Typically Outer will use FORCE CACHE to set the option() to a particular value before including Inner with add_subdirectory(). This option still appears in the CMake settings list for no good reason, and worse still, changing this value will do nothing.
Instead, I've been using this pattern in the Inner project:
function (option_if_not_defined name description default)
if(NOT DEFINED ${name})
option(${name} ${description} ${default})
endif()
endfunction()
option_if_not_defined(MARL_WARNINGS_AS_ERRORS "Treat warnings as errors" OFF)
option_if_not_defined(MARL_BUILD_EXAMPLES "Build example applications" OFF)
option_if_not_defined(MARL_BUILD_TESTS "Build tests" OFF)
option_if_not_defined(MARL_BUILD_BENCHMARKS "Build benchmarks" OFF)
This acts as a regular option() when Inner is used as a standalone project, or if the setting is not set by Outer.
However, if Outer does set() any of these settings before calling add_subdirectory(), then they do not act as options, nor show up in the CMake settings / CMakeCache.txt.
Offline, Ben also pointed to https://stackoverflow.com/questions/48524359/naming-convention-for-components-and-namespaces-in-cmake for naming possibilities; just capturing that here as well.
Strictly speaking, exported targets should be following the :: convention (e.g. glslang::glslang) because CMake knows that linking against a name with a double-colon is strictly a target (and not, for example, a library/file on the filesystem somewhere).
