kou commented on code in PR #6: URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900073077
########## README.md: ########## @@ -44,9 +61,14 @@ After installing the core libraries, you can build the examples: ```bash cd iceberg-cpp/example -mkdir build && cd build -cmake .. -DCMAKE_PREFIX_PATH=/tmp/iceberg -cmake --build . +cmake -S . -B build -DCMAKE_PREFIX_PATH=/path/to/install +cmake --build build +``` + +If you are using provided Apache Arrow, you need to include `/path/to/arrow` in `CMAKE_PREFIX_PATH` as below. + +```bash +cmake .. -DCMAKE_PREFIX_PATH="/path/to/install;/path/to/arrow" Review Comment: ```suggestion cmake -S . -B build -DCMAKE_PREFIX_PATH="/path/to/install;/path/to/arrow" ``` ########## cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -0,0 +1,132 @@ +# 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. + +# Accumulate all dependencies to provide suitable static link parameters to the +# third party libraries. +set(ICEBERG_SYSTEM_DEPENDENCIES) +set(ICEBERG_ARROW_INSTALL_INTERFACE_LIBS) + +# ---------------------------------------------------------------------- +# Versions and URLs for toolchain builds + +set(ICEBERG_ARROW_BUILD_VERSION "18.1.0") +set(ICEBERG_ARROW_BUILD_SHA256_CHECKSUM + "2dc8da5f8796afe213ecc5e5aba85bb82d91520eff3cf315784a52d0fa61d7fc") + +if(DEFINED ENV{ICEBERG_ARROW_URL}) + set(ARROW_SOURCE_URL "$ENV{ICEBERG_ARROW_URL}") +else() + set(ARROW_SOURCE_URL + "https://www.apache.org/dyn/closer.cgi?action=download&filename=/arrow/arrow-${ICEBERG_ARROW_BUILD_VERSION}/apache-arrow-${ICEBERG_ARROW_BUILD_VERSION}.tar.gz"; Review Comment: `.lua` is better: https://infra.apache.org/release-download-pages.html#download-page ```suggestion "https://www.apache.org/dyn/closer.lua?action=download&filename=/arrow/arrow-${ICEBERG_ARROW_BUILD_VERSION}/apache-arrow-${ICEBERG_ARROW_BUILD_VERSION}.tar.gz"; ``` ########## example/CMakeLists.txt: ########## @@ -22,10 +22,10 @@ project(example) set(CMAKE_CXX_STANDARD 20) -find_package(iceberg CONFIG REQUIRED) -find_package(puffin CONFIG REQUIRED) +find_package(Iceberg CONFIG REQUIRED) add_executable(demo_example demo_example.cc) -target_link_libraries(demo_example PRIVATE iceberg::iceberg_core_static - puffin::iceberg_puffin_static) +target_link_libraries(demo_example + PRIVATE Iceberg::iceberg_core_static Iceberg::iceberg_puffin_static + Iceberg::iceberg_arrow_static) Review Comment: OK. Then `PRIVATE Iceberg::iceberg_puffin_static Iceberg::iceberg_arrow_static` will be better. (`Iceberg::iceberg_core_static` should be linked after `Iceberg::iceberg_{puffine,arrow}_static`. `Iceberg::iceberg_core_static` should be linked automatically by `Iceberg::iceberg_{arrow,puffine}_static`.) ########## cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -0,0 +1,142 @@ +# 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. + +# Accumulate all dependencies to provide suitable static link parameters to the +# third party libraries. +set(ICEBERG_SYSTEM_DEPENDENCIES) +set(ICEBERG_VENDOR_DEPENDENCIES) +set(ICEBERG_ARROW_INSTALL_INTERFACE_LIBS) + +# ---------------------------------------------------------------------- +# Versions and URLs for toolchain builds + +set(ICEBERG_ARROW_BUILD_VERSION "18.1.0") +set(ICEBERG_ARROW_BUILD_SHA256_CHECKSUM + "2dc8da5f8796afe213ecc5e5aba85bb82d91520eff3cf315784a52d0fa61d7fc") +set(ARROW_VENDORED TRUE) + +if(DEFINED ENV{ICEBERG_ARROW_URL}) + set(ARROW_SOURCE_URL "$ENV{ICEBERG_ARROW_URL}") +else() + set(ARROW_SOURCE_URL + "https://www.apache.org/dyn/closer.cgi?action=download&filename=/arrow/arrow-${ICEBERG_ARROW_BUILD_VERSION}/apache-arrow-${ICEBERG_ARROW_BUILD_VERSION}.tar.gz"; + "https://downloads.apache.org/arrow/arrow-${ICEBERG_ARROW_BUILD_VERSION}/apache-arrow-${ICEBERG_ARROW_BUILD_VERSION}.tar.gz"; + "https://github.com/apache/arrow/releases/download/apache-arrow-${ICEBERG_ARROW_BUILD_VERSION}/apache-arrow-${ICEBERG_ARROW_BUILD_VERSION}.tar.gz"; + ) +endif() + +# ---------------------------------------------------------------------- +# FetchContent + +include(FetchContent) +set(FC_DECLARE_COMMON_OPTIONS) +if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.28) + list(APPEND FC_DECLARE_COMMON_OPTIONS EXCLUDE_FROM_ALL TRUE) +endif() + +macro(prepare_fetchcontent) + set(BUILD_SHARED_LIBS OFF) + set(BUILD_STATIC_LIBS ON) + set(CMAKE_COMPILE_WARNING_AS_ERROR FALSE) + set(CMAKE_EXPORT_NO_PACKAGE_REGISTRY TRUE) + set(CMAKE_POSITION_INDEPENDENT_CODE ON) +endmacro() + +# ---------------------------------------------------------------------- +# Apache Arrow + +function(resolve_arrow_dependency) + prepare_fetchcontent() + + set(ARROW_BUILD_SHARED + OFF + CACHE BOOL "" FORCE) + set(ARROW_BUILD_STATIC + ON + CACHE BOOL "" FORCE) + set(ARROW_FILESYSTEM + OFF + CACHE BOOL "" FORCE) + set(ARROW_SIMD_LEVEL + "NONE" + CACHE STRING "" FORCE) + set(ARROW_RUNTIME_SIMD_LEVEL + "NONE" + CACHE STRING "" FORCE) + set(ARROW_POSITION_INDEPENDENT_CODE + ON + CACHE BOOL "" FORCE) + set(ARROW_DEPENDENCY_SOURCE + "AUTO" + CACHE STRING "" FORCE) + + fetchcontent_declare(Arrow + ${FC_DECLARE_COMMON_OPTIONS} + URL ${ARROW_SOURCE_URL} + URL_HASH "SHA256=${ICEBERG_ARROW_BUILD_SHA256_CHECKSUM}" + SOURCE_SUBDIR + cpp + FIND_PACKAGE_ARGS + NAMES + Arrow + CONFIG) + + # Add Arrow cmake modules to the search path + list(PREPEND CMAKE_MODULE_PATH + ${CMAKE_CURRENT_BINARY_DIR}/_deps/arrow-src/cpp/cmake_modules) Review Comment: Ah, we should use `PREPEND` not `APPEND` here...: https://github.com/apache/arrow/blob/1e45e182a71d51223cfb82b236dde07b436f44f1/cpp/CMakeLists.txt#L95 ########## src/arrow/CMakeLists.txt: ########## @@ -0,0 +1,65 @@ +# 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. + +if(NOT ICEBERG_ARROW) + return() +endif() + +set(ICEBERG_ARROW_SOURCES demo_arrow.cc) +set(ICEBERG_ARROW_INCLUDES "${ICEBERG_API_DIR}") + +# Libraries to link with exported libiceberg_arrow.{so,a}. +set(ICEBERG_ARROW_STATIC_BUILD_INTERFACE_LIBS) +set(ICEBERG_ARROW_SHARED_BUILD_INTERFACE_LIBS) +set(ICEBERG_ARROW_STATIC_INSTALL_INTERFACE_LIBS) +set(ICEBERG_ARROW_SHARED_INSTALL_INTERFACE_LIBS) + +list(APPEND ICEBERG_ARROW_STATIC_BUILD_INTERFACE_LIBS + "$<IF:$<TARGET_EXISTS:Arrow::arrow_static>,Arrow::arrow_static,Arrow::arrow_shared>") +list(APPEND ICEBERG_ARROW_SHARED_BUILD_INTERFACE_LIBS + "$<IF:$<TARGET_EXISTS:Arrow::arrow_shared>,Arrow::arrow_shared,Arrow::arrow_static>") + +if(ARROW_VENDORED) + list(APPEND ICEBERG_ARROW_STATIC_INSTALL_INTERFACE_LIBS Iceberg::arrow_static) + list(APPEND ICEBERG_ARROW_SHARED_INSTALL_INTERFACE_LIBS Iceberg::arrow_static) +else() + list(APPEND + ICEBERG_ARROW_STATIC_INSTALL_INTERFACE_LIBS + "$<IF:$<TARGET_EXISTS:Arrow::arrow_static>,Arrow::arrow_static,Arrow::arrow_shared>" + ) + list(APPEND + ICEBERG_ARROW_SHARED_INSTALL_INTERFACE_LIBS + "$<IF:$<TARGET_EXISTS:Arrow::arrow_shared>,Arrow::arrow_shared,Arrow::arrow_static>" + ) +endif() Review Comment: ```suggestion endif() list(APPEND ICEBERG_ARROW_STATIC_INSTALL_INTERFACE_LIBS "$<IF:$<TARGET_EXISTS:iceberg_core_static>,Iceberg::iceberg_core_static,Iceberg::iceberg_core_shared>" ) list(APPEND ICEBERG_ARROW_SHARED_INSTALL_INTERFACE_LIBS "$<IF:$<TARGET_EXISTS:iceberg_core_shared>,Iceberg::iceberg_core_shared,Iceberg::iceberg_core_static>" ) ``` ########## cmake_modules/BuildUtils.cmake: ########## @@ -182,13 +183,7 @@ function(ADD_ICEBERG_LIB LIB_NAME) target_include_directories(${LIB_NAME}_static PRIVATE ${ARG_PRIVATE_INCLUDES}) endif() - if(MSVC_TOOLCHAIN) - set(LIB_NAME_STATIC ${LIB_NAME}_static) - else() - set(LIB_NAME_STATIC ${LIB_NAME}) - endif() - - set_target_properties(${LIB_NAME}_static PROPERTIES OUTPUT_NAME ${LIB_NAME_STATIC}) + set_target_properties(${LIB_NAME}_static PROPERTIES OUTPUT_NAME ${LIB_NAME}) Review Comment: If we remove this, `iceberg_core.lib` file name will be conflicted with `-DICEBERG_BUILD_SHARED=ON`, `-DICEBERG_BUILD_STATIC=ON` and `-G 'Visual Studio ...'`. ########## src/IcebergConfig.cmake.in: ########## @@ -0,0 +1,80 @@ +# 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. +# +# This config sets the following variables in your project:: +# +# Iceberg_FOUND - true if Iceberg found on the system +# Iceberg_VERSION - version of the found Iceberg +# +# This config sets the following targets (if built) in your project:: +# +# Iceberg::iceberg_core_shared +# Iceberg::iceberg_core_static +# Iceberg::iceberg_puffin_shared +# Iceberg::iceberg_puffin_static +# Iceberg::iceberg_arrow_shared +# Iceberg::iceberg_arrow_static + +@PACKAGE_INIT@ + +set(ICEBERG_BUILD_STATIC "@ICEBERG_BUILD_STATIC@") +set(ICEBERG_SYSTEM_DEPENDENCIES "@ICEBERG_SYSTEM_DEPENDENCIES@") + +include(CMakeFindDependencyMacro) + +macro(iceberg_find_dependencies dependencies) + if(DEFINED CMAKE_MODULE_PATH) + set(ICEBERG_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH}) + else() + unset(ICEBERG_CMAKE_MODULE_PATH_OLD) + endif() + set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}") + + foreach(dependency ${dependencies}) + find_dependency(${dependency}) + endforeach() + + if(DEFINED ICEBERG_CMAKE_MODULE_PATH_OLD) + set(CMAKE_MODULE_PATH ${ICEBERG_CMAKE_MODULE_PATH_OLD}) + unset(ICEBERG_CMAKE_MODULE_PATH_OLD) + else() + unset(CMAKE_MODULE_PATH) + endif() +endmacro() + +macro(iceberg_find_components components) + foreach(comp ${components}) + string(TOLOWER "${comp}" _comp_lower_case) + if(TARGET "Iceberg::iceberg_${_comp_lower_case}_shared" OR + TARGET "Iceberg::iceberg_${_comp_lower_case}_static") + set(Iceberg_${comp}_FOUND TRUE) + else() + set(Iceberg_${comp}_FOUND FALSE) + set(Iceberg_NOT_FOUND_MESSAGE "Component ${comp} was not installed") + endif() + endforeach() +endmacro() + +include("${CMAKE_CURRENT_LIST_DIR}/IcebergTargets.cmake") + +# Find required components +iceberg_find_components("${Iceberg_FIND_COMPONENTS}") + +# Find system dependencies +iceberg_find_dependencies("${ICEBERG_SYSTEM_DEPENDENCIES}") Review Comment: I think that this should be done before `include("${CMAKE_CURRENT_LIST_DIR}/IcebergTargets.cmake")`. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
