OpenColorIO icon indicating copy to clipboard operation
OpenColorIO copied to clipboard

CMake build types are hard-coded case-sensitive

Open anderslanglands opened this issue 3 years ago • 2 comments

Hello, I've got a build setup where the build type is being passed as lower case for $reasons. CMake <= 3.21 explicitly says CMAKE_BUILD_TYPE is case insensitive (3.22 and greater say "Depending on the situation, the value of this variable may be treated case-sensitively or case-insensitively" which is about as helpful as you'd expect).

In OCIO's CMakeLists.txt the following check essentially forces it to be case sensitive, which breaks my build:

if(NOT "${CMAKE_BUILD_TYPE}" IN_LIST CMAKE_CONFIGURATION_TYPES)
    string(REPLACE ";" ", " _CMAKE_CONFIGURATION_TYPES_STR "${CMAKE_CONFIGURATION_TYPES}")
    message(FATAL_ERROR 
            "CMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} is unsupported. Supported values are: ${_CMAKE_CONFIGURATION_TYPES_STR}.")
endif()

which is especially annoying because you do an explicitly case-insensitive check for "Debug" directly underneath :)

set(_BUILD_TYPE_DEBUG OFF)
if(CMAKE_BUILD_TYPE MATCHES "[Dd][Ee][Bb][Uu][Gg]")
    set(_BUILD_TYPE_DEBUG ON)
endif()

Is that check on the build type really necessary? It's not an idiom I've seen elsewhere. I started trying to figure out how to rewrite it cleanly in a case-insensitive way but cmake gave me a headache so I just removed it completely on my fork to get my build working.

anderslanglands avatar Jul 03 '22 09:07 anderslanglands

I would also vote to remove that check as you proposed, I'm not sure it should be the project's responsibility to check that. @hodoulp do you have reasons to keep that check (seeing that you included it in a previous PR)?

remia avatar Jul 04 '22 17:07 remia

The goal of the check is to validate that the build is doing what it's expected to do. If a dev requests debug then the build must do in debug mode or fail. If a dev request an unknown build type (e.g. debugg) then it must fail right away and not fallback to a default build type. Ideally, the check must be case insensitive.

hodoulp avatar Jul 04 '22 18:07 hodoulp

This is now fixed via PR #1765. Thanks for reporting @anderslanglands !

doug-walker avatar Mar 17 '23 23:03 doug-walker