Repository navigation
Avoid checking out unused CMake dependency submodules #4725
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -307,44 +307,94 @@ endforeach() | |
|
|
||
| ############################################################################### | ||
| ## Check that found libraries can be linked to catch linking conflicts early. | ||
| # A failed combined link check cannot identify which libraries need rebuilding. | ||
| # TIP: cmake --debug-trycompile keeps the temporary sources and binaries | ||
| option(CHECK_LIBRARY_COMPATIBILITY "Check for library incompatibilities" ON) | ||
| option(REBUILD_LIBRARIES_ON_LINK_FAILURE | ||
| "Rebuild checked libraries instead of stopping on a failed compatibility check" OFF) | ||
|
|
||
| set(CMAKE_REQUIRED_LIBRARIES "") | ||
| set(CMAKE_REQUIRED_INCLUDES "") | ||
| set(CHECKED_LIBRARIES "") | ||
| set(_m2_rebuild_libraries "") | ||
|
|
||
| if(CHECK_LIBRARY_COMPATIBILITY) | ||
| message(CHECK_START " Checking library compatibility") | ||
|
|
||
| foreach(_attempt RANGE 1 2) | ||
| set(CMAKE_REQUIRED_LIBRARIES "") | ||
| set(CHECKED_LIBRARIES "") | ||
| foreach(LIB IN LISTS LIBRARIES_LIST) | ||
| if(${LIB}_FOUND) | ||
| list(APPEND CMAKE_REQUIRED_LIBRARIES ${${LIB}_LIBRARIES}) | ||
| list(APPEND CHECKED_LIBRARIES ${LIB}) | ||
| endif() | ||
| endforeach() | ||
|
|
||
| foreach(LIB IN LISTS PKGLIB_LIST) | ||
| if(${LIB}_FOUND) | ||
| list(APPEND CMAKE_REQUIRED_LIBRARIES PkgConfig::${LIB}) | ||
| list(APPEND CHECKED_LIBRARIES ${LIB}) | ||
| endif() | ||
| endforeach() | ||
|
|
||
| # Retry after paths or versions change, including after a failed configuration. | ||
| unset(LIBRARY_COMPATIBILITY CACHE) | ||
| check_cxx_source_compiles([[int main(){return 0;}]] LIBRARY_COMPATIBILITY | ||
| FAIL_REGEX "conflict" OUTPUT_VARIABLE LIBRARY_COMPATIBILITY_OUTPUT) | ||
| if(LIBRARY_COMPATIBILITY OR _attempt EQUAL 2) | ||
| break() | ||
| endif() | ||
|
|
||
| # Keep compatible local installations even when BUILD_LIBRARIES is set, but | ||
| # allow explicitly selected ones to be repaired after a failed link check. | ||
| # Retest the remaining libraries before generating any rebuild targets. | ||
| foreach(_library IN LISTS INSTALLED_LIBRARIES) | ||
| string(TOUPPER "${_library}" _name) | ||
| if(_name IN_LIST CHECKED_LIBRARIES AND | ||
| (BUILD_LIBRARIES MATCHES "(ALL|ON)" OR _name IN_LIST BUILD_LIBRARIES)) | ||
| unset(${_name}_FOUND CACHE) | ||
| unset(${_name}_FOUND) | ||
| list(APPEND _m2_rebuild_libraries ${_name}) | ||
| list(REMOVE_ITEM INSTALLED_LIBRARIES ${_library}) | ||
| endif() | ||
| endforeach() | ||
| if(NOT _m2_rebuild_libraries) | ||
| break() | ||
| endif() | ||
| message(STATUS "Retrying library compatibility without selected local libraries: ${_m2_rebuild_libraries}") | ||
| endforeach() | ||
|
|
||
| if(NOT LIBRARY_COMPATIBILITY) | ||
| message(CHECK_FAIL " Detected library incompatibilities") | ||
| if(REBUILD_LIBRARIES_ON_LINK_FAILURE) | ||
| message(WARNING | ||
| "Library compatibility check failed; rebuilding checked libraries " | ||
| "because REBUILD_LIBRARIES_ON_LINK_FAILURE=ON:\n" | ||
| " ${CHECKED_LIBRARIES}\n\n${LIBRARY_COMPATIBILITY_OUTPUT}") | ||
| foreach(LIB IN LISTS CHECKED_LIBRARIES) | ||
| unset(${LIB}_FOUND) | ||
| endforeach() | ||
| else() | ||
| message(FATAL_ERROR | ||
| "The detected libraries could not be linked together:\n" | ||
| " ${CHECKED_LIBRARIES}\n\n" | ||
| "Selected GMP headers: ${GMP_INCLUDE_DIRS}\n" | ||
| "Selected GMP libraries: ${GMP_LIBRARIES}\n" | ||
| "CMAKE_PREFIX_PATH: ${CMAKE_PREFIX_PATH}\n\n" | ||
| "Fix the library paths or versions (including GMP), then rerun CMake.\n" | ||
| "To rebuild selected dependencies, configure with " | ||
| "'-DBUILD_LIBRARIES=<LIB1>;<LIB2>'. " | ||
| "Use -DBUILD_LIBRARIES=ALL to explicitly rebuild all supported dependencies.\n\n" | ||
| "To restore automatic rebuilding after a failed link check, configure with " | ||
| "-DREBUILD_LIBRARIES_ON_LINK_FAILURE=ON.\n\n" | ||
| "Link check output:\n${LIBRARY_COMPATIBILITY_OUTPUT}") | ||
|
Member
There was a problem hiding this comment. Did you just pass my gmp library mismatch example to your coding agent?! The example I gave is one of many situations that library incompatibility can happen, and most often the solution is indeed rerunning the build-libraries target, as the message you deleted suggested, and giving a fatal error means the user can't even do that! Moreover, even in the limited gmp version mismatch situation, passing
Contributor
Author
There was a problem hiding this comment. I haven't deleted anything
Contributor
Author
There was a problem hiding this comment. giving a fatal error is not the end. There is an option to explicitly stick to the current behaviour. Moreover, one of the last commits fix a bug which let frobby to shop around for a gmp, rather than using one found by the cmake builder already
Contributor
Author
There was a problem hiding this comment. an incompatibility like this is a bug in the build system. It's not something one is forced to live with forever
Contributor
Author
There was a problem hiding this comment. What happened is that not all such cases were checked. Sorry. A learning curve :-( I started digging why. Apparently, as 🤖 was previously told here not to write tests for cmake files, cf (d2ddc78) it literally replaced tests written in Python it runs on its code with sequences of commands it executed "manually". Not dissimilar to humans here. :-) Perhaps it's not a good idea with AI-written build systems to keep to old habits of not writing tests... |
||
| endif() | ||
| else() | ||
| message(CHECK_PASS " Libraries are compatible!") | ||
| endif() | ||
|
|
||
| unset(LIBRARY_COMPATIBILITY_OUTPUT) | ||
| endif() | ||
|
|
||
| unset(CMAKE_REQUIRED_LIBRARIES) | ||
|
|
||
|
Member
There was a problem hiding this comment. There's already an option to disable cloning submodules, which you could enable on github builds if you'd like. Moving submodule handling all to this file is a good idea, but otherwise the rest of this PR adds unnecessary complexity imo. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| # check-libraries.cmake has already applied BUILD_LIBRARIES overrides. | ||
| # The engine and editor sources are always needed. ExternalProject sources are | ||
| # needed only for missing libraries, or to keep testing a previous local build. | ||
| function(_m2_select_fallback _name) | ||
| string(TOUPPER "${_name}" _upper) | ||
| set(_build FALSE) | ||
| if(NOT ${_upper}_FOUND AND (NOT _name STREQUAL "googletest" OR BUILD_TESTING)) | ||
| set(_build TRUE) | ||
| endif() | ||
| if(EXISTS "${CMAKE_BINARY_DIR}/libraries/${_name}/src/build-${_name}-stamp/build-${_name}-install") | ||
| set(_build TRUE) | ||
| endif() | ||
| set(_m2_build_${_name} ${_build} PARENT_SCOPE) | ||
| endfunction() | ||
|
|
||
| set(_m2_gitmodules "${CMAKE_SOURCE_DIR}/../.gitmodules") | ||
| if(NOT EXISTS "${_m2_gitmodules}") | ||
| if(EXISTS "${CMAKE_SOURCE_DIR}/../.git") | ||
| message(FATAL_ERROR "${_m2_gitmodules} is missing from this Git checkout") | ||
| endif() | ||
| # A source archive may omit .gitmodules. Set the fallback guards from the | ||
| # existing library options; ExternalProject_Add checks their source paths. | ||
| foreach(_library IN LISTS LIBRARY_OPTIONS) | ||
| string(TOLOWER "${_library}" _name) | ||
| if(_name STREQUAL "gtest") | ||
| set(_name googletest) | ||
| endif() | ||
| _m2_select_fallback("${_name}") | ||
| endforeach() | ||
| return() | ||
| endif() | ||
| file(STRINGS "${_m2_gitmodules}" _m2_gitmodule_paths REGEX "^[ \t]*path[ \t]*=") | ||
| # Optional sources use their directory name as the *_FOUND prefix, except | ||
| # googletest, which is detected as GTest. Other submodules are always needed. | ||
| set(_m2_library_options) | ||
| foreach(_library IN LISTS LIBRARY_OPTIONS) | ||
| string(TOUPPER "${_library}" _library_upper) | ||
| list(APPEND _m2_library_options "${_library_upper}") | ||
| endforeach() | ||
| set(_m2_required_submodules) | ||
| set(_m2_update_submodules) | ||
| foreach(_entry IN LISTS _m2_gitmodule_paths) | ||
| string(REGEX REPLACE "^[ \t]*path[ \t]*=[ \t]*" "" _path "${_entry}") | ||
| string(STRIP "${_path}" _path) | ||
| if(NOT _path MATCHES "^M2/") | ||
| continue() | ||
| endif() | ||
| string(REGEX REPLACE "^M2/" "" _path "${_path}") | ||
|
|
||
| if(_path MATCHES "^submodules/([^/]+)$") | ||
| set(_name "${CMAKE_MATCH_1}") | ||
| string(TOUPPER "${_name}" _upper) | ||
| set(_option_name "${_upper}") | ||
| if(_name STREQUAL "googletest") | ||
| set(_option_name GTEST) | ||
| endif() | ||
| if(NOT _option_name IN_LIST _m2_library_options) | ||
| # Engine sources have no system-library alternative. | ||
| set(_m2_build_${_name} TRUE) | ||
| else() | ||
| _m2_select_fallback("${_name}") | ||
| endif() | ||
| if(_m2_build_${_name}) | ||
| list(APPEND _m2_required_submodules "${_path}") | ||
| list(APPEND _m2_update_submodules "${_path}") | ||
| endif() | ||
| else() | ||
| # Keep the editor submodule's existing warning/skip behavior when absent. | ||
| list(APPEND _m2_update_submodules "${_path}") | ||
| endif() | ||
| endforeach() | ||
| if(NOT _m2_update_submodules) | ||
| message(FATAL_ERROR "No M2 submodule paths found in ${_m2_gitmodules}") | ||
| endif() | ||
|
Comment on lines
+42
to
+74
Member
There was a problem hiding this comment. @antonleykin, I understand exactly how the submodule mechanics works, and genuinely do not think this is a simplification of what we already have. It shaves ~20 seconds off from CMake's configure step, but trying to decipher this block here wasted 20 minutes of my time and I still don't fully understand it. As I see it, merging it makes the system more complicated and dependent on a coding agent for maintenance. That said, as I said in the other CMake PR, feel free to disregard my review and merge if anyone who wants it can say that they 100% understand it and take responsibility for its maintenance. |
||
|
|
||
| if(GIT_SUBMODULE AND GIT_FOUND AND EXISTS "${CMAKE_SOURCE_DIR}/../.git") | ||
| message(STATUS "Updating required submodules: ${_m2_update_submodules}") | ||
| execute_process(COMMAND ${GIT_EXECUTABLE} submodule update --init --recursive -- | ||
| ${_m2_update_submodules} | ||
| WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}" | ||
| RESULT_VARIABLE _m2_submodule_result) | ||
| if(NOT _m2_submodule_result EQUAL 0) | ||
| message(FATAL_ERROR "Could not update required submodules (${_m2_submodule_result})") | ||
| endif() | ||
| endif() | ||
|
|
||
| # Support offline builds and source archives containing the required sources. | ||
| # Retain the editors' existing warning/skip behavior if Emacs sources are absent. | ||
| foreach(_path IN LISTS _m2_required_submodules) | ||
| file(GLOB _m2_submodule_files "${CMAKE_SOURCE_DIR}/${_path}/*") | ||
| # Older non-Git configurations may have left .nogit placeholders in empty | ||
| # submodule directories. Neither those nor .git metadata count as sources. | ||
| list(FILTER _m2_submodule_files EXCLUDE REGEX "/\\.(git|nogit)$") | ||
|
Comment on lines
+89
to
+93
Member
There was a problem hiding this comment. What's this block doing? You said you're not using
Contributor
Author
There was a problem hiding this comment. Older non-Git configurations may have left .nogit placeholders in empty submodule directories. Neither these nor .git metadata count as source files. I can add this as a source comment if you like. |
||
| if(NOT _m2_submodule_files) | ||
| message(FATAL_ERROR "Required submodule ${_path} is missing. Enable GIT_SUBMODULE or run git submodule update --init --recursive -- ${_path} from ${CMAKE_SOURCE_DIR} before configuring.") | ||
| endif() | ||
| endforeach() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these checks necessary? Even if we don't need to clone a submodule, or even if git isn't available, we can safely call ExternalProject_Add here, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, not quite. ExternalProject_Add without a download method requires a nonempty source directory—even if its target won’t be built. Therefore those guards. Cf https://cmake.org/cmake/help/latest/module/ExternalProject.html#download-step-options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why we called
file(TOUCH ${_submodule}/.nogit).Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it cleaner just not have directories if there is no stuff there?
(and as submodules are gone here, calling anything nogit is just wrong)
So this just treating the dependencies in question as normal external dependencies, without any old hacks left)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, not all submodules are gone, I am conflating other PRs here