Copilot commented on code in PR #7487: URL: https://github.com/apache/ignite-3/pull/7487#discussion_r2735483164
########## modules/platforms/cpp/cmake/packaging.cmake: ########## @@ -0,0 +1,74 @@ +# +# 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. +# + +set(CPACK_ERROR_ON_ABSOLUTE_INSTALL_DESTINATION ON) + +set(CPACK_COMPONENTS_ALL client odbc) + +set(CPACK_PACKAGE_NAME ignite3 CACHE STRING "The resulting package name") +set(CPACK_PACKAGE_DESCRIPTION_SUMMARY "Apache Ignite 3 client library for C++" + CACHE STRING "Package description for the package metadata" +) +set(CPACK_PACKAGE_VENDOR "Apache Software Foundation") + +set(CPACK_VERBATIM_VARIABLES YES) + +set(CPACK_PACKAGE_INSTALL_DIRECTORY ${CPACK_PACKAGE_NAME}) +SET(CPACK_OUTPUT_FILE_PREFIX "_packages") + +set(CPACK_PACKAGE_VERSION_MAJOR ${PROJECT_VERSION_MAJOR}) +set(CPACK_PACKAGE_VERSION_MINOR ${PROJECT_VERSION_MINOR}) +set(CPACK_PACKAGE_VERSION_PATCH ${PROJECT_VERSION_PATCH}) + +set(CPACK_PACKAGE_CONTACT "Apache Software Foundation") +set(CPACK_DEBIAN_PACKAGE_MAINTAINER "Apache Software Foundation") + +set(CPACK_RESOURCE_FILE_LICENSE "${IGNITE_CMAKE_TOP_DIR}/LICENSE") + +set(CPACK_DEBIAN_FILE_NAME DEB-DEFAULT) +set(CPACK_RPM_FILE_NAME RPM-DEFAULT) +set(CPACK_COMPONENTS_GROUPING ONE_PER_GROUP) +set(CPACK_DEB_COMPONENT_INSTALL ON) +set(CPACK_RPM_COMPONENT_INSTALL ON) + +set(CPACK_ARCHIVE_COMPONENT_INSTALL 1) +set(CPACK_TGZ_COMPONENT_INSTALL ON) + +set(ODBC_SCRIPT_DIR "${IGNITE_CMAKE_TOP_DIR}/cmake/scripts/odbc") + +configure_file("${ODBC_SCRIPT_DIR}/post_install.sh" "${CMAKE_CURRENT_BINARY_DIR}/postinst" COPYONLY) +configure_file("${ODBC_SCRIPT_DIR}/pre_uninstall.sh" "${CMAKE_CURRENT_BINARY_DIR}/prerm" COPYONLY) +set(CPACK_DEBIAN_ODBC_PACKAGE_CONTROL_EXTRA "${CMAKE_CURRENT_BINARY_DIR}/postinst;${CMAKE_CURRENT_BINARY_DIR}/prerm") +set(CPACK_DEBIAN_ODBC_PACKAGE_CONTROL_STRICT_PERMISSION TRUE) +set(CPACK_DEBIAN_ODBC_PACKAGE_DEPENDS unixodbc) + +configure_file("${ODBC_SCRIPT_DIR}/post_install.sh" "${CMAKE_CURRENT_BINARY_DIR}/post_install.sh" COPYONLY) +configure_file("${ODBC_SCRIPT_DIR}/pre_uninstall.sh" "${CMAKE_CURRENT_BINARY_DIR}/pre_uninstall.sh" COPYONLY) +set(CPACK_RPM_ODBC_POST_INSTALL_SCRIPT_FILE "${CMAKE_CURRENT_BINARY_DIR}/post_install.sh") +set(CPACK_RPM_ODBC_PRE_UNINSTALL_SCRIPT_FILE "${CMAKE_CURRENT_BINARY_DIR}/pre_uninstall.sh") +set(CPACK_RPM_ODBC_PACKAGE_DEPENDS unixodbc) + +install(FILES + "${ODBC_SCRIPT_DIR}/ignite3-odbc-linux.ini" + COMPONENT odbc + DESTINATION "${CMAKE_INSTALL_DATAROOTDIR}/ignite/" + RENAME "ignite3-odbc.ini" +) + +set(CPACK_COMPONENT_CLIENT_DESCRIPTION "Apache Ignite 3 client library for C++") +set(CPACK_COMPONENT_ODBC_DESCRIPTION "Apache Ignite ODBC driver") Review Comment: Minor inconsistency in component description: The client component uses "Apache Ignite 3 client library for C++" while the ODBC component uses "Apache Ignite ODBC driver" (without the version "3"). For consistency, consider changing the ODBC description to "Apache Ignite 3 ODBC driver" to match the client component's naming pattern. ```suggestion set(CPACK_COMPONENT_ODBC_DESCRIPTION "Apache Ignite 3 ODBC driver") ``` ########## modules/platforms/cpp/cmake/packaging.cmake: ########## @@ -0,0 +1,74 @@ +# +# 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. +# + +set(CPACK_ERROR_ON_ABSOLUTE_INSTALL_DESTINATION ON) + +set(CPACK_COMPONENTS_ALL client odbc) + +set(CPACK_PACKAGE_NAME ignite3 CACHE STRING "The resulting package name") +set(CPACK_PACKAGE_DESCRIPTION_SUMMARY "Apache Ignite 3 client library for C++" + CACHE STRING "Package description for the package metadata" +) +set(CPACK_PACKAGE_VENDOR "Apache Software Foundation") + +set(CPACK_VERBATIM_VARIABLES YES) + +set(CPACK_PACKAGE_INSTALL_DIRECTORY ${CPACK_PACKAGE_NAME}) +SET(CPACK_OUTPUT_FILE_PREFIX "_packages") Review Comment: Inconsistent use of set() case. Line 31 uses SET() with uppercase while all other set() commands in this file use lowercase. For consistency, this should be changed to lowercase: set(CPACK_OUTPUT_FILE_PREFIX "_packages") ```suggestion set(CPACK_OUTPUT_FILE_PREFIX "_packages") ``` ########## modules/platforms/cpp/cmake/scripts/odbc/post_install.sh: ########## @@ -0,0 +1,18 @@ +# Review Comment: The shell script is missing a shebang line (#!/bin/bash or #!/bin/sh). Without a shebang, the script may not execute correctly when called by package managers or directly. Add a shebang line at the beginning of the file before the license header. ########## modules/platforms/cpp/CMakeLists.txt: ########## @@ -216,6 +217,14 @@ write_basic_package_version_file( install(FILES "${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config.cmake" "${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config-version.cmake" + COMPONENT client + DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/ignite" +) + +install(FILES + "${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config.cmake" + "${CMAKE_CURRENT_BINARY_DIR}/cmake/ignite-config-version.cmake" + COMPONENT odbc DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/ignite" ) Review Comment: These cmake config files are being installed twice with different components (client and odbc). This creates duplicate files in packages that include both components. Consider using a single install rule without a component specification, or installing to component-specific directories if truly needed separately. ```suggestion DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/ignite" ) ``` ########## modules/platforms/cpp/cmake/scripts/odbc/post_install.sh: ########## @@ -0,0 +1,18 @@ +# +# 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. +# + +odbcinst -i -d -f /usr/share/ignite/ignite3-odbc.ini -v Review Comment: The path /usr/share/ignite/ignite3-odbc.ini is hardcoded, which may not be correct for non-standard installation prefixes. Consider making this path configurable or using CMAKE_INSTALL_FULL_DATAROOTDIR to generate the correct path at configure time. ```suggestion odbcinst -i -d -f "${IGNITE_ODBC_INI_PATH:-/usr/share/ignite/ignite3-odbc.ini}" -v ``` ########## modules/platforms/cpp/cmake/scripts/odbc/post_install.sh: ########## @@ -0,0 +1,18 @@ +# +# 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. +# + +odbcinst -i -d -f /usr/share/ignite/ignite3-odbc.ini -v Review Comment: The script lacks error handling. If odbcinst fails (e.g., if unixodbc is not installed or the ini file is missing), the script will silently continue. Consider adding error checking with 'set -e' at the beginning of the script or checking the return value of odbcinst to ensure the installation succeeds. ```suggestion if ! odbcinst -i -d -f /usr/share/ignite/ignite3-odbc.ini -v; then echo "Error: Failed to install Ignite3 ODBC driver using odbcinst." >&2 exit 1 fi ``` ########## modules/platforms/cpp/cmake/scripts/odbc/ignite3-odbc-linux.ini: ########## @@ -0,0 +1,5 @@ +[Apache Ignite 3] +Description=Apache Ignite 3 ODBC Driver +Driver=/usr/lib/libignite3-odbc.so +Setup=/usr/lib/libignite3-odbc.so Review Comment: The paths /usr/lib/libignite3-odbc.so are hardcoded in the Driver and Setup fields, which may not be correct for non-standard installation prefixes or different library directories (e.g., /usr/lib64 on some systems). Consider configuring this file at build time using configure_file() and CMAKE_INSTALL_FULL_LIBDIR to generate the correct paths. ```suggestion Driver=@CMAKE_INSTALL_FULL_LIBDIR@/libignite3-odbc.so Setup=@CMAKE_INSTALL_FULL_LIBDIR@/libignite3-odbc.so ``` ########## modules/platforms/cpp/cmake/packaging.cmake: ########## @@ -0,0 +1,74 @@ +# +# 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. +# + +set(CPACK_ERROR_ON_ABSOLUTE_INSTALL_DESTINATION ON) + +set(CPACK_COMPONENTS_ALL client odbc) + +set(CPACK_PACKAGE_NAME ignite3 CACHE STRING "The resulting package name") +set(CPACK_PACKAGE_DESCRIPTION_SUMMARY "Apache Ignite 3 client library for C++" + CACHE STRING "Package description for the package metadata" +) +set(CPACK_PACKAGE_VENDOR "Apache Software Foundation") + +set(CPACK_VERBATIM_VARIABLES YES) + +set(CPACK_PACKAGE_INSTALL_DIRECTORY ${CPACK_PACKAGE_NAME}) +SET(CPACK_OUTPUT_FILE_PREFIX "_packages") + +set(CPACK_PACKAGE_VERSION_MAJOR ${PROJECT_VERSION_MAJOR}) +set(CPACK_PACKAGE_VERSION_MINOR ${PROJECT_VERSION_MINOR}) +set(CPACK_PACKAGE_VERSION_PATCH ${PROJECT_VERSION_PATCH}) + +set(CPACK_PACKAGE_CONTACT "Apache Software Foundation") +set(CPACK_DEBIAN_PACKAGE_MAINTAINER "Apache Software Foundation") + +set(CPACK_RESOURCE_FILE_LICENSE "${IGNITE_CMAKE_TOP_DIR}/LICENSE") + +set(CPACK_DEBIAN_FILE_NAME DEB-DEFAULT) +set(CPACK_RPM_FILE_NAME RPM-DEFAULT) +set(CPACK_COMPONENTS_GROUPING ONE_PER_GROUP) +set(CPACK_DEB_COMPONENT_INSTALL ON) +set(CPACK_RPM_COMPONENT_INSTALL ON) + +set(CPACK_ARCHIVE_COMPONENT_INSTALL 1) +set(CPACK_TGZ_COMPONENT_INSTALL ON) + +set(ODBC_SCRIPT_DIR "${IGNITE_CMAKE_TOP_DIR}/cmake/scripts/odbc") + +configure_file("${ODBC_SCRIPT_DIR}/post_install.sh" "${CMAKE_CURRENT_BINARY_DIR}/postinst" COPYONLY) +configure_file("${ODBC_SCRIPT_DIR}/pre_uninstall.sh" "${CMAKE_CURRENT_BINARY_DIR}/prerm" COPYONLY) +set(CPACK_DEBIAN_ODBC_PACKAGE_CONTROL_EXTRA "${CMAKE_CURRENT_BINARY_DIR}/postinst;${CMAKE_CURRENT_BINARY_DIR}/prerm") +set(CPACK_DEBIAN_ODBC_PACKAGE_CONTROL_STRICT_PERMISSION TRUE) +set(CPACK_DEBIAN_ODBC_PACKAGE_DEPENDS unixodbc) + +configure_file("${ODBC_SCRIPT_DIR}/post_install.sh" "${CMAKE_CURRENT_BINARY_DIR}/post_install.sh" COPYONLY) +configure_file("${ODBC_SCRIPT_DIR}/pre_uninstall.sh" "${CMAKE_CURRENT_BINARY_DIR}/pre_uninstall.sh" COPYONLY) Review Comment: The configure_file() command copies the shell scripts but doesn't set executable permissions. While CPACK_DEBIAN_ODBC_PACKAGE_CONTROL_STRICT_PERMISSION is set, the source files should be marked as executable or use FILE_PERMISSIONS in the configure_file call. Consider using configure_file with @ONLY mode and file(COPY ... FILE_PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE) or ensure the source files have executable permissions in the repository. ```suggestion file(COPY "${ODBC_SCRIPT_DIR}/post_install.sh" "${ODBC_SCRIPT_DIR}/pre_uninstall.sh" DESTINATION "${CMAKE_CURRENT_BINARY_DIR}" FILE_PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE ) set(CPACK_DEBIAN_ODBC_PACKAGE_CONTROL_EXTRA "${CMAKE_CURRENT_BINARY_DIR}/postinst;${CMAKE_CURRENT_BINARY_DIR}/prerm") set(CPACK_DEBIAN_ODBC_PACKAGE_CONTROL_STRICT_PERMISSION TRUE) set(CPACK_DEBIAN_ODBC_PACKAGE_DEPENDS unixodbc) file(COPY "${ODBC_SCRIPT_DIR}/post_install.sh" "${ODBC_SCRIPT_DIR}/pre_uninstall.sh" DESTINATION "${CMAKE_CURRENT_BINARY_DIR}" FILE_PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE ) ``` ########## modules/platforms/cpp/cmake/scripts/odbc/pre_uninstall.sh: ########## @@ -0,0 +1,18 @@ +# +# 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. +# + +odbcinst -u -d -n "Apache Ignite 3" -v Review Comment: The script lacks error handling. If odbcinst fails (e.g., driver is not registered), the script will return a non-zero exit code which could cause package removal to fail. Consider adding error suppression (|| true) or 'set -e; set +e' around the command to prevent uninstallation failures if the driver was never registered or already removed. ```suggestion # Best-effort: ignore failures so uninstall does not fail if the driver is not registered or already removed. odbcinst -u -d -n "Apache Ignite 3" -v || true ``` ########## modules/platforms/cpp/cmake/scripts/odbc/pre_uninstall.sh: ########## @@ -0,0 +1,18 @@ +# Review Comment: The shell script is missing a shebang line (#!/bin/bash or #!/bin/sh). Without a shebang, the script may not execute correctly when called by package managers or directly. Add a shebang line at the beginning of the file before the license header. ########## modules/platforms/cpp/cmake/ignite_install_headers.cmake: ########## @@ -19,10 +19,10 @@ # # Function to install header files. function(ignite_install_headers) - cmake_parse_arguments(IGNITE_INSTALL "" "DESTINATION" "FILES" ${ARGN}) + cmake_parse_arguments(IGNITE_INSTALL "" "DESTINATION" "FILES;COMPONENT" ${ARGN}) Review Comment: The COMPONENT parameter should be in the one-value argument section of cmake_parse_arguments, not in the multi-value section. Currently COMPONENT is listed together with FILES which treats it as a multi-value list. The correct syntax should be: cmake_parse_arguments(IGNITE_INSTALL "" "DESTINATION;COMPONENT" "FILES" ${ARGN}) ```suggestion cmake_parse_arguments(IGNITE_INSTALL "" "DESTINATION;COMPONENT" "FILES" ${ARGN}) ``` -- 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]
