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

Prefix cmake options and compiler definitions with GLSLANG_ #2115

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeremyong
Copy link

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.

@CLAassistant
Copy link

CLAassistant commented Mar 11, 2020

CLA assistant check
All committers have signed the CLA.

@johnkslang
Copy link
Member

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.

@jeremyong
Copy link
Author

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.

@jeremyong
Copy link
Author

jeremyong commented Mar 11, 2020

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.

@dj2
Copy link
Contributor

dj2 commented Mar 11, 2020

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.

@dneto0
Copy link
Contributor

dneto0 commented Mar 11, 2020

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:

I'd really want to solve this problem one way across all these projects (and others I influence). Thanks everybody for your patience.

@jeremyong
Copy link
Author

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

@ben-clayton
Copy link
Contributor

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.

@johnkslang
Copy link
Member

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.

@jeremyong
Copy link
Author

jeremyong commented Mar 11, 2020

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).

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.

6 participants