Copilot commented on code in PR #7300: URL: https://github.com/apache/ignite-3/pull/7300#discussion_r2684099600
########## modules/platforms/cpp/cmake/ignite-config.cmake.in: ########## @@ -0,0 +1,70 @@ +# +# 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. +# + +@PACKAGE_INIT@ + +set(ignite_VERSION @PROJECT_VERSION@) + +cmake_policy(SET CMP0057 NEW) + +# Compute installation paths +get_filename_component(ignite_CMAKE_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH) +set(ignite_INCLUDE_DIR "${ignite_CMAKE_DIR}/..") Review Comment: The ignite_INCLUDE_DIR is computed incorrectly using a relative path "../" from the cmake directory. The config file will be installed to CMAKE_INSTALL_LIBDIR/cmake (e.g., lib/cmake), so the include directory would be at "lib/cmake/.." = "lib", not the actual include directory. This should use the PACKAGE_INIT macro's computed paths or be set to a proper absolute path using CMAKE_INSTALL_INCLUDEDIR. ```suggestion # Use the package prefix and configured include directory for a correct, absolute include path set(ignite_INCLUDE_DIR "${PACKAGE_PREFIX_DIR}/@CMAKE_INSTALL_INCLUDEDIR@") ``` ########## modules/platforms/cpp/CMakeLists.txt: ########## @@ -191,3 +195,34 @@ if (CLANG_FORMAT_BIN) else() message(STATUS "Failed to find clang-format. So there is no 'format' target now.") endif() + +if (${INSTALL_IGNITE_FILES}) + if (${ENABLE_CLIENT}) + list(APPEND IGNITE_AVAILABLE_COMPONENTS client) + endif() + if (${ENABLE_ODBC}) + list(APPEND IGNITE_AVAILABLE_COMPONENTS odbc) + endif() + + include(CMakePackageConfigHelpers) + configure_package_config_file( + ${IGNITE_CMAKE_TOP_DIR}/cmake/ignite-config.cmake.in + ${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config.cmake + INSTALL_DESTINATION ${CMAKE_INSTALL_PREFIX} + PATH_VARS IGNITE_INCLUDEDIR + ) + + add_subdirectory(pkgconfig) + + set(IGNITE_INSTALL_CMAKEDIR ${CMAKE_INSTALL_LIBDIR}/cmake) + Review Comment: The INSTALL_DESTINATION parameter should be set to the actual installation directory of the config file (CMAKE_INSTALL_LIBDIR/cmake/ignite), not CMAKE_INSTALL_PREFIX. This affects how @PACKAGE_INIT@ computes relative paths. The correct value should be "${CMAKE_INSTALL_LIBDIR}/cmake/ignite" or use the IGNITE_INSTALL_CMAKEDIR variable defined at line 217 (but with /ignite appended). ```suggestion set(IGNITE_INSTALL_CMAKEDIR ${CMAKE_INSTALL_LIBDIR}/cmake/ignite) include(CMakePackageConfigHelpers) configure_package_config_file( ${IGNITE_CMAKE_TOP_DIR}/cmake/ignite-config.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config.cmake INSTALL_DESTINATION ${IGNITE_INSTALL_CMAKEDIR} PATH_VARS IGNITE_INCLUDEDIR ) add_subdirectory(pkgconfig) ``` ########## modules/platforms/cpp/tests/package-test/cmake_package/CMakeLists.txt: ########## @@ -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. +# + +cmake_minimum_required(VERSION 3.18) + +project(ignite_package_test) + +set(IGNITE_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../../../") +set(IGNITE_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/cmake_package") + +execute_process( + COMMAND "${CMAKE_COMMAND}" + "-H${IGNITE_SOURCE_DIR}" + "-B${IGNITE_BINARY_DIR}" + "-DENABLE_ODBC=${ENABLE_ODBC}" + "-DENABLE_CLIENT=${ENABLE_CLIENT}" +) + +execute_process( + COMMAND "${CMAKE_COMMAND}" + --build "${IGNITE_BINARY_DIR}" +) + Review Comment: The execute_process commands don't check for errors (missing RESULT_VARIABLE or ERROR_VARIABLE). If the CMake configure or build fails, the test will continue and produce confusing error messages later. Add RESULT_VARIABLE and check the result, or use COMMAND_ERROR_IS_FATAL ANY (available in CMake 3.19+) to automatically fail on errors. ```suggestion "-DENABLE_CLIENT=${ENABLE_CLIENT}" RESULT_VARIABLE CONFIGURE_RESULT ERROR_VARIABLE CONFIGURE_ERROR ) if(NOT CONFIGURE_RESULT EQUAL 0) message(FATAL_ERROR "CMake configure step failed: ${CONFIGURE_ERROR}") endif() execute_process( COMMAND "${CMAKE_COMMAND}" --build "${IGNITE_BINARY_DIR}" RESULT_VARIABLE BUILD_RESULT ERROR_VARIABLE BUILD_ERROR RESULT_VARIABLE BUILD_RESULT ERROR_VARIABLE BUILD_ERROR ) if(NOT BUILD_RESULT EQUAL 0) message(FATAL_ERROR "CMake build step failed: ${BUILD_ERROR}") endif() ``` ########## modules/platforms/cpp/ignite/client/CMakeLists.txt: ########## @@ -113,16 +115,35 @@ foreach(_target_lib IN LISTS _target_libs) set_target_properties(${_target_lib} PROPERTIES OUTPUT_NAME ${PROJECT_NAME}) endif() - target_link_libraries(${_target_lib} ${LIBRARIES}) + target_link_libraries(${_target_lib} + PRIVATE ignite-tuple ignite-network ignite-protocol uni-algo::uni-algo + INTERFACE ignite-common) endforeach() unset(_target_libs) +add_library(ignite::client ALIAS ${TARGET}) +add_library(ignite::client-static ALIAS ${TARGET}-static) + if (${INSTALL_IGNITE_FILES}) - install(TARGETS ${TARGET} - RUNTIME DESTINATION bin - ARCHIVE DESTINATION lib - LIBRARY DESTINATION lib + get_target_property(IGNITE_CLIENT_INCLUDES ${TARGET} INCLUDE_DIRECTORIES) + install(TARGETS ${TARGET} ignite-common tfpsacrypto + EXPORT ignite-client-targets + RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} + ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} + LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} + INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} + ) Review Comment: The install command includes ignite-common and tfpsacrypto targets, but these targets don't appear to have their own install commands defined. ignite-common is a static library that should either be installed separately with its own EXPORT, or the ignite-client library should be statically linked with ignite-common. Including them here without proper export setup will cause CMake errors during installation. ########## modules/platforms/cpp/tests/package-test/README.md: ########## @@ -0,0 +1,42 @@ +# Ignite Package Test + +This directory contains tests to verify that the installed Ignite package can be found and used correctly via CMake's `find_package()` mechanism. + +## Purpose + +These tests verify: +1. The `ignite-config.cmake` package configuration works correctly +2. Component-based discovery (client, odbc) functions properly +3. The exported CMake targets link correctly +4. Headers and libraries are accessible + +## Building the Tests + +Each test cmake can build two tests: `test_client` and `test_odbc`, there are corresponding cmake options: + * `ENABLE_CLIENT` + * `ENABLE_ODBC` +These options could be used together. Each cmake configuration should build two tests (if `ENABLE_CLIENT=ON` and `ENABLE_ODBC=ON` specified). +Successful compilation of these tests means we have found Ignite components, include directories and libraries to link with. + +### `cmake_package` + +This test implies that there is Ignite source code folder and Ignite was built at least once, but was not installed. +This test uses `ignite-config.cmake` which will be generated in the binary directory. + +Build command: +`cmake -DENABLE_CLIENT=ON -DENABLE_ODBC=ON -S ../tests/package-test/cmake_package/ -B build_package && cmake --build build_package` + +### `cmake_package_install` + +This test will search for the Ignite installation. For test purposes install will be done in the directory inside build directory. +Test will search `ignite-config.cmake` in the installation directory. Review Comment: Grammar issue: "Test will search" should be "The test will search". ```suggestion The test will search for `ignite-config.cmake` in the installation directory. ``` ########## modules/platforms/cpp/tests/package-test/cmake_package_install/CMakeLists.txt: ########## @@ -0,0 +1,73 @@ +# +# 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. +# + +cmake_minimum_required(VERSION 3.18) + +project(ignite_package_install_test) + +set(IGNITE_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../../../") +set(IGNITE_INSTALL_DIR "${CMAKE_CURRENT_BINARY_DIR}/cmake_package_install") +set(IGNITE_BINARY_DIR "${IGNITE_INSTALL_DIR}${CMAKE_FILES_DIRECTORY}") + +execute_process( + COMMAND "${CMAKE_COMMAND}" + "-H${IGNITE_SOURCE_DIR}" + "-B${IGNITE_BINARY_DIR}" + "-DENABLE_ODBC=${ENABLE_ODBC}" + "-DENABLE_CLIENT=${ENABLE_CLIENT}" + "-DENABLE_TESTS=OFF" + "-DCMAKE_INSTALL_PREFIX=${IGNITE_INSTALL_DIR}" +) + +execute_process( + COMMAND "${CMAKE_COMMAND}" + --build "${IGNITE_BINARY_DIR}" + --target install +) + Review Comment: Same issue: the execute_process commands don't check for errors. Add error handling to fail fast if the CMake configure, build, or install steps fail. ```suggestion "-DCMAKE_INSTALL_PREFIX=${IGNITE_INSTALL_DIR}" RESULT_VARIABLE IGNITE_CONFIGURE_RESULT ) if(NOT IGNITE_CONFIGURE_RESULT EQUAL 0) message(FATAL_ERROR "CMake configure step failed with exit code: ${IGNITE_CONFIGURE_RESULT}") endif() execute_process( COMMAND "${CMAKE_COMMAND}" --build "${IGNITE_BINARY_DIR}" --target install RESULT_VARIABLE IGNITE_BUILD_RESULT ) if(NOT IGNITE_BUILD_RESULT EQUAL 0) message(FATAL_ERROR "CMake build/install step failed with exit code: ${IGNITE_BUILD_RESULT}") endif() ``` ########## modules/platforms/cpp/tests/package-test/README.md: ########## @@ -0,0 +1,42 @@ +# Ignite Package Test + +This directory contains tests to verify that the installed Ignite package can be found and used correctly via CMake's `find_package()` mechanism. + +## Purpose + +These tests verify: +1. The `ignite-config.cmake` package configuration works correctly +2. Component-based discovery (client, odbc) functions properly +3. The exported CMake targets link correctly +4. Headers and libraries are accessible + +## Building the Tests + +Each test cmake can build two tests: `test_client` and `test_odbc`, there are corresponding cmake options: + * `ENABLE_CLIENT` + * `ENABLE_ODBC` +These options could be used together. Each cmake configuration should build two tests (if `ENABLE_CLIENT=ON` and `ENABLE_ODBC=ON` specified). Review Comment: Grammar issue: "These options could be used together" should be "These options can be used together". ```suggestion These options can be used together. Each cmake configuration should build two tests (if `ENABLE_CLIENT=ON` and `ENABLE_ODBC=ON` specified). ``` ########## modules/platforms/cpp/CMakeLists.txt: ########## @@ -191,3 +195,34 @@ if (CLANG_FORMAT_BIN) else() message(STATUS "Failed to find clang-format. So there is no 'format' target now.") endif() + +if (${INSTALL_IGNITE_FILES}) + if (${ENABLE_CLIENT}) + list(APPEND IGNITE_AVAILABLE_COMPONENTS client) + endif() + if (${ENABLE_ODBC}) + list(APPEND IGNITE_AVAILABLE_COMPONENTS odbc) + endif() + + include(CMakePackageConfigHelpers) + configure_package_config_file( + ${IGNITE_CMAKE_TOP_DIR}/cmake/ignite-config.cmake.in + ${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config.cmake + INSTALL_DESTINATION ${CMAKE_INSTALL_PREFIX} + PATH_VARS IGNITE_INCLUDEDIR + ) + + add_subdirectory(pkgconfig) + + set(IGNITE_INSTALL_CMAKEDIR ${CMAKE_INSTALL_LIBDIR}/cmake) + + write_basic_package_version_file(${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config-version.cmake + VERSION ${PROJECT_VERSION} + COMPATIBILITY ExactVersion) + + install(FILES + ${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config.cmake + ${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config-version.cmake + DESTINATION ${IGNITE_INSTALL_CMAKEDIR} Review Comment: The config files are being installed to CMAKE_INSTALL_LIBDIR/cmake (without the /ignite subdirectory), but the component target files (ignite-client-targets.cmake, ignite-odbc-targets.cmake) are installed to CMAKE_INSTALL_LIBDIR/cmake/ignite (see ignite/client/CMakeLists.txt line 145 and ignite/odbc/CMakeLists.txt line 120). This mismatch will cause find_package to fail to locate the target files. The config files should be installed to the same directory: "${CMAKE_INSTALL_LIBDIR}/cmake/ignite". ########## modules/platforms/cpp/ignite/common/CMakeLists.txt: ########## @@ -57,9 +57,9 @@ add_library(${TARGET} STATIC ${SOURCES}) target_link_libraries(${TARGET} PUBLIC TF-PSA-Crypto::tfpsacrypto) target_include_directories(${TARGET} INTERFACE - $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}> + $<BUILD_INTERFACE:${IGNITE_CMAKE_TOP_DIR}> $<INSTALL_INTERFACE:${IGNITE_INCLUDEDIR}> - PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) + PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}>) Review Comment: The PUBLIC include directory has been changed from CMAKE_CURRENT_SOURCE_DIR to a generator expression with BUILD_INTERFACE. This is correct for exported targets, as it ensures the include directory is only used during build time. However, removing the unconditional PUBLIC include may break internal targets that depend on this library if they're not using the same build interface path. Verify that all internal consumers can still find the headers. ########## modules/platforms/cpp/ignite/client/CMakeLists.txt: ########## @@ -113,16 +115,35 @@ foreach(_target_lib IN LISTS _target_libs) set_target_properties(${_target_lib} PROPERTIES OUTPUT_NAME ${PROJECT_NAME}) endif() - target_link_libraries(${_target_lib} ${LIBRARIES}) + target_link_libraries(${_target_lib} + PRIVATE ignite-tuple ignite-network ignite-protocol uni-algo::uni-algo + INTERFACE ignite-common) Review Comment: The target_link_libraries has been changed to use PRIVATE for implementation dependencies and INTERFACE for ignite-common. However, this creates an inconsistency: ignite-common is linked as INTERFACE (meaning consumers will link against it), but ignite-tuple, ignite-network, and ignite-protocol are marked PRIVATE. If ignite-common headers are exposed in the public API of ignite-client, then INTERFACE is correct, but the other dependencies might also need to be INTERFACE if their headers are exposed. Additionally, INTERFACE dependencies must be available at consumer link time, which means ignite-common must be properly exported and findable. ```suggestion PUBLIC ignite-common ignite-tuple ignite-network ignite-protocol PRIVATE uni-algo::uni-algo) ``` ########## modules/platforms/cpp/ignite/odbc/CMakeLists.txt: ########## @@ -56,16 +56,17 @@ set(EXTRA_FILES) if (WIN32) string(REPLACE "." "," CMAKE_PROJECT_VERSION_COMMAS ${CMAKE_PROJECT_VERSION}) - configure_file(${CMAKE_CURRENT_SOURCE_DIR}/version.rc.in ${CMAKE_CURRENT_BINARY_DIR}/version.rc @ONLY) + configure_file(${IGNITE_CMAKE_TOP_DIR}/version.rc.in ${CMAKE_CURRENT_BINARY_DIR}/version.rc @ONLY) Review Comment: The configure_file command references ${IGNITE_CMAKE_TOP_DIR}/version.rc.in, but this file doesn't exist at the top level. The version.rc.in file exists at ignite/odbc/version.rc.in (as shown in the diff context). This should be ${CMAKE_CURRENT_SOURCE_DIR}/version.rc.in to reference the local file, or the file needs to be moved to the top-level directory as this change seems to intend. ```suggestion configure_file(${CMAKE_CURRENT_SOURCE_DIR}/version.rc.in ${CMAKE_CURRENT_BINARY_DIR}/version.rc @ONLY) ``` ########## modules/platforms/cpp/CMakeLists.txt: ########## @@ -191,3 +195,34 @@ if (CLANG_FORMAT_BIN) else() message(STATUS "Failed to find clang-format. So there is no 'format' target now.") endif() + +if (${INSTALL_IGNITE_FILES}) + if (${ENABLE_CLIENT}) + list(APPEND IGNITE_AVAILABLE_COMPONENTS client) + endif() + if (${ENABLE_ODBC}) + list(APPEND IGNITE_AVAILABLE_COMPONENTS odbc) + endif() + + include(CMakePackageConfigHelpers) + configure_package_config_file( + ${IGNITE_CMAKE_TOP_DIR}/cmake/ignite-config.cmake.in + ${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config.cmake + INSTALL_DESTINATION ${CMAKE_INSTALL_PREFIX} + PATH_VARS IGNITE_INCLUDEDIR + ) + + add_subdirectory(pkgconfig) + + set(IGNITE_INSTALL_CMAKEDIR ${CMAKE_INSTALL_LIBDIR}/cmake) + + write_basic_package_version_file(${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config-version.cmake + VERSION ${PROJECT_VERSION} + COMPATIBILITY ExactVersion) Review Comment: A custom ignite-config-version.cmake.in template file exists but is not being used. The code uses write_basic_package_version_file() which generates a version file automatically, ignoring the custom template. Either remove the unused template file, or use configure_file() to process the custom template. The custom template implements a less restrictive version compatibility check (VERSION_LESS instead of exact match) which may be intentional. ```suggestion configure_file( ${IGNITE_CMAKE_TOP_DIR}/cmake/ignite-config-version.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config-version.cmake @ONLY ) ``` ########## modules/platforms/cpp/tests/package-test/cmake_package/CMakeLists.txt: ########## @@ -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. +# + +cmake_minimum_required(VERSION 3.18) + +project(ignite_package_test) + +set(IGNITE_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../../../") +set(IGNITE_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/cmake_package") + +execute_process( + COMMAND "${CMAKE_COMMAND}" + "-H${IGNITE_SOURCE_DIR}" + "-B${IGNITE_BINARY_DIR}" + "-DENABLE_ODBC=${ENABLE_ODBC}" + "-DENABLE_CLIENT=${ENABLE_CLIENT}" +) + +execute_process( + COMMAND "${CMAKE_COMMAND}" + --build "${IGNITE_BINARY_DIR}" +) + +set(ignite_DIR "${IGNITE_BINARY_DIR}/cmake/") Review Comment: The ignite_DIR is set to "${IGNITE_BINARY_DIR}/cmake/" to locate the config file, which expects to find ignite-config.cmake directly in that directory. However, based on the main CMakeLists.txt, the component target files are exported to subdirectory "ignite" (e.g., cmake/ignite/ignite-client-targets.cmake). The ignite_DIR should be set to "${IGNITE_BINARY_DIR}/cmake/ignite/" to match where the targets will actually be located, or the main CMakeLists.txt should be fixed to install everything consistently. ```suggestion set(ignite_DIR "${IGNITE_BINARY_DIR}/cmake/ignite/") ``` ########## modules/platforms/cpp/tests/package-test/README.md: ########## @@ -0,0 +1,42 @@ +# Ignite Package Test + +This directory contains tests to verify that the installed Ignite package can be found and used correctly via CMake's `find_package()` mechanism. + +## Purpose + +These tests verify: +1. The `ignite-config.cmake` package configuration works correctly +2. Component-based discovery (client, odbc) functions properly +3. The exported CMake targets link correctly +4. Headers and libraries are accessible + +## Building the Tests + +Each test cmake can build two tests: `test_client` and `test_odbc`, there are corresponding cmake options: + * `ENABLE_CLIENT` + * `ENABLE_ODBC` +These options could be used together. Each cmake configuration should build two tests (if `ENABLE_CLIENT=ON` and `ENABLE_ODBC=ON` specified). Review Comment: Grammar issue: "Each test cmake can build two tests" should be "Each test configuration can build two tests" or "Each CMake test can build two executables". ```suggestion Each test configuration can build two executables: `test_client` and `test_odbc`; there are corresponding CMake options: * `ENABLE_CLIENT` * `ENABLE_ODBC` These options can be used together. Each CMake configuration should build two tests (if `ENABLE_CLIENT=ON` and `ENABLE_ODBC=ON` are specified). ``` ########## modules/platforms/cpp/tests/package-test/cmake_package_install/CMakeLists.txt: ########## @@ -0,0 +1,73 @@ +# +# 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. +# + +cmake_minimum_required(VERSION 3.18) + +project(ignite_package_install_test) + +set(IGNITE_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../../../") +set(IGNITE_INSTALL_DIR "${CMAKE_CURRENT_BINARY_DIR}/cmake_package_install") +set(IGNITE_BINARY_DIR "${IGNITE_INSTALL_DIR}${CMAKE_FILES_DIRECTORY}") + +execute_process( + COMMAND "${CMAKE_COMMAND}" + "-H${IGNITE_SOURCE_DIR}" + "-B${IGNITE_BINARY_DIR}" + "-DENABLE_ODBC=${ENABLE_ODBC}" + "-DENABLE_CLIENT=${ENABLE_CLIENT}" + "-DENABLE_TESTS=OFF" + "-DCMAKE_INSTALL_PREFIX=${IGNITE_INSTALL_DIR}" +) + +execute_process( + COMMAND "${CMAKE_COMMAND}" + --build "${IGNITE_BINARY_DIR}" + --target install +) + +list(INSERT CMAKE_PREFIX_PATH 0 "${IGNITE_INSTALL_DIR}") + +set(ignite_DIR "${IGNITE_BINARY_DIR}/cmake/") Review Comment: Same issue as cmake_package: the ignite_DIR is set to "${IGNITE_BINARY_DIR}/cmake/" but the component target files will be in a subdirectory. Should be "${IGNITE_BINARY_DIR}/cmake/ignite/" to match the export locations. ```suggestion set(ignite_DIR "${IGNITE_BINARY_DIR}/cmake/ignite/") ``` ########## modules/platforms/cpp/tests/package-test/README.md: ########## @@ -0,0 +1,42 @@ +# Ignite Package Test + +This directory contains tests to verify that the installed Ignite package can be found and used correctly via CMake's `find_package()` mechanism. + +## Purpose + +These tests verify: +1. The `ignite-config.cmake` package configuration works correctly +2. Component-based discovery (client, odbc) functions properly +3. The exported CMake targets link correctly +4. Headers and libraries are accessible + +## Building the Tests + +Each test cmake can build two tests: `test_client` and `test_odbc`, there are corresponding cmake options: + * `ENABLE_CLIENT` + * `ENABLE_ODBC` +These options could be used together. Each cmake configuration should build two tests (if `ENABLE_CLIENT=ON` and `ENABLE_ODBC=ON` specified). +Successful compilation of these tests means we have found Ignite components, include directories and libraries to link with. + +### `cmake_package` + +This test implies that there is Ignite source code folder and Ignite was built at least once, but was not installed. +This test uses `ignite-config.cmake` which will be generated in the binary directory. + +Build command: +`cmake -DENABLE_CLIENT=ON -DENABLE_ODBC=ON -S ../tests/package-test/cmake_package/ -B build_package && cmake --build build_package` + +### `cmake_package_install` + +This test will search for the Ignite installation. For test purposes install will be done in the directory inside build directory. Review Comment: Grammar issue: "install will be done in the directory inside build directory" should be "the install will be done in a directory inside the build directory". ```suggestion This test will search for the Ignite installation. For test purposes, the install will be done in a directory inside the build directory. ``` ########## modules/platforms/cpp/CMakeLists.txt: ########## @@ -191,3 +195,34 @@ if (CLANG_FORMAT_BIN) else() message(STATUS "Failed to find clang-format. So there is no 'format' target now.") endif() + +if (${INSTALL_IGNITE_FILES}) + if (${ENABLE_CLIENT}) + list(APPEND IGNITE_AVAILABLE_COMPONENTS client) + endif() + if (${ENABLE_ODBC}) + list(APPEND IGNITE_AVAILABLE_COMPONENTS odbc) + endif() + + include(CMakePackageConfigHelpers) + configure_package_config_file( + ${IGNITE_CMAKE_TOP_DIR}/cmake/ignite-config.cmake.in + ${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config.cmake + INSTALL_DESTINATION ${CMAKE_INSTALL_PREFIX} + PATH_VARS IGNITE_INCLUDEDIR + ) + + add_subdirectory(pkgconfig) + + set(IGNITE_INSTALL_CMAKEDIR ${CMAKE_INSTALL_LIBDIR}/cmake) + + write_basic_package_version_file(${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config-version.cmake + VERSION ${PROJECT_VERSION} + COMPATIBILITY ExactVersion) Review Comment: The version compatibility is set to ExactVersion, which is extremely restrictive. This means users must have the exact same version installed. For a library, SameMajorVersion or AnyNewerVersion compatibility would be more appropriate, allowing patch and minor version updates. ExactVersion should only be used if the library has strict binary compatibility requirements across all patch versions. ```suggestion COMPATIBILITY SameMajorVersion) ``` ########## modules/platforms/cpp/ignite/protocol/CMakeLists.txt: ########## @@ -34,7 +34,7 @@ set(SOURCES ) add_library(${TARGET} OBJECT ${SOURCES}) -target_link_libraries(${TARGET} ignite-common msgpack-c) +target_link_libraries(${TARGET} PUBLIC ignite-common msgpack-c) Review Comment: The target_link_libraries visibility has been changed from implicit PUBLIC to explicit PUBLIC. While this is good practice, since ignite-protocol is an OBJECT library, the PUBLIC keyword may not have the intended effect for consumers. OBJECT libraries have special linking semantics in CMake. Consider whether the dependencies should actually be INTERFACE (exposed to consumers) or if this OBJECT library's usage pattern is correct for the export mechanism. ```suggestion target_link_libraries(${TARGET} INTERFACE ignite-common msgpack-c) ``` ########## modules/platforms/cpp/tests/fake_server/CMakeLists.txt: ########## @@ -23,7 +23,7 @@ set(SOURCES main.cpp fake_server.cpp tcp_client_channel.cpp - connection_test.cpp + connection_test.cpp ) -ignite_test(${TARGET} SOURCES ${SOURCES} LIBS ignite-test-common ignite3-client) \ No newline at end of file +ignite_test(${TARGET} SOURCES ${SOURCES} LIBS ignite-test-common ignite3-client msgpack-c) Review Comment: The msgpack-c library has been added as an explicit dependency. This suggests it was previously inherited transitively but is now needed explicitly. This is likely due to changes in the target_link_libraries visibility in ignite-protocol (changed to PUBLIC). This change appears correct but should be verified that msgpack-c is properly found and available in the test environment. ```suggestion find_library(MSGPACKC_LIB NAMES msgpackc msgpackc-cxx msgpack-c DOC "msgpack-c library for fake server tests" ) if (NOT MSGPACKC_LIB) message(FATAL_ERROR "msgpack-c library not found. Please install msgpack-c and ensure it is discoverable by CMake.") endif() ignite_test(${TARGET} SOURCES ${SOURCES} LIBS ignite-test-common ignite3-client ${MSGPACKC_LIB}) ``` -- 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]
