From eff9e2f4ceb1e322b1a19b0b17a4b1dc803c2de4 Mon Sep 17 00:00:00 2001 From: Ray molenkamp Date: Tue, 27 Jun 2023 20:57:50 +0200 Subject: [PATCH] CMake: plumbing for modern CMake usage This is the minimal change required to start using modern CMake in the blender build system. This change is designed to allow small incremental changes to the build system rather than doing it in one big bang which would be unmaintainable (for me) The biggest functional change is, previously all libraries in the `LIB` section of a `blender_add_lib` call had the `INTERFACE` scope, which is rarely, if ever the correct scope. This diff changes this to `PRIVATE` Concrete implications of this diff : The `LIB`, `INC` and `INC_SYS` sections of an `blender_add_lib` call now allow scoping keywords (`PUBLIC`, `PRIVATE,` `INTERFACE`) to declare the scope of the dependency. Right now the only library using any modern cmake is `bf_intern_atomic` which is an header only interface library that will just advertise its include directories. This allows us to clean up any `CMakeLists.txt` that adds `../../../intern/atomic` to its `INC` section to remove it in `INC` by adding a `PRIVATE bf_intern_atomic` to the `LIB` section. Pull Request: https://projects.blender.org/blender/blender/pulls/107858 --- build_files/cmake/Modules/GTestTesting.cmake | 38 +---- build_files/cmake/macros.cmake | 154 +++++++++++++----- intern/atomic/CMakeLists.txt | 20 +++ intern/cycles/test/CMakeLists.txt | 3 +- .../blenlib/tests/performance/CMakeLists.txt | 13 +- source/creator/CMakeLists.txt | 2 +- tests/gtests/runner/CMakeLists.txt | 4 +- 7 files changed, 159 insertions(+), 75 deletions(-) diff --git a/build_files/cmake/Modules/GTestTesting.cmake b/build_files/cmake/Modules/GTestTesting.cmake index db93cd79c85..e1a07a6d611 100644 --- a/build_files/cmake/Modules/GTestTesting.cmake +++ b/build_files/cmake/Modules/GTestTesting.cmake @@ -47,17 +47,17 @@ macro(BLENDER_SRC_GTEST_EX) target_compile_definitions(${TARGET_NAME} PRIVATE ${GLOG_DEFINES}) target_include_directories(${TARGET_NAME} PUBLIC "${TEST_INC}") target_include_directories(${TARGET_NAME} SYSTEM PUBLIC "${TEST_INC_SYS}") - target_link_libraries(${TARGET_NAME} ${ARG_EXTRA_LIBS} ${PLATFORM_LINKLIBS}) + blender_link_libraries(${TARGET_NAME} "${ARG_EXTRA_LIBS};${PLATFORM_LINKLIBS}") if(WITH_TBB) # Force TBB libraries to be in front of MKL (part of OpenImageDenoise), so # that it is initialized before MKL and static library initialization order # issues are avoided. - target_link_libraries(${TARGET_NAME} ${TBB_LIBRARIES}) + target_link_libraries(${TARGET_NAME} PRIVATE ${TBB_LIBRARIES}) if(WITH_OPENIMAGEDENOISE) - target_link_libraries(${TARGET_NAME} ${OPENIMAGEDENOISE_LIBRARIES}) + target_link_libraries(${TARGET_NAME} PRIVATE ${OPENIMAGEDENOISE_LIBRARIES}) endif() endif() - target_link_libraries(${TARGET_NAME} + target_link_libraries(${TARGET_NAME} PRIVATE bf_testing_main bf_intern_eigen bf_intern_guardedalloc @@ -68,16 +68,16 @@ macro(BLENDER_SRC_GTEST_EX) ${GLOG_LIBRARIES} ${GFLAGS_LIBRARIES}) if(WITH_OPENMP_STATIC) - target_link_libraries(${TARGET_NAME} ${OpenMP_LIBRARIES}) + target_link_libraries(${TARGET_NAME} PRIVATE ${OpenMP_LIBRARIES}) endif() if(UNIX AND NOT APPLE) - target_link_libraries(${TARGET_NAME} bf_intern_libc_compat) + target_link_libraries(${TARGET_NAME} PRIVATE bf_intern_libc_compat) endif() if(WITH_TBB) - target_link_libraries(${TARGET_NAME} ${TBB_LIBRARIES}) + target_link_libraries(${TARGET_NAME} PRIVATE ${TBB_LIBRARIES}) endif() if(WITH_GMP) - target_link_libraries(${TARGET_NAME} ${GMP_LIBRARIES}) + target_link_libraries(${TARGET_NAME} PRIVATE ${GMP_LIBRARIES}) endif() GET_BLENDER_TEST_INSTALL_DIR(TEST_INSTALL_DIR) @@ -109,25 +109,3 @@ macro(BLENDER_SRC_GTEST_EX) unset(TARGET_NAME) endif() endmacro() - -macro(BLENDER_SRC_GTEST NAME SRC EXTRA_LIBS) - BLENDER_SRC_GTEST_EX( - NAME "${NAME}" - SRC "${SRC}" - EXTRA_LIBS "${EXTRA_LIBS}") -endmacro() - -macro(BLENDER_TEST NAME EXTRA_LIBS) - BLENDER_SRC_GTEST_EX( - NAME "${NAME}" - SRC "${NAME}_test.cc" - EXTRA_LIBS "${EXTRA_LIBS}") -endmacro() - -macro(BLENDER_TEST_PERFORMANCE NAME EXTRA_LIBS) - BLENDER_SRC_GTEST_EX( - NAME "${NAME}" - SRC "${NAME}_test.cc" - EXTRA_LIBS "${EXTRA_LIBS}" - SKIP_ADD_TEST) -endmacro() diff --git a/build_files/cmake/macros.cmake b/build_files/cmake/macros.cmake index dc8b54db0ce..08513a4d7a1 100644 --- a/build_files/cmake/macros.cmake +++ b/build_files/cmake/macros.cmake @@ -159,31 +159,60 @@ function(absolute_include_dirs set(_ALL_INCS "") foreach(_INC ${ARGN}) - get_filename_component(_ABS_INC ${_INC} ABSOLUTE) - list(APPEND _ALL_INCS ${_ABS_INC}) - # for checking for invalid includes, disable for regular use - # if(NOT EXISTS "${_ABS_INC}/") - # message(FATAL_ERROR "Include not found: ${_ABS_INC}/") - # endif() + # Pass any scoping keywords as is + if(("${_INC}" STREQUAL "PUBLIC") OR + ("${_INC}" STREQUAL "PRIVATE") OR + ("${_INC}" STREQUAL "INTERFACE")) + list(APPEND _ALL_INCS ${_INC}) + else() + get_filename_component(_ABS_INC ${_INC} ABSOLUTE) + list(APPEND _ALL_INCS ${_ABS_INC}) + # for checking for invalid includes, disable for regular use + # if(NOT EXISTS "${_ABS_INC}/") + # message(FATAL_ERROR "Include not found: ${_ABS_INC}/") + # endif() + endif() endforeach() set(${includes_absolute} ${_ALL_INCS} PARENT_SCOPE) endfunction() -function(blender_target_include_dirs - name +function(blender_target_include_dirs_impl + target + system + includes ) + set(next_interface_mode "PRIVATE") + foreach(_INC ${includes}) + if(("${_INC}" STREQUAL "PUBLIC") OR + ("${_INC}" STREQUAL "PRIVATE") OR + ("${_INC}" STREQUAL "INTERFACE")) + set(next_interface_mode "${_INC}") + else() + if(system) + target_include_directories(${target} SYSTEM ${next_interface_mode} ${_INC}) + else() + target_include_directories(${target} ${next_interface_mode} ${_INC}) + endif() + set(next_interface_mode "PRIVATE") + endif() + endforeach() +endfunction() +# Nicer makefiles with -I/1/foo/ instead of -I/1/2/3/../../foo/ +# use it instead of target_include_directories() +function(blender_target_include_dirs + target + ) absolute_include_dirs(_ALL_INCS ${ARGN}) - target_include_directories(${name} PRIVATE ${_ALL_INCS}) + blender_target_include_dirs_impl(${target} FALSE "${_ALL_INCS}") endfunction() function(blender_target_include_dirs_sys - name + target ) - absolute_include_dirs(_ALL_INCS ${ARGN}) - target_include_directories(${name} SYSTEM PRIVATE ${_ALL_INCS}) + blender_target_include_dirs_impl(${target} TRUE "${_ALL_INCS}") endfunction() # Set include paths for header files included with "*.h" syntax. @@ -277,23 +306,11 @@ macro(add_cc_flags_custom_test endmacro() - -# only MSVC uses SOURCE_GROUP -function(blender_add_lib__impl - name - sources - includes - includes_sys +function(blender_link_libraries + target library_deps ) - # message(STATUS "Configuring library ${name}") - - add_library(${name} ${sources}) - - blender_target_include_dirs(${name} ${includes}) - blender_target_include_dirs_sys(${name} ${includes_sys}) - # On Windows certain libraries have two sets of binaries: one for debug builds and one for # release builds. The root of this requirement goes into ABI, I believe, but that's outside # of a scope of this comment. @@ -331,23 +348,49 @@ function(blender_add_lib__impl # NOT: "optimized libfoo libbar debug libfoo_d libbar_d" if(NOT "${library_deps}" STREQUAL "") set(next_library_mode "") + set(next_interface_mode "PRIVATE") foreach(library ${library_deps}) string(TOLOWER "${library}" library_lower) if(("${library_lower}" STREQUAL "optimized") OR ("${library_lower}" STREQUAL "debug")) set(next_library_mode "${library_lower}") + elseif(("${library}" STREQUAL "PUBLIC") OR + ("${library}" STREQUAL "PRIVATE") OR + ("${library}" STREQUAL "INTERFACE")) + set(next_interface_mode "${library}") else() if("${next_library_mode}" STREQUAL "optimized") - target_link_libraries(${name} INTERFACE optimized ${library}) + target_link_libraries(${target} ${next_interface_mode} optimized ${library}) elseif("${next_library_mode}" STREQUAL "debug") - target_link_libraries(${name} INTERFACE debug ${library}) + target_link_libraries(${target} ${next_interface_mode} debug ${library}) else() - target_link_libraries(${name} INTERFACE ${library}) + target_link_libraries(${target} ${next_interface_mode} ${library}) endif() set(next_library_mode "") endif() endforeach() endif() +endfunction() + +# only MSVC uses SOURCE_GROUP +function(blender_add_lib__impl + name + sources + includes + includes_sys + library_deps + ) + + # message(STATUS "Configuring library ${name}") + + add_library(${name} ${sources}) + + blender_target_include_dirs(${name} ${includes}) + blender_target_include_dirs_sys(${name} ${includes_sys}) + + if(library_deps) + blender_link_libraries(${name} "${library_deps}") + endif() # works fine without having the includes # listed is helpful for IDE's (QtCreator/MSVC) @@ -477,8 +520,9 @@ endfunction() # To be used for smaller isolated libraries, that do not have many dependencies. # For libraries that do drag in many other Blender libraries and would create a # very large executable, blender_add_test_lib() should be used instead. -function(blender_add_test_executable +function(blender_add_test_executable_impl name + add_test_suite sources includes includes_sys @@ -496,14 +540,48 @@ function(blender_add_test_executable EXTRA_LIBS "${library_deps}" SKIP_ADD_TEST ) - + if(add_test_suite) + blender_add_test_suite( + TARGET ${name}_test + SUITE_NAME ${name} + SOURCES "${sources}" + ) + endif() blender_target_include_dirs(${name}_test ${includes}) blender_target_include_dirs_sys(${name}_test ${includes_sys}) +endfunction() - blender_add_test_suite( - TARGET ${name}_test - SUITE_NAME ${name} - SOURCES "${sources}" +function(blender_add_test_executable + name + sources + includes + includes_sys + library_deps + ) + blender_add_test_executable_impl( + "${name}" + TRUE + "${sources}" + "${includes}" + "${includes_sys}" + "${library_deps}" + ) +endfunction() + +function(blender_add_performancetest_executable + name + sources + includes + includes_sys + library_deps + ) + blender_add_test_executable_impl( + "${name}" + FALSE + "${sources}" + "${includes}" + "${includes_sys}" + "${library_deps}" ) endfunction() @@ -547,17 +625,17 @@ function(setup_platform_linker_libs ) # jemalloc must be early in the list, to be before pthread (see #57998). if(WITH_MEM_JEMALLOC) - target_link_libraries(${target} ${JEMALLOC_LIBRARIES}) + target_link_libraries(${target} PRIVATE ${JEMALLOC_LIBRARIES}) endif() if(WIN32 AND NOT UNIX) if(DEFINED PTHREADS_LIBRARIES) - target_link_libraries(${target} ${PTHREADS_LIBRARIES}) + target_link_libraries(${target} PRIVATE ${PTHREADS_LIBRARIES}) endif() endif() # target_link_libraries(${target} ${PLATFORM_LINKLIBS} ${CMAKE_DL_LIBS}) - target_link_libraries(${target} ${PLATFORM_LINKLIBS}) + target_link_libraries(${target} PRIVATE ${PLATFORM_LINKLIBS}) endfunction() macro(TEST_SSE_SUPPORT diff --git a/intern/atomic/CMakeLists.txt b/intern/atomic/CMakeLists.txt index 7307007525d..42ab86d1ca5 100644 --- a/intern/atomic/CMakeLists.txt +++ b/intern/atomic/CMakeLists.txt @@ -9,6 +9,25 @@ set(INC set(INC_SYS ) +add_library(bf_intern_atomic INTERFACE) +target_include_directories(bf_intern_atomic INTERFACE .) + +# CMake 3.19+ allows one to populate the interface library with +# source files to show in the IDE, for people on older CMake versions +# these headers will be visible in the bf_intern_guardedalloc project +# where they historically have been. +if(${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.19") + set(SRC + atomic_ops.h + intern/atomic_ops_ext.h + intern/atomic_ops_msvc.h + intern/atomic_ops_unix.h + intern/atomic_ops_utils.h + ) + target_sources(bf_intern_atomic PRIVATE ${SRC}) + blender_source_group(bf_intern_atomic ${SRC}) +endif() + if(WITH_GTESTS) set(TEST_SRC tests/atomic_test.cc @@ -16,6 +35,7 @@ if(WITH_GTESTS) set(TEST_INC ) set(TEST_LIB + PRIVATE bf_intern_atomic ) include(GTestTesting) blender_add_test_executable(atomic "${TEST_SRC}" "${INC};${TEST_INC}" "${INC_SYS}" "${LIB};${TEST_LIB}") diff --git a/intern/cycles/test/CMakeLists.txt b/intern/cycles/test/CMakeLists.txt index 191d03cf1e9..1e74b160e40 100644 --- a/intern/cycles/test/CMakeLists.txt +++ b/intern/cycles/test/CMakeLists.txt @@ -55,5 +55,6 @@ if(NOT APPLE) endif() if(WITH_GTESTS AND WITH_CYCLES_LOGGING) - blender_src_gtest(cycles "${SRC}" "${LIB}") + set(INC_SYS ) + blender_add_test_executable(cycles "${SRC}" "${INC}" "${INC_SYS}" "${LIB}") endif() diff --git a/source/blender/blenlib/tests/performance/CMakeLists.txt b/source/blender/blenlib/tests/performance/CMakeLists.txt index dd1e61165fb..531bdf8a2fb 100644 --- a/source/blender/blenlib/tests/performance/CMakeLists.txt +++ b/source/blender/blenlib/tests/performance/CMakeLists.txt @@ -11,7 +11,14 @@ set(INC ../../../../../intern/guardedalloc ) -include_directories(${INC}) +set(INC_SYS +) -blender_test_performance(BLI_ghash_performance "bf_blenlib") -blender_test_performance(BLI_task_performance "bf_blenlib") +set(LIB + PRIVATE bf_blenlib + PRIVATE bf_intern_guardedalloc + PRIVATE bf_intern_atomic +) + +blender_add_performancetest_executable(BLI_ghash_performance "BLI_ghash_performance_test.cc" "${INC}" "${INC_SYS}" "${LIB}") +blender_add_performancetest_executable(BLI_task_performance "BLI_task_performance_test.cc" "${INC}" "${INC_SYS}" "${LIB}") diff --git a/source/creator/CMakeLists.txt b/source/creator/CMakeLists.txt index f3ba78490fc..8f572d71a81 100644 --- a/source/creator/CMakeLists.txt +++ b/source/creator/CMakeLists.txt @@ -1575,7 +1575,7 @@ endif() # Setup link libraries add_dependencies(blender makesdna) -target_link_libraries(blender ${LIB}) +target_link_libraries(blender PRIVATE ${LIB}) unset(LIB) setup_platform_linker_flags(blender) diff --git a/tests/gtests/runner/CMakeLists.txt b/tests/gtests/runner/CMakeLists.txt index 5e832545db4..99ca99d951a 100644 --- a/tests/gtests/runner/CMakeLists.txt +++ b/tests/gtests/runner/CMakeLists.txt @@ -41,7 +41,7 @@ if(WIN32) # Both target_link_libraries and target_link_options are required here # target_link_libraries will add any dependent libraries, while just setting # the wholearchive flag in target link options will not. - target_link_libraries(blender_test ${_lib}) + target_link_libraries(blender_test PRIVATE ${_lib}) target_link_options(blender_test PRIVATE /wholearchive:$) endforeach() set_target_properties(blender_test PROPERTIES VS_DEBUGGER_ENVIRONMENT "${PLATFORM_ENV_INSTALL};$") @@ -49,7 +49,7 @@ elseif(APPLE) foreach(_lib ${_test_libs}) # We need -force_load for every test library and target_link_libraries will # deduplicate it. So explicitly set as linker option for every test lib. - target_link_libraries(blender_test ${_lib} "-Wl,-force_load,$") + target_link_libraries(blender_test PRIVATE ${_lib} "-Wl,-force_load,$") endforeach() endif()