Copilot commented on code in PR #7929:
URL: https://github.com/apache/ignite-3/pull/7929#discussion_r3033293047
##########
modules/platforms/cpp/cmake/ignite_compile_headers.cmake:
##########
@@ -15,10 +15,11 @@
# limitations under the License.
#
-# Compile-time check for public headers (always enabled when ENABLE_CLIENT=ON).
+# Compile-time check for all public headers (always enabled when
ENABLE_CLIENT=ON).
#
-# For every public header of ignite3-client, compiles a minimal .cpp that
-# includes ONLY that header against the INSTALLED package. This catches:
+# For every public header registered via ignite_collect_public_headers(),
compiles
+# a minimal .cpp that includes ONLY that header against the INSTALLED package.
+# This catches:
Review Comment:
Now that the check compiles *all* collected public headers (not just those
transitively used by ignite3-client), the stamp-based caching should be
invalidated when any of those header files change. Currently the custom command
only depends on the ignite3-client target, so edits to a header that isn't
included by any compiled TU may not trigger a rerun, leaving regressions
undetected. Consider adding the public header source files as explicit DEPENDS
of the custom command (e.g., prefix ${CMAKE_SOURCE_DIR}/ to each entry in
IGNITE3_ALL_PUBLIC_HEADERS), or make the target always run in CI builds.
##########
modules/platforms/cpp/tests/package-test/compile_public_headers/CMakeLists.txt:
##########
@@ -49,13 +49,18 @@ find_package(ignite REQUIRED COMPONENTS client)
include("${IGNITE_HEADERS_LIST_FILE}")
foreach(H IN LISTS IGNITE_PUBLIC_HEADERS)
- # Derive a CMake-identifier-safe target name from the header path.
- string(MAKE_C_IDENTIFIER "${H}" CPH_SAFE_NAME)
+ # Use a short MD5 hash of the header path as the target/file name.
+ # A full MAKE_C_IDENTIFIER name can exceed 260-char Windows path limits
+ # when combined with MSBuild's .tlog directory structure (VS 2017 does not
+ # honour the LongPathsEnabled registry key).
+ string(MD5 CPH_HASH "${H}")
+ string(SUBSTRING "${CPH_HASH}" 0 8 CPH_SHORT)
+ message(STATUS "HEADER MAP: ${H} -> ${CPH_HASH}")
Review Comment:
The per-header STATUS log line ("HEADER MAP: ...") will produce very noisy
configure output when compiling many headers and can slow CI log processing.
Consider removing it or guarding it behind an option (e.g., only print when a
CPH_VERBOSE flag is enabled).
##########
modules/platforms/cpp/tests/package-test/compile_public_headers/CMakeLists.txt:
##########
@@ -49,13 +49,18 @@ find_package(ignite REQUIRED COMPONENTS client)
include("${IGNITE_HEADERS_LIST_FILE}")
foreach(H IN LISTS IGNITE_PUBLIC_HEADERS)
- # Derive a CMake-identifier-safe target name from the header path.
- string(MAKE_C_IDENTIFIER "${H}" CPH_SAFE_NAME)
+ # Use a short MD5 hash of the header path as the target/file name.
+ # A full MAKE_C_IDENTIFIER name can exceed 260-char Windows path limits
+ # when combined with MSBuild's .tlog directory structure (VS 2017 does not
+ # honour the LongPathsEnabled registry key).
+ string(MD5 CPH_HASH "${H}")
+ string(SUBSTRING "${CPH_HASH}" 0 8 CPH_SHORT)
+ message(STATUS "HEADER MAP: ${H} -> ${CPH_HASH}")
- set(CPH_CPP "${CMAKE_CURRENT_BINARY_DIR}/${CPH_SAFE_NAME}.cpp")
- file(WRITE "${CPH_CPP}" "#include <ignite/client/${H}>\n")
+ set(CPH_CPP "${CMAKE_CURRENT_BINARY_DIR}/cph_${CPH_SHORT}.cpp")
Review Comment:
Using only the first 8 hex chars of the MD5 as the target/file identifier
risks collisions (two different headers could map to the same CPH_SHORT). A
collision would overwrite the generated .cpp and/or create duplicate CMake
targets (configure error or silently missing coverage). Consider increasing the
substring length (e.g., 12–16) and/or adding collision detection (e.g., keep a
set/map and fall back to a longer hash when a duplicate is found).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
