build: switch to target-based definitions (#4193)

We had lots of stray usage of add_defintions, add_compile_options, etc. In modern cmake, this is considered bad form, since it makes subprojects pollute the global option lists. A better way is to add all these to specific targets, so now that's what we do in this PR. This should make all the definitions and options we want to set for compilng OIIO's code not have any interference with subprojects, or with higher-level surrounding projects. Signed-off-by: Larry Gritz <[email protected]>
AcademySoftwareFoundation · Mar 26, 2024 · 45a91c9 · 45a91c9
1 parent 5217ca3
commit 45a91c9
Show file tree

Hide file tree

Showing 10 changed files with 230 additions and 170 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -119,18 +119,9 @@ message(STATUS "CMAKE_UNITY_BUILD_MODE = ${CMAKE_UNITY_BUILD_MODE}")
 message(STATUS "CMAKE_UNITY_BUILD_BATCH_SIZE = ${CMAKE_UNITY_BUILD_BATCH_SIZE}")
 
 option (OIIO_THREAD_ALLOW_DCLP "OIIO threads may use DCLP for speed" ON)
-if (NOT OIIO_THREAD_ALLOW_DCLP)
-    add_definitions ("-DOIIO_THREAD_ALLOW_DCLP=0")
-endif ()
 
 set (TEX_BATCH_SIZE "" CACHE STRING "Force TextureSystem SIMD batch size (e.g. 16)")
-if (TEX_BATCH_SIZE)
-    add_definitions ("-DOIIO_TEXTURE_SIMD_BATCH_WIDTH=${TEX_BATCH_SIZE}")
-endif ()
 option (OIIO_TEX_IMPLEMENT_VARYINGREF "Implement the deprecated batch texture functions taking VaryingRef params" ON)
-if (NOT OIIO_TEX_IMPLEMENT_VARYINGREF)
-    add_definitions (-DOIIO_TEX_NO_IMPLEMENT_VARYINGREF=1)
-endif ()
 
 # Set the default namespace
 set (${PROJ_NAME}_NAMESPACE ${PROJECT_NAME} CACHE STRING
@@ -145,10 +136,6 @@ endif ()
 message(STATUS "Setting Namespace to: ${PROJ_NAMESPACE_V}")
 
 
-# Define OIIO_INTERNAL symbol only when building OIIO itself, will not be
-# defined for downstream projects using OIIO.
-add_definitions (-DOIIO_INTERNAL=1)
-
 list (APPEND CMAKE_MODULE_PATH
       "${PROJECT_SOURCE_DIR}/src/cmake/modules"
       "${PROJECT_SOURCE_DIR}/src/cmake")
@@ -190,11 +177,26 @@ include_directories (
     "${CMAKE_BINARY_DIR}/include/OpenImageIO"
   )
 
-# Tell CMake to process the sub-directories
-add_subdirectory (src/libutil)
+
+# Define OIIO_INTERNAL symbol only when building OIIO itself, will not be
+# defined for downstream projects using OIIO.
+proj_add_compile_definitions (OIIO_INTERNAL=1)
+
+if (NOT OIIO_THREAD_ALLOW_DCLP)
+    proj_add_compile_definitions (OIIO_THREAD_ALLOW_DCLP=0)
+endif ()
+if (TEX_BATCH_SIZE)
+    proj_add_compile_definitions (OIIO_TEXTURE_SIMD_BATCH_WIDTH=${TEX_BATCH_SIZE})
+endif ()
+if (NOT OIIO_TEX_IMPLEMENT_VARYINGREF)
+    proj_add_compile_definitions (OIIO_TEX_NO_IMPLEMENT_VARYINGREF=1)
+endif ()
 
 
 
+# Tell CMake to process the sub-directories
+add_subdirectory (src/libutil)
+
 # Add IO plugin directories -- if we are embedding plugins, we need to visit
 # these directories BEFORE the OpenImageIO target is established (in
 # src/libOpenImageIO). For each plugin, we append to the lists of source

diff --git a/src/cmake/checked_find_package.cmake b/src/cmake/checked_find_package.cmake
@@ -39,8 +39,8 @@ endfunction ()
 #     turned off explicitly from one of these sources.
 #   * Print a message if the package is enabled but not found. This is based
 #     on ${Pkgname}_FOUND or $PKGNAME_FOUND.
-#   * Optional DEFINITIONS <string>... are passed to add_definitions if the
-#     package is found.
+#   * Optional DEFINITIONS <string>... are passed to
+#     proj_add_compile_definitions if the package is found.
 #   * Optional SETVARIABLES <id>... is a list of CMake variables to set to
 #     TRUE if the package is found (they will not be set or changed if the
 #     package is not found).
@@ -151,7 +151,7 @@ macro (checked_find_package pkgname)
                 endif ()
             endforeach ()
             message (STATUS "${ColorGreen}Found ${pkgname} ${${pkgname}_VERSION} ${_config_status}${ColorReset}")
-            add_definitions (${_pkg_DEFINITIONS})
+            proj_add_compile_definitions (${_pkg_DEFINITIONS})
             foreach (_v IN LISTS _pkg_SETVARIABLES)
                 set (${_v} TRUE)
             endforeach ()

diff --git a/src/cmake/compiler.cmake b/src/cmake/compiler.cmake
@@ -23,6 +23,28 @@ message (STATUS  "CMAKE_CXX_COMPILER_ID  = ${CMAKE_CXX_COMPILER_ID}")
 message (VERBOSE "CMAKE_CXX_COMPILE_FEATURES = ${CMAKE_CXX_COMPILE_FEATURES}")
 
 
+###########################################################################
+# The proj_add_compile_definitions, proj_add_compile_options, and
+# proj_add_link_options are like the global add_compile_definitions (etc), but
+# they merely add to ${PROJECT_NAME}_blah lists, which are expected to be
+# added to library and executable targets in our project. The point is that
+# we really shouldn't be polluting the global definitions, in case our
+# cmake files are included in an "outer" project.
+#
+macro (proj_add_compile_definitions)
+    list (APPEND ${PROJECT_NAME}_compile_definitions ${ARGN})
+endmacro ()
+
+macro (proj_add_compile_options)
+    list (APPEND ${PROJECT_NAME}_compile_options ${ARGN})
+endmacro ()
+
+macro (proj_add_link_options)
+    list (APPEND ${PROJECT_NAME}_link_options ${ARGN})
+endmacro ()
+
+
+
 ###########################################################################
 # C++ language standard
 #
@@ -92,12 +114,12 @@ else ()
 endif()
 option (EXTRA_WARNINGS "Enable lots of extra pedantic warnings" OFF)
 if (NOT MSVC)
-    add_compile_options ("-Wall")
+    proj_add_compile_options ("-Wall")
     if (EXTRA_WARNINGS)
-        add_compile_options ("-Wextra")
+        proj_add_compile_options ("-Wextra")
     endif ()
     if (STOP_ON_WARNING)
-        add_compile_options ("-Werror")
+        proj_add_compile_options ("-Werror")
     endif ()
 endif ()
 
@@ -135,88 +157,88 @@ endif ()
 #
 if (CMAKE_COMPILER_IS_CLANG OR CMAKE_COMPILER_IS_APPLECLANG)
     # Clang-specific options
-    add_compile_options ("-Wno-unused-function")
-    add_compile_options ("-Wno-overloaded-virtual")
-    add_compile_options ("-Wno-unneeded-internal-declaration")
-    add_compile_options ("-Wno-unused-private-field")
-    add_compile_options ("-Wno-tautological-compare")
+    proj_add_compile_options ("-Wno-unused-function")
+    proj_add_compile_options ("-Wno-overloaded-virtual")
+    proj_add_compile_options ("-Wno-unneeded-internal-declaration")
+    proj_add_compile_options ("-Wno-unused-private-field")
+    proj_add_compile_options ("-Wno-tautological-compare")
     # disable warning about unused command line arguments
-    add_compile_options ("-Qunused-arguments")
+    proj_add_compile_options ("-Qunused-arguments")
     # Don't warn if we ask it not to warn about warnings it doesn't know
-    add_compile_options ("-Wunknown-warning-option")
+    proj_add_compile_options ("-Wunknown-warning-option")
     if (CLANG_VERSION_STRING VERSION_GREATER_EQUAL 3.6 OR
         APPLECLANG_VERSION_STRING VERSION_GREATER 6.1)
-        add_compile_options ("-Wno-unused-local-typedefs")
+        proj_add_compile_options ("-Wno-unused-local-typedefs")
     endif ()
     if (CLANG_VERSION_STRING VERSION_GREATER_EQUAL 3.9)
         # Don't warn about using unknown preprocessor symbols in `#if`
-        add_compile_options ("-Wno-expansion-to-defined")
+        proj_add_compile_options ("-Wno-expansion-to-defined")
     endif ()
     if (CMAKE_GENERATOR MATCHES "Xcode")
-        add_compile_options ("-Wno-shorten-64-to-32")
+        proj_add_compile_options ("-Wno-shorten-64-to-32")
     endif ()
 endif ()
 
 if (CMAKE_COMPILER_IS_GNUCC AND NOT (CMAKE_COMPILER_IS_CLANG OR CMAKE_COMPILER_IS_APPLECLANG))
     # gcc specific options
-    add_compile_options ("-Wno-unused-local-typedefs")
-    add_compile_options ("-Wno-unused-result")
+    proj_add_compile_options ("-Wno-unused-local-typedefs")
+    proj_add_compile_options ("-Wno-unused-result")
     if (NOT ${GCC_VERSION} VERSION_LESS 7.0)
-        add_compile_options ("-Wno-aligned-new")
-        add_compile_options ("-Wno-noexcept-type")
+        proj_add_compile_options ("-Wno-aligned-new")
+        proj_add_compile_options ("-Wno-noexcept-type")
     endif ()
 endif ()
 
 if (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_CLANG)
     # Options common to gcc and clang
 
     # Ensure this macro is set for stdint.h
-    add_definitions ("-D__STDC_LIMIT_MACROS")
-    add_definitions ("-D__STDC_CONSTANT_MACROS")
+    proj_add_compile_definitions ("-D__STDC_LIMIT_MACROS")
+    proj_add_compile_definitions ("-D__STDC_CONSTANT_MACROS")
 endif ()
 
 if (INTELCLANG_VERSION_STRING VERSION_GREATER_EQUAL 2022.1.0)
     # New versions of icx warn about changing certain floating point options
-    add_compile_options ("-Wno-overriding-t-option")
+    proj_add_compile_options ("-Wno-overriding-t-option")
 endif ()
 
 if (MSVC)
     # Microsoft specific options
-    add_compile_options (/W1)
-    add_compile_options (/MP)
-    add_definitions (-D_CRT_SECURE_NO_DEPRECATE)
-    add_definitions (-D_CRT_SECURE_NO_WARNINGS)
-    add_definitions (-D_CRT_NONSTDC_NO_WARNINGS)
-    add_definitions (-D_SCL_SECURE_NO_WARNINGS)
-    add_definitions (-DJAS_WIN_MSVC_BUILD)
+    proj_add_compile_options (/W1)
+    proj_add_compile_options (/MP)
+    proj_add_compile_definitions (-D_CRT_SECURE_NO_DEPRECATE)
+    proj_add_compile_definitions (-D_CRT_SECURE_NO_WARNINGS)
+    proj_add_compile_definitions (-D_CRT_NONSTDC_NO_WARNINGS)
+    proj_add_compile_definitions (-D_SCL_SECURE_NO_WARNINGS)
+    proj_add_compile_definitions (-DJAS_WIN_MSVC_BUILD)
 endif (MSVC)
 
 if (${CMAKE_SYSTEM_NAME} STREQUAL "FreeBSD"
     AND ${CMAKE_SYSTEM_PROCESSOR} STREQUAL "i386")
     # For FreeBSD, minimum arch of i586 is needed for atomic cpu instructions
-    add_compile_options (-march=i586)
+    proj_add_compile_options (-march=i586)
 endif ()
 
 # Fast-math mode may go faster, but it breaks IEEE and also makes inconsistent
 # results on different compilers/platforms, so we don't use it by default.
 option (ENABLE_FAST_MATH "Use fast math (may break IEEE fp rules)" OFF)
 if (ENABLE_FAST_MATH)
     if (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_CLANG)
-        add_compile_options ("-ffast-math")
+        proj_add_compile_options ("-ffast-math")
     elseif (MSVC)
-        add_compile_options ("/fp:fast")
+        proj_add_compile_options ("/fp:fast")
     endif ()
 else ()
     if (CMAKE_COMPILER_IS_INTELCLANG)
         # Intel icx is fast-math by default, so if we don't want that, we need
         # to explicitly disable it.
-        add_compile_options ("-fno-fast-math")
+        proj_add_compile_options ("-fno-fast-math")
     endif ()
 endif ()
 
 if (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_CLANG)
     # this allows native instructions to be used for sqrtf instead of a function call
-    add_compile_options ("-fno-math-errno")
+    proj_add_compile_options ("-fno-math-errno")
 endif ()
 
 
@@ -285,7 +307,7 @@ endif ()
 set (GLIBCXX_USE_CXX11_ABI "" CACHE STRING "For gcc, use the new C++11 library ABI (0|1)")
 if (CMAKE_COMPILER_IS_GNUCC AND ${GCC_VERSION} VERSION_GREATER_EQUAL 5.0)
     if (NOT ${GLIBCXX_USE_CXX11_ABI} STREQUAL "")
-        add_definitions ("-D_GLIBCXX_USE_CXX11_ABI=${GLIBCXX_USE_CXX11_ABI}")
+        proj_add_compile_definitions ("-D_GLIBCXX_USE_CXX11_ABI=${GLIBCXX_USE_CXX11_ABI}")
     endif ()
 endif ()
 
@@ -309,20 +331,20 @@ if (NOT USE_SIMD STREQUAL "")
         foreach (feature ${SIMD_FEATURE_LIST})
             message (VERBOSE "SIMD feature: ${feature}")
             if (MSVC OR CMAKE_COMPILER_IS_INTEL)
-                set (SIMD_COMPILE_FLAGS ${SIMD_COMPILE_FLAGS} "/arch:${feature}")
+                list (APPEND SIMD_COMPILE_FLAGS "/arch:${feature}")
             else ()
-                set (SIMD_COMPILE_FLAGS ${SIMD_COMPILE_FLAGS} "-m${feature}")
+                list (APPEND SIMD_COMPILE_FLAGS "-m${feature}")
             endif ()
             if (feature STREQUAL "fma" AND (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_CLANG))
                 # If fma is requested, for numerical accuracy sake, turn it
                 # off by default except when we explicitly use madd. At some
                 # future time, we should look at this again carefully and
                 # see if we want to use it more widely by ffp-contract=fast.
-                add_compile_options ("-ffp-contract=off")
+                proj_add_compile_options ("-ffp-contract=off")
             endif ()
         endforeach()
     endif ()
-    add_compile_options (${SIMD_COMPILE_FLAGS})
+    proj_add_compile_options (${SIMD_COMPILE_FLAGS})
 endif ()
 
 
@@ -382,10 +404,10 @@ endif ()
 if (USE_STD_FILESYSTEM)
     # Note: std::filesystem seems unreliable for gcc until 9
     message (STATUS "Compiler supports std::filesystem")
-    add_definitions (-DUSE_STD_FILESYSTEM)
+    proj_add_compile_definitions (-DUSE_STD_FILESYSTEM)
 else ()
     message (STATUS "Using Boost::filesystem")
-    add_definitions (-DUSE_BOOST_FILESYSTEM)
+    proj_add_compile_definitions (-DUSE_BOOST_FILESYSTEM)
 endif ()
 cmake_pop_check_state ()
 
@@ -396,9 +418,9 @@ cmake_pop_check_state ()
 option (CODECOV "Build code coverage tests" OFF)
 if (CODECOV AND (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_CLANG))
     message (STATUS "Compiling for code coverage analysis")
-    add_compile_options (-ftest-coverage -fprofile-arcs)
-    add_link_options (-ftest-coverage -fprofile-arcs)
-    add_definitions ("-D${PROJ_NAME}_CODE_COVERAGE=1")
+    proj_add_compile_options (-ftest-coverage -fprofile-arcs)
+    proj_add_link_options (-ftest-coverage -fprofile-arcs)
+    proj_add_compile_definitions ("-D${PROJ_NAME}_CODE_COVERAGE=1")
 endif ()
 
 
@@ -411,15 +433,15 @@ if (SANITIZE AND (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_CLANG))
     string (REPLACE "," ";" SANITIZE_FEATURE_LIST ${SANITIZE})
     foreach (feature ${SANITIZE_FEATURE_LIST})
         message (STATUS "  sanitize feature: ${feature}")
-        add_compile_options (-fsanitize=${feature})
-        add_link_options (-fsanitize=${feature})
+        proj_add_compile_options (-fsanitize=${feature})
+        proj_add_link_options (-fsanitize=${feature})
     endforeach()
-    add_compile_options (-g -fno-omit-frame-pointer)
+    proj_add_compile_options (-g -fno-omit-frame-pointer)
     if (CMAKE_COMPILER_IS_GNUCC)
         # turn on glibcxx extra annotations to find vector writes past end
-        add_definitions ("-D_GLIBCXX_SANITIZE_VECTOR=1")
+        proj_add_compile_definitions ("-D_GLIBCXX_SANITIZE_VECTOR=1")
     endif ()
-    add_definitions ("-D${PROJECT_NAME}_SANITIZE=1")
+    proj_add_compile_definitions ("-D${PROJECT_NAME}_SANITIZE=1")
 endif ()
 
 
@@ -442,7 +464,7 @@ endif ()
 set (FORTIFY_SOURCE "0" CACHE STRING "Turn on Fortification level (0, 1, 2, 3)")
 if (FORTIFY_SOURCE AND (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_CLANG))
     message (STATUS "Compiling with _FORTIFY_SOURCE=${FORTIFY_SOURCE}")
-    add_compile_options (-D_FORTIFY_SOURCE=${FORTIFY_SOURCE})
+    proj_add_compile_options (-D_FORTIFY_SOURCE=${FORTIFY_SOURCE})
 endif ()
 
 
@@ -541,7 +563,7 @@ endif ()
 set (EXTRA_CPP_ARGS "" CACHE STRING "Extra C++ command line definitions")
 if (EXTRA_CPP_ARGS)
     message (STATUS "Extra C++ args: ${EXTRA_CPP_ARGS}")
-    add_compile_options (${EXTRA_CPP_ARGS})
+    proj_add_compile_options (${EXTRA_CPP_ARGS})
 endif()
 set (EXTRA_DSO_LINK_ARGS "" CACHE STRING "Extra command line definitions when building DSOs")
 
@@ -569,7 +591,7 @@ message(VERBOSE "Setting SOVERSION to: ${SOVERSION}")
 #
 option (BUILD_SHARED_LIBS "Build shared libraries (set to OFF to build static libs)" ON)
 if (NOT BUILD_SHARED_LIBS)
-    add_definitions (-D${PROJ_NAME}_STATIC_DEFINE=1)
+    proj_add_compile_definitions (-D${PROJ_NAME}_STATIC_DEFINE=1)
 endif ()
 
 
@@ -594,7 +616,7 @@ endif ()
 # We expect our own CI runs to define env variable ${PROJECT_NAME}_CI
 #
 if (DEFINED ENV{${PROJECT_NAME}_CI})
-    add_definitions (-D${PROJ_NAME}_CI=1 -DBUILD_CI=1)
+    proj_add_compile_definitions (-D${PROJ_NAME}_CI=1 -DBUILD_CI=1)
     if (APPLE)
         # Keep Mono framework from being incorrectly searched for include
         # files on GitHub Actions CI.

diff --git a/src/cmake/externalpackages.cmake b/src/cmake/externalpackages.cmake
@@ -36,7 +36,7 @@ include (FindThreads)
 # Boost setup
 if (MSVC)
     # Disable automatic linking using pragma comment(lib,...) of boost libraries upon including of a header
-    add_definitions (-DBOOST_ALL_NO_LIB=1)
+    proj_add_compile_definitions (BOOST_ALL_NO_LIB=1)
 endif ()
 
 # If the build system hasn't been specifically told how to link Boost, link it the same way as other
@@ -48,7 +48,7 @@ endif ()
 if (MSVC)
     # Not linking Boost as static libraries: either an explicit setting or LINKSTATIC is FALSE:
     if (NOT Boost_USE_STATIC_LIBS)
-        add_definitions (-DBOOST_ALL_DYN_LINK=1)
+        proj_add_compile_definitions (BOOST_ALL_DYN_LINK=1)
     endif ()
 endif ()
 
@@ -109,7 +109,7 @@ checked_find_package (OpenEXR REQUIRED
 # install version of 2.x.
 include_directories(BEFORE ${IMATH_INCLUDES} ${OPENEXR_INCLUDES})
 if (MSVC AND NOT LINKSTATIC)
-    add_definitions (-DOPENEXR_DLL) # Is this needed for new versions?
+    proj_add_compile_definitions (OPENEXR_DLL) # Is this needed for new versions?
 endif ()
 if (OpenEXR_VERSION VERSION_GREATER_EQUAL 3.0)
     set (OIIO_USING_IMATH 3)
@@ -191,7 +191,7 @@ if (OpenColorIO_FOUND)
     option (OIIO_DISABLE_BUILTIN_OCIO_CONFIGS
            "For deveoper debugging/testing ONLY! Disable OCIO 2.2 builtin configs." OFF)
     if (OIIO_DISABLE_BUILTIN_OCIO_CONFIGS OR "$ENV{OIIO_DISABLE_BUILTIN_OCIO_CONFIGS}")
-        add_compile_definitions(OIIO_DISABLE_BUILTIN_OCIO_CONFIGS)
+        proj_add_compile_definitions(OIIO_DISABLE_BUILTIN_OCIO_CONFIGS)
     endif ()
 else ()
     set (OpenColorIO_FOUND 0)