Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts CMake/Version.cmake so a consuming (super)project can set VERSION_OUT_DIR without it being overwritten when the module is included as a submodule, and extends the generated version header with build timestamps.
Changes:
- Avoid overwriting a pre-set
VERSION_OUT_DIRby conditionally initializing the cache variable. - Add
VERSION_DATE/VERSION_DATETIMEvariables and emit them into the default generatedVersion.h.in. - Ignore
/build/in.gitignore.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| CMake/Version.cmake | Preserve caller-provided output dir; add date/datetime variables; refactors formatting in several CMake blocks. |
| .gitignore | Adds /build/ to ignored paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| list(APPEND CMAKE_MESSAGE_INDENT " ") | ||
|
|
||
| set(VERSION_OUT_DIR "${CMAKE_BINARY_DIR}" CACHE PATH "Destination directory into which `Version.cmake` shall genrate Versioning header files") | ||
| if(NOT VERSION_OUT_DIR) |
There was a problem hiding this comment.
The guard if(NOT VERSION_OUT_DIR) checks truthiness, so values like ...-NOTFOUND/OFF/0 will be treated as false and silently replaced. Since this is a PATH option, it’s safer to check whether it is defined (and optionally whether it’s an empty string) rather than boolean-evaluating the path value.
| if(NOT VERSION_OUT_DIR) | |
| if(NOT DEFINED VERSION_OUT_DIR OR "${VERSION_OUT_DIR}" STREQUAL "") |
|
|
||
| set(VERSION_OUT_DIR "${CMAKE_BINARY_DIR}" CACHE PATH "Destination directory into which `Version.cmake` shall genrate Versioning header files") | ||
| if(NOT VERSION_OUT_DIR) | ||
| set(VERSION_OUT_DIR "${CMAKE_BINARY_DIR}" CACHE PATH "Destination directory into which `Version.cmake` shall genrate Versioning header files") |
There was a problem hiding this comment.
Spelling in the cache description: "shall genrate" should be "shall generate".
| set(VERSION_OUT_DIR "${CMAKE_BINARY_DIR}" CACHE PATH "Destination directory into which `Version.cmake` shall genrate Versioning header files") | |
| set(VERSION_OUT_DIR "${CMAKE_BINARY_DIR}" CACHE PATH "Destination directory into which `Version.cmake` shall generate Versioning header files") |
|
|
||
| if(${_VERSION_SET}) | ||
| message(CHECK_PASS "Tag '${git_describe}' is a valid semantic version [${_VERSION_SEMANTIC}]") | ||
| message(STATUS "Git Datetime: ${VERSION_DATETIME}") |
There was a problem hiding this comment.
message(STATUS "Git Datetime: ${VERSION_DATETIME}") is misleading: VERSION_DATETIME is the current configure-time timestamp (from string(TIMESTAMP ...)), not a Git-derived datetime. Consider renaming the label (e.g., "Configure datetime"/"Build datetime") or extracting the commit date if you intend it to be Git time.
| message(STATUS "Git Datetime: ${VERSION_DATETIME}") | |
| message(STATUS "Configure datetime: ${VERSION_DATETIME}") |
| gitversion_configure_file( ${VERSION_H_TEMPLATE} ${VERSION_H}) | ||
| else() | ||
| if(VERSION_GENERATE_NOW) | ||
| gitversion_configure_file(${VERSION_H_TEMPLATE} ${VERSION_H}) |
There was a problem hiding this comment.
gitversion_configure_file(${VERSION_H_TEMPLATE} ${VERSION_H}) passes unquoted paths; if either path contains spaces/semicolons, argument splitting will break and configure_file() won’t receive the intended filenames. Quote both arguments at the call site.
| gitversion_configure_file(${VERSION_H_TEMPLATE} ${VERSION_H}) | |
| gitversion_configure_file("${VERSION_H_TEMPLATE}" "${VERSION_H}") |
| if(NOT EXISTS ${VERSION_H_TEMPLATE}) | ||
| set(VERSION_H_TEMPLATE "${VERSION_OUT_DIR}/${VERSION_H_FILENAME}.in") | ||
| message( CHECK_FAIL "Not Found. Generating '${VERSION_H_TEMPLATE}'") | ||
| message(CHECK_FAIL "Not Found. Generating '${VERSION_H_TEMPLATE}'") | ||
|
|
||
| file(WRITE ${VERSION_H_TEMPLATE} | ||
| [=[ | ||
| [=[ |
There was a problem hiding this comment.
if(NOT EXISTS ${VERSION_H_TEMPLATE}) / file(WRITE ${VERSION_H_TEMPLATE} ...) use unquoted paths. If the module lives under a directory with spaces (common on Windows), this will mis-parse and can error. Quote ${VERSION_H_TEMPLATE} (and other file paths) anywhere they’re used as single arguments to CMake commands/conditions.
| -D VERSION_GENERATE_NOW=YES | ||
| -D VERSION_H_TEMPLATE=${VERSION_H_TEMPLATE} | ||
| -D VERSION_H=${VERSION_H} | ||
| -D VERSION_PREFIX=${VERSION_PREFIX} | ||
| -D GIT_EXECUTABLE=${GIT_EXECUTABLE} | ||
| -D CMAKE_MODULE_PATH=${CMAKE_MODULE_PATH} | ||
| -P ${CMAKE_CURRENT_LIST_FILE} | ||
| WORKING_DIRECTORY ${VERSION_SOURCE_DIR} |
There was a problem hiding this comment.
In the custom target invocation, -D CMAKE_MODULE_PATH=${CMAKE_MODULE_PATH} (and other -D ...=${...} args) are unquoted. If CMAKE_MODULE_PATH contains multiple entries (semicolon-separated) or any value contains spaces, CMake will split the argument list and the script will receive truncated/incorrect -D values. Pass these -D values as a single argument (quote/escape), or serialize list variables explicitly before passing them.
| -D VERSION_GENERATE_NOW=YES | |
| -D VERSION_H_TEMPLATE=${VERSION_H_TEMPLATE} | |
| -D VERSION_H=${VERSION_H} | |
| -D VERSION_PREFIX=${VERSION_PREFIX} | |
| -D GIT_EXECUTABLE=${GIT_EXECUTABLE} | |
| -D CMAKE_MODULE_PATH=${CMAKE_MODULE_PATH} | |
| -P ${CMAKE_CURRENT_LIST_FILE} | |
| WORKING_DIRECTORY ${VERSION_SOURCE_DIR} | |
| "-DVERSION_GENERATE_NOW=YES" | |
| "-DVERSION_H_TEMPLATE=${VERSION_H_TEMPLATE}" | |
| "-DVERSION_H=${VERSION_H}" | |
| "-DVERSION_PREFIX=${VERSION_PREFIX}" | |
| "-DGIT_EXECUTABLE=${GIT_EXECUTABLE}" | |
| "-DCMAKE_MODULE_PATH=${CMAKE_MODULE_PATH}" | |
| -P "${CMAKE_CURRENT_LIST_FILE}" | |
| WORKING_DIRECTORY "${VERSION_SOURCE_DIR}" |
|
Thanks for the PR, I like it - I need to take some time and make sure it works for all/existing use-cases. Its a time I need to look at setting up some test harness around to ensure it doesn't break other cases. In meantime I think this could be landed with some tweaks, just need to go through the Copilot checks, I suspect some of them are from existing behaviour on a quick scan but some look actionable - but AI does make mistakes! Thanks again. |
When
Version.cmakeis as a submodule of aCMakeproject,VERSION_OUT_DIRset in the project not work. This PR will check ifVERSION_OUT_DIRset in the project before set the cached variable inVersion.cmake.