Copilot commented on code in PR #7: URL: https://github.com/apache/paimon-cpp/pull/7#discussion_r3287833553
########## cmake_modules/FindPackageUtils.cmake: ########## @@ -0,0 +1,94 @@ +# 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. + +function(paimon_find_target_headers OUT_VAR TARGET_NAME) + set(options NO_DEFAULT_PATH) + set(one_value_args) + set(multi_value_args NAMES HINTS PATH_SUFFIXES) + cmake_parse_arguments(ARG + "${options}" + "${one_value_args}" + "${multi_value_args}" + ${ARGN}) Review Comment: paimon_find_target_headers()` declares a `NO_DEFAULT_PATH` option, but it unconditionally appends `NO_DEFAULT_PATH` to `_find_args` (line 71). This makes the function ignore default search locations even when callers did not request that behavior, and can cause header discovery to fail when only system/default paths contain headers. Make `NO_DEFAULT_PATH` conditional on `ARG_NO_DEFAULT_PATH` (and/or default to allowing standard paths when no explicit hints are provided). ########## cmake_modules/FindPackageUtils.cmake: ########## @@ -0,0 +1,94 @@ +# 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. + +function(paimon_find_target_headers OUT_VAR TARGET_NAME) + set(options NO_DEFAULT_PATH) + set(one_value_args) + set(multi_value_args NAMES HINTS PATH_SUFFIXES) + cmake_parse_arguments(ARG + "${options}" + "${one_value_args}" + "${multi_value_args}" + ${ARGN}) + + if(NOT TARGET ${TARGET_NAME} OR NOT ARG_NAMES) + set(${OUT_VAR} + "${OUT_VAR}-NOTFOUND" + PARENT_SCOPE) + return() + endif() + + set(_target_include_dirs) + foreach(_property INTERFACE_INCLUDE_DIRECTORIES INTERFACE_SYSTEM_INCLUDE_DIRECTORIES + INCLUDE_DIRECTORIES) + get_target_property(_property_value ${TARGET_NAME} ${_property}) + if(_property_value AND NOT _property_value MATCHES "-NOTFOUND$") + list(APPEND _target_include_dirs ${_property_value}) + endif() + endforeach() + + set(_search_dirs) + foreach(_dir IN LISTS _target_include_dirs) + if(_dir MATCHES "^\\$<BUILD_INTERFACE:(.*)>$") + list(APPEND _search_dirs "${CMAKE_MATCH_1}") + elseif(_dir MATCHES "^\\$<INSTALL_INTERFACE:(.*)>$") + if(IS_ABSOLUTE "${CMAKE_MATCH_1}") + list(APPEND _search_dirs "${CMAKE_MATCH_1}") + endif() + elseif(NOT _dir MATCHES "^\\$<") + list(APPEND _search_dirs "${_dir}") + endif() + endforeach() + + list(APPEND _search_dirs ${ARG_HINTS}) + if(_search_dirs) + list(REMOVE_DUPLICATES _search_dirs) + endif() + + string(MAKE_C_IDENTIFIER "${TARGET_NAME}_${ARG_NAMES}" _header_var_suffix) + set(_header_dir_var "PAIMON_${_header_var_suffix}_HEADER_DIR") + set(_find_args NAMES ${ARG_NAMES}) + if(_search_dirs) + list(APPEND _find_args HINTS ${_search_dirs}) + endif() + if(ARG_PATH_SUFFIXES) + list(APPEND _find_args PATH_SUFFIXES ${ARG_PATH_SUFFIXES}) + endif() + list(APPEND _find_args NO_DEFAULT_PATH) Review Comment: paimon_find_target_headers()` declares a `NO_DEFAULT_PATH` option, but it unconditionally appends `NO_DEFAULT_PATH` to `_find_args` (line 71). This makes the function ignore default search locations even when callers did not request that behavior, and can cause header discovery to fail when only system/default paths contain headers. Make `NO_DEFAULT_PATH` conditional on `ARG_NO_DEFAULT_PATH` (and/or default to allowing standard paths when no explicit hints are provided). ########## cmake_modules/FindZLIBAlt.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(_PAIMON_ZLIB_ROOTS ${ZLIB_ROOT} ${PAIMON_PACKAGE_PREFIX}) +list(REMOVE_ITEM _PAIMON_ZLIB_ROOTS "") +if(_PAIMON_ZLIB_ROOTS) + set(_PAIMON_ZLIB_FIND_ARGS HINTS ${_PAIMON_ZLIB_ROOTS} NO_DEFAULT_PATH) +endif() + +include(FindPackageUtils) +if(_PAIMON_ZLIB_ROOTS) + find_package(ZLIB CONFIG QUIET ${_PAIMON_ZLIB_FIND_ARGS}) +else() + find_package(ZLIB QUIET) +endif() + +if(TARGET ZLIB::ZLIB) + paimon_find_target_headers(ZLIB_INCLUDE_DIR + ZLIB::ZLIB + NAMES + zlib.h + HINTS + ${ZLIB_INCLUDE_DIRS} + ${_PAIMON_ZLIB_FIND_ARGS}) Review Comment: This call mixes an explicit `HINTS ${ZLIB_INCLUDE_DIRS}` with `${_PAIMON_ZLIB_FIND_ARGS}`, which (when roots are set) itself expands to `HINTS ... NO_DEFAULT_PATH`. That results in duplicated `HINTS` keywords being passed through `cmake_parse_arguments()`, making the actual effective hint list harder to reason about and potentially overriding earlier hint values depending on how repeated keywords are parsed. Consider normalizing by building a single consolidated hint list (e.g., append `${ZLIB_INCLUDE_DIRS}` into the same `_PAIMON_ZLIB_FIND_ARGS`-style list, or pass only one `HINTS` occurrence). ########## cmake_modules/FindORCAlt.cmake: ########## @@ -0,0 +1,94 @@ +# 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(_PAIMON_ORC_ROOTS ${ORC_ROOT} ${orc_ROOT} ${PAIMON_PACKAGE_PREFIX}) +list(REMOVE_ITEM _PAIMON_ORC_ROOTS "") +if(_PAIMON_ORC_ROOTS) + set(_PAIMON_ORC_FIND_ARGS HINTS ${_PAIMON_ORC_ROOTS} NO_DEFAULT_PATH) +endif() + +find_package(orc CONFIG QUIET ${_PAIMON_ORC_FIND_ARGS}) +find_package(ORC CONFIG QUIET ${_PAIMON_ORC_FIND_ARGS}) + +set(_PAIMON_ORC_TARGETS orc::orc ORC::orc ORC::ORC orc) +foreach(_target IN LISTS _PAIMON_ORC_TARGETS) + if(TARGET ${_target}) + set(_PAIMON_ORC_TARGET ${_target}) + break() + endif() +endforeach() + +if(_PAIMON_ORC_TARGET) + if(NOT TARGET orc::orc) + add_library(orc::orc INTERFACE IMPORTED) + target_link_libraries(orc::orc INTERFACE ${_PAIMON_ORC_TARGET}) + endif() Review Comment: ORCAlt_FOUND` is set to `TRUE` whenever a target is found, without validating that `ORC_INCLUDE_DIR` is actually resolvable/usable. `get_target_property(... INTERFACE_INCLUDE_DIRECTORIES)` can return empty or generator expressions, which means consumers may see `ORCAlt_FOUND` but no usable includes. Consider using `paimon_find_target_headers()` (like other modules) and `find_package_handle_standard_args(ORCAlt REQUIRED_VARS ORC_INCLUDE_DIR)` to keep “found” status consistent with a working include path. ########## cmake_modules/FindglogAlt.cmake: ########## @@ -0,0 +1,87 @@ +# 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(_PAIMON_GLOG_ROOTS ${glog_ROOT} ${GLOG_ROOT} ${PAIMON_PACKAGE_PREFIX}) +list(REMOVE_ITEM _PAIMON_GLOG_ROOTS "") +if(_PAIMON_GLOG_ROOTS) + set(_PAIMON_GLOG_FIND_ARGS HINTS ${_PAIMON_GLOG_ROOTS} NO_DEFAULT_PATH) +endif() + +include(FindPackageUtils) +find_package(glog CONFIG QUIET ${_PAIMON_GLOG_FIND_ARGS}) + +set(_PAIMON_GLOG_TARGETS glog::glog glog) +foreach(_target IN LISTS _PAIMON_GLOG_TARGETS) + if(TARGET ${_target}) + set(_PAIMON_GLOG_TARGET ${_target}) + break() + endif() +endforeach() + +if(_PAIMON_GLOG_TARGET) + paimon_find_target_headers(GLOG_INCLUDE_DIR + ${_PAIMON_GLOG_TARGET} + NAMES + glog/logging.h + ${_PAIMON_GLOG_FIND_ARGS}) + + include(FindPackageHandleStandardArgs) + find_package_handle_standard_args(glogAlt REQUIRED_VARS GLOG_INCLUDE_DIR) + + if(glogAlt_FOUND AND NOT TARGET glog) + add_library(glog INTERFACE IMPORTED) + set_target_properties(glog PROPERTIES INTERFACE_INCLUDE_DIRECTORIES + "${GLOG_INCLUDE_DIR}") + target_link_libraries(glog INTERFACE ${_PAIMON_GLOG_TARGET}) + endif() Review Comment: Unlike several other *Alt find modules in this PR (e.g., fmt/zstd/lz4/snappy), this module does not populate a `GLOG_LIBRARIES` variable when `glogAlt_FOUND` is true. If existing build logic expects the traditional `<PKG>_LIBRARIES` variable, this asymmetry can break consumers. Consider setting `GLOG_LIBRARIES` to the resolved target (e.g., `${_PAIMON_GLOG_TARGET}` or `glog`) when found, mirroring the pattern used in other modules. ########## cmake_modules/FindORCAlt.cmake: ########## @@ -0,0 +1,94 @@ +# 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(_PAIMON_ORC_ROOTS ${ORC_ROOT} ${orc_ROOT} ${PAIMON_PACKAGE_PREFIX}) +list(REMOVE_ITEM _PAIMON_ORC_ROOTS "") +if(_PAIMON_ORC_ROOTS) + set(_PAIMON_ORC_FIND_ARGS HINTS ${_PAIMON_ORC_ROOTS} NO_DEFAULT_PATH) +endif() + +find_package(orc CONFIG QUIET ${_PAIMON_ORC_FIND_ARGS}) +find_package(ORC CONFIG QUIET ${_PAIMON_ORC_FIND_ARGS}) + +set(_PAIMON_ORC_TARGETS orc::orc ORC::orc ORC::ORC orc) +foreach(_target IN LISTS _PAIMON_ORC_TARGETS) + if(TARGET ${_target}) + set(_PAIMON_ORC_TARGET ${_target}) + break() + endif() +endforeach() + +if(_PAIMON_ORC_TARGET) + if(NOT TARGET orc::orc) + add_library(orc::orc INTERFACE IMPORTED) + target_link_libraries(orc::orc INTERFACE ${_PAIMON_ORC_TARGET}) + endif() + + foreach(_dependency + zstd + snappy + lz4 + zlib + libprotobuf) + if(TARGET ${_dependency}) + target_link_libraries(orc::orc INTERFACE ${_dependency}) + endif() + endforeach() + + get_target_property(ORC_INCLUDE_DIR orc::orc INTERFACE_INCLUDE_DIRECTORIES) + set(ORCAlt_FOUND TRUE) +else() Review Comment: ORCAlt_FOUND` is set to `TRUE` whenever a target is found, without validating that `ORC_INCLUDE_DIR` is actually resolvable/usable. `get_target_property(... INTERFACE_INCLUDE_DIRECTORIES)` can return empty or generator expressions, which means consumers may see `ORCAlt_FOUND` but no usable includes. Consider using `paimon_find_target_headers()` (like other modules) and `find_package_handle_standard_args(ORCAlt REQUIRED_VARS ORC_INCLUDE_DIR)` to keep “found” status consistent with a working include path. ########## cmake_modules/FindRE2Alt.cmake: ########## @@ -0,0 +1,69 @@ +# 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(_PAIMON_RE2_ROOTS ${RE2_ROOT} ${re2_ROOT} ${PAIMON_PACKAGE_PREFIX}) +list(REMOVE_ITEM _PAIMON_RE2_ROOTS "") +if(_PAIMON_RE2_ROOTS) + set(_PAIMON_RE2_FIND_ARGS HINTS ${_PAIMON_RE2_ROOTS} NO_DEFAULT_PATH) +endif() + +include(FindPackageUtils) +find_package(re2 CONFIG QUIET ${_PAIMON_RE2_FIND_ARGS}) + +if(TARGET re2::re2) + paimon_find_target_headers(RE2_INCLUDE_DIR + re2::re2 + NAMES + re2/re2.h + ${_PAIMON_RE2_FIND_ARGS}) + + include(FindPackageHandleStandardArgs) + find_package_handle_standard_args(RE2Alt REQUIRED_VARS RE2_INCLUDE_DIR) + + if(RE2Alt_FOUND) + set(RE2_LIBRARIES re2::re2) + endif() +else() + find_package(PkgConfig QUIET) + if(PkgConfig_FOUND) + pkg_check_modules(PC_RE2 QUIET re2) + endif() + + find_path(RE2_INCLUDE_DIR + NAMES re2/re2.h ${_PAIMON_RE2_FIND_ARGS} + HINTS ${PC_RE2_INCLUDE_DIRS} + PATH_SUFFIXES include) + find_library(RE2_LIBRARY + NAMES re2 ${_PAIMON_RE2_FIND_ARGS} + HINTS ${PC_RE2_LIBRARY_DIRS} + PATH_SUFFIXES lib lib64) + + include(FindPackageHandleStandardArgs) + find_package_handle_standard_args(RE2Alt REQUIRED_VARS RE2_LIBRARY RE2_INCLUDE_DIR) + + if(RE2Alt_FOUND) + add_library(re2::re2 UNKNOWN IMPORTED) + set_target_properties(re2::re2 + PROPERTIES IMPORTED_LOCATION "${RE2_LIBRARY}" + INTERFACE_INCLUDE_DIRECTORIES + "${RE2_INCLUDE_DIR}") + set(RE2_LIBRARIES "${RE2_LIBRARY}") Review Comment: In the pkg-config/manual fallback path you create an imported target `re2::re2`, but then set `RE2_LIBRARIES` to the raw library path (`"${RE2_LIBRARY}"`). In the config-target path, `RE2_LIBRARIES` is set to `re2::re2`. This inconsistency can lead to different linking semantics across environments (loss of transitive usage requirements, harder-to-debug link order issues). Prefer setting `RE2_LIBRARIES` to `re2::re2` in both branches once the imported target exists. -- 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]
