Copilot commented on code in PR #7718: URL: https://github.com/apache/ignite-3/pull/7718#discussion_r2982888660
########## modules/platforms/cpp/cmake/ignite_compile_headers.cmake: ########## @@ -0,0 +1,83 @@ + # +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# Compile-time check for 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: +# 1. Headers missing their own #include dependencies. +# 2. Public headers that #include internal (non-installed) headers. +# 3. Headers missing from the installed package. +# +# The check installs the already-built client to a temporary prefix inside +# the build tree, then configures a sub-project under +# tests/package-test/compile_public_headers/ against that prefix. +# Because INSTALL_INTERFACE is used, the compiler only sees the installed +# include tree — internal headers absent from the package are not accessible. +# +# Target produced: +# compile-public-headers + +if (NOT ENABLE_CLIENT) + message(STATUS "compile-public-headers: ENABLE_CLIENT=OFF, skipping.") +else() + if (NOT IGNITE3_CLIENT_PUBLIC_HEADERS) + message(WARNING "compile-public-headers: IGNITE3_CLIENT_PUBLIC_HEADERS is empty. " + "Check ignite/client/CMakeLists.txt.") + else() + set(_hcc_dir "${CMAKE_BINARY_DIR}/hcc") + + # Write the list of public headers to a cmake file that the + # sub-project will include. This avoids command-line quoting + # issues when passing a list with semicolons. + set(_hcc_list_file "${_hcc_dir}/headers_list.cmake") + set(_hcc_list_content "set(IGNITE_PUBLIC_HEADERS\n") + foreach(_h IN LISTS IGNITE3_CLIENT_PUBLIC_HEADERS) + string(APPEND _hcc_list_content " \"${_h}\"\n") + endforeach() + string(APPEND _hcc_list_content ")\n") + file(MAKE_DIRECTORY "${_hcc_dir}") + file(WRITE "${_hcc_list_file}" "${_hcc_list_content}") + + set(_hcc_install_prefix "${_hcc_dir}/install") + set(_hcc_sub_src "${CMAKE_SOURCE_DIR}/tests/package-test/compile_public_headers") + set(_hcc_sub_bin "${_hcc_dir}/build") + + add_custom_target(compile-public-headers ALL + # Install the already-built client to a temp prefix. Review Comment: The `compile-public-headers` target is added with `ALL`. Since `add_custom_target` has no outputs, this will run the install + nested configure/build on every normal build, which is likely to slow down local development and CI significantly. Consider removing `ALL` (make it an explicit/CI-only target) or refactor to a `add_custom_command(OUTPUT ...)`-based target so it can be up-to-date when nothing changed. ########## modules/platforms/cpp/cmake/ignite_compile_headers.cmake: ########## @@ -0,0 +1,83 @@ + # +# Licensed to the Apache Software Foundation (ASF) under one or more Review Comment: Minor formatting: the first line appears to have leading indentation before `#`, which is inconsistent with the rest of the license header and makes the file look like it has an accidental whitespace change. Consider removing the leading spaces. ########## modules/platforms/cpp/cmake/ignite_check_headers.cmake: ########## @@ -0,0 +1,67 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# ignite_check_headers(SOURCE_DIR <dir> PUBLIC <list> PRIVATE <list>) +# +# Configure-time check that every .h file found under SOURCE_DIR belongs to +# exactly one of the PUBLIC or PRIVATE header lists. +# +# Fails with FATAL_ERROR when: +# - A header is not listed in either PUBLIC or PRIVATE. +# - A header is listed in both PUBLIC and PRIVATE. +function(ignite_check_headers) + cmake_parse_arguments(ARGS "" "SOURCE_DIR" "PUBLIC;PRIVATE" ${ARGN}) + + if (NOT ARGS_SOURCE_DIR) + message(FATAL_ERROR "ignite_check_headers: SOURCE_DIR is required") + endif() + + file(GLOB_RECURSE _all_headers RELATIVE "${ARGS_SOURCE_DIR}" "${ARGS_SOURCE_DIR}/*.h") Review Comment: `ignite_check_headers` uses `file(GLOB_RECURSE ...)` without `CONFIGURE_DEPENDS`, so adding/removing header files may not trigger a CMake re-configure and the classification check might not run until someone manually reruns CMake. Consider using `file(GLOB_RECURSE ... CONFIGURE_DEPENDS ...)` (or an explicit header list) to ensure the check stays effective when headers change. ```suggestion file(GLOB_RECURSE CONFIGURE_DEPENDS _all_headers RELATIVE "${ARGS_SOURCE_DIR}" "${ARGS_SOURCE_DIR}/*.h") ``` ########## modules/platforms/cpp/cmake/ignite_compile_headers.cmake: ########## @@ -0,0 +1,83 @@ + # +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# Compile-time check for 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: +# 1. Headers missing their own #include dependencies. +# 2. Public headers that #include internal (non-installed) headers. +# 3. Headers missing from the installed package. +# +# The check installs the already-built client to a temporary prefix inside +# the build tree, then configures a sub-project under +# tests/package-test/compile_public_headers/ against that prefix. +# Because INSTALL_INTERFACE is used, the compiler only sees the installed +# include tree — internal headers absent from the package are not accessible. +# +# Target produced: +# compile-public-headers + +if (NOT ENABLE_CLIENT) + message(STATUS "compile-public-headers: ENABLE_CLIENT=OFF, skipping.") +else() + if (NOT IGNITE3_CLIENT_PUBLIC_HEADERS) + message(WARNING "compile-public-headers: IGNITE3_CLIENT_PUBLIC_HEADERS is empty. " + "Check ignite/client/CMakeLists.txt.") + else() + set(_hcc_dir "${CMAKE_BINARY_DIR}/hcc") + + # Write the list of public headers to a cmake file that the + # sub-project will include. This avoids command-line quoting + # issues when passing a list with semicolons. + set(_hcc_list_file "${_hcc_dir}/headers_list.cmake") + set(_hcc_list_content "set(IGNITE_PUBLIC_HEADERS\n") + foreach(_h IN LISTS IGNITE3_CLIENT_PUBLIC_HEADERS) + string(APPEND _hcc_list_content " \"${_h}\"\n") + endforeach() + string(APPEND _hcc_list_content ")\n") + file(MAKE_DIRECTORY "${_hcc_dir}") + file(WRITE "${_hcc_list_file}" "${_hcc_list_content}") + + set(_hcc_install_prefix "${_hcc_dir}/install") + set(_hcc_sub_src "${CMAKE_SOURCE_DIR}/tests/package-test/compile_public_headers") + set(_hcc_sub_bin "${_hcc_dir}/build") + + add_custom_target(compile-public-headers ALL + # Install the already-built client to a temp prefix. + # Only files declared with COMPONENT client are installed — + # internal headers not in PUBLIC_HEADERS are absent. + COMMAND ${CMAKE_COMMAND} --install "${CMAKE_BINARY_DIR}" + --prefix "${_hcc_install_prefix}" + --component client + # Configure sub-project against the installed package. + # INTERFACE_INCLUDE_DIRECTORIES resolves to <prefix>/include + # (INSTALL_INTERFACE), so the compiler cannot reach internal headers. + COMMAND ${CMAKE_COMMAND} + "-DCMAKE_PREFIX_PATH=${_hcc_install_prefix}" + "-DIGNITE_HEADERS_LIST_FILE=${_hcc_list_file}" + "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}" + "-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}" + "-S${_hcc_sub_src}" + "-B${_hcc_sub_bin}" + COMMAND ${CMAKE_COMMAND} --build "${_hcc_sub_bin}" Review Comment: For multi-config generators (Visual Studio/Xcode), `cmake --install` typically needs `--config <Debug|Release...>` and `CMAKE_BUILD_TYPE` is usually empty/ignored. As written, the install step and the nested `cmake --build` will likely default to Debug (or fail to find built artifacts) even when the parent build is using another configuration. Consider forwarding the active configuration (`--config` / `CMAKE_CFG_INTDIR`) and avoiding reliance on `CMAKE_BUILD_TYPE` for multi-config builds. ########## modules/platforms/cpp/tests/package-test/compile_public_headers/CMakeLists.txt: ########## @@ -0,0 +1,61 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# Header self-containment test. +# +# For every public header of ignite3-client, compiles a minimal .cpp that +# includes ONLY that header (via the installed/exported ignite::client target). +# This verifies that each header includes all its own dependencies and can be +# used by consumers without relying on a particular include order. +# +# Invoked by the parent build's 'header-self-containment-check' target: +# cmake -S <this_dir> -B <build_dir> +# -Dignite_DIR=<ignite_build>/cmake +# -DIGNITE_HEADERS_LIST_FILE=<path/to/headers_list.cmake> +# -DCMAKE_CXX_COMPILER=<compiler> +# [-DCMAKE_BUILD_TYPE=<type>] +# cmake --build <build_dir> Review Comment: The comment block mentions a parent target named `header-self-containment-check` and shows an example using `-Dignite_DIR=...`, but the implementation in this PR creates/uses `compile-public-headers` and configures the subproject via `CMAKE_PREFIX_PATH`. Please update the comment so the documented invocation matches the actual target/arguments. ########## modules/platforms/cpp/cmake/ignite_compile_headers.cmake: ########## @@ -0,0 +1,83 @@ + # +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# Compile-time check for 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: +# 1. Headers missing their own #include dependencies. +# 2. Public headers that #include internal (non-installed) headers. +# 3. Headers missing from the installed package. +# +# The check installs the already-built client to a temporary prefix inside +# the build tree, then configures a sub-project under +# tests/package-test/compile_public_headers/ against that prefix. +# Because INSTALL_INTERFACE is used, the compiler only sees the installed +# include tree — internal headers absent from the package are not accessible. +# +# Target produced: +# compile-public-headers + +if (NOT ENABLE_CLIENT) + message(STATUS "compile-public-headers: ENABLE_CLIENT=OFF, skipping.") +else() + if (NOT IGNITE3_CLIENT_PUBLIC_HEADERS) + message(WARNING "compile-public-headers: IGNITE3_CLIENT_PUBLIC_HEADERS is empty. " + "Check ignite/client/CMakeLists.txt.") + else() + set(_hcc_dir "${CMAKE_BINARY_DIR}/hcc") + + # Write the list of public headers to a cmake file that the + # sub-project will include. This avoids command-line quoting + # issues when passing a list with semicolons. + set(_hcc_list_file "${_hcc_dir}/headers_list.cmake") + set(_hcc_list_content "set(IGNITE_PUBLIC_HEADERS\n") + foreach(_h IN LISTS IGNITE3_CLIENT_PUBLIC_HEADERS) + string(APPEND _hcc_list_content " \"${_h}\"\n") + endforeach() + string(APPEND _hcc_list_content ")\n") + file(MAKE_DIRECTORY "${_hcc_dir}") + file(WRITE "${_hcc_list_file}" "${_hcc_list_content}") + + set(_hcc_install_prefix "${_hcc_dir}/install") + set(_hcc_sub_src "${CMAKE_SOURCE_DIR}/tests/package-test/compile_public_headers") + set(_hcc_sub_bin "${_hcc_dir}/build") + + add_custom_target(compile-public-headers ALL + # Install the already-built client to a temp prefix. + # Only files declared with COMPONENT client are installed — + # internal headers not in PUBLIC_HEADERS are absent. + COMMAND ${CMAKE_COMMAND} --install "${CMAKE_BINARY_DIR}" + --prefix "${_hcc_install_prefix}" + --component client + # Configure sub-project against the installed package. + # INTERFACE_INCLUDE_DIRECTORIES resolves to <prefix>/include + # (INSTALL_INTERFACE), so the compiler cannot reach internal headers. + COMMAND ${CMAKE_COMMAND} + "-DCMAKE_PREFIX_PATH=${_hcc_install_prefix}" + "-DIGNITE_HEADERS_LIST_FILE=${_hcc_list_file}" + "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}" + "-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}" + "-S${_hcc_sub_src}" + "-B${_hcc_sub_bin}" + COMMAND ${CMAKE_COMMAND} --build "${_hcc_sub_bin}" Review Comment: The nested CMake configure call does not pass the parent generator/toolset/platform (e.g. `-G`, `-A`, `-T`). If the main build is using Ninja or Visual Studio, the subproject may silently pick a different default generator and fail to build or use different flags. Consider forwarding `${CMAKE_GENERATOR}` (and platform/toolset variables when defined), and also any toolchain-related variables used by the parent build. ########## modules/platforms/cpp/cmake/ignite_compile_headers.cmake: ########## @@ -0,0 +1,83 @@ + # +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# Compile-time check for 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: +# 1. Headers missing their own #include dependencies. +# 2. Public headers that #include internal (non-installed) headers. +# 3. Headers missing from the installed package. +# +# The check installs the already-built client to a temporary prefix inside +# the build tree, then configures a sub-project under +# tests/package-test/compile_public_headers/ against that prefix. +# Because INSTALL_INTERFACE is used, the compiler only sees the installed +# include tree — internal headers absent from the package are not accessible. +# +# Target produced: +# compile-public-headers + +if (NOT ENABLE_CLIENT) + message(STATUS "compile-public-headers: ENABLE_CLIENT=OFF, skipping.") +else() + if (NOT IGNITE3_CLIENT_PUBLIC_HEADERS) + message(WARNING "compile-public-headers: IGNITE3_CLIENT_PUBLIC_HEADERS is empty. " + "Check ignite/client/CMakeLists.txt.") + else() + set(_hcc_dir "${CMAKE_BINARY_DIR}/hcc") + + # Write the list of public headers to a cmake file that the + # sub-project will include. This avoids command-line quoting + # issues when passing a list with semicolons. + set(_hcc_list_file "${_hcc_dir}/headers_list.cmake") + set(_hcc_list_content "set(IGNITE_PUBLIC_HEADERS\n") + foreach(_h IN LISTS IGNITE3_CLIENT_PUBLIC_HEADERS) + string(APPEND _hcc_list_content " \"${_h}\"\n") + endforeach() + string(APPEND _hcc_list_content ")\n") + file(MAKE_DIRECTORY "${_hcc_dir}") + file(WRITE "${_hcc_list_file}" "${_hcc_list_content}") + + set(_hcc_install_prefix "${_hcc_dir}/install") + set(_hcc_sub_src "${CMAKE_SOURCE_DIR}/tests/package-test/compile_public_headers") + set(_hcc_sub_bin "${_hcc_dir}/build") + + add_custom_target(compile-public-headers ALL + # Install the already-built client to a temp prefix. + # Only files declared with COMPONENT client are installed — + # internal headers not in PUBLIC_HEADERS are absent. + COMMAND ${CMAKE_COMMAND} --install "${CMAKE_BINARY_DIR}" + --prefix "${_hcc_install_prefix}" + --component client + # Configure sub-project against the installed package. + # INTERFACE_INCLUDE_DIRECTORIES resolves to <prefix>/include + # (INSTALL_INTERFACE), so the compiler cannot reach internal headers. + COMMAND ${CMAKE_COMMAND} + "-DCMAKE_PREFIX_PATH=${_hcc_install_prefix}" + "-DIGNITE_HEADERS_LIST_FILE=${_hcc_list_file}" + "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}" + "-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}" + "-S${_hcc_sub_src}" + "-B${_hcc_sub_bin}" + COMMAND ${CMAKE_COMMAND} --build "${_hcc_sub_bin}" + DEPENDS ignite3-client + COMMENT "compile-public-headers: compiling each public header against installed package" + VERBATIM + ) Review Comment: This header compilation check relies on headers being present in the installed prefix, but installing headers is currently gated by `INSTALL_IGNITE_FILES` in several submodules. If someone configures with `-DINSTALL_IGNITE_FILES=OFF`, this target will likely fail during the sub-build. Consider skipping the check (with a status message) unless `INSTALL_IGNITE_FILES` is ON, or ensure the required headers are always installed for `--component client`. ```suggestion if (NOT INSTALL_IGNITE_FILES) message(STATUS "compile-public-headers: INSTALL_IGNITE_FILES=OFF, skipping header compile check.") else() if (NOT IGNITE3_CLIENT_PUBLIC_HEADERS) message(WARNING "compile-public-headers: IGNITE3_CLIENT_PUBLIC_HEADERS is empty. " "Check ignite/client/CMakeLists.txt.") else() set(_hcc_dir "${CMAKE_BINARY_DIR}/hcc") # Write the list of public headers to a cmake file that the # sub-project will include. This avoids command-line quoting # issues when passing a list with semicolons. set(_hcc_list_file "${_hcc_dir}/headers_list.cmake") set(_hcc_list_content "set(IGNITE_PUBLIC_HEADERS\n") foreach(_h IN LISTS IGNITE3_CLIENT_PUBLIC_HEADERS) string(APPEND _hcc_list_content " \"${_h}\"\n") endforeach() string(APPEND _hcc_list_content ")\n") file(MAKE_DIRECTORY "${_hcc_dir}") file(WRITE "${_hcc_list_file}" "${_hcc_list_content}") set(_hcc_install_prefix "${_hcc_dir}/install") set(_hcc_sub_src "${CMAKE_SOURCE_DIR}/tests/package-test/compile_public_headers") set(_hcc_sub_bin "${_hcc_dir}/build") add_custom_target(compile-public-headers ALL # Install the already-built client to a temp prefix. # Only files declared with COMPONENT client are installed — # internal headers not in PUBLIC_HEADERS are absent. COMMAND ${CMAKE_COMMAND} --install "${CMAKE_BINARY_DIR}" --prefix "${_hcc_install_prefix}" --component client # Configure sub-project against the installed package. # INTERFACE_INCLUDE_DIRECTORIES resolves to <prefix>/include # (INSTALL_INTERFACE), so the compiler cannot reach internal headers. COMMAND ${CMAKE_COMMAND} "-DCMAKE_PREFIX_PATH=${_hcc_install_prefix}" "-DIGNITE_HEADERS_LIST_FILE=${_hcc_list_file}" "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}" "-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}" "-S${_hcc_sub_src}" "-B${_hcc_sub_bin}" COMMAND ${CMAKE_COMMAND} --build "${_hcc_sub_bin}" DEPENDS ignite3-client COMMENT "compile-public-headers: compiling each public header against installed package" VERBATIM ) endif() ``` -- 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]
