kou commented on code in PR #6: URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1899057489
########## 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"; Review Comment: We can't use this because this file is different from other above 2 URLs. (The `closer.cgi` URL and `downloads.apache.org` URL refers the same file.) We can't assign multiple checksums for this URL list. So we can't mix the last GitHub Releases URL to this URL list. ########## 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) + + fetchcontent_makeavailable(Arrow) + + if(NOT TARGET Arrow::arrow_static) + add_library(Arrow::arrow_static INTERFACE IMPORTED) + target_link_libraries(Arrow::arrow_static INTERFACE arrow_static) + target_include_directories(Arrow::arrow_static INTERFACE ${arrow_SOURCE_DIR}/cpp/src + ${arrow_BINARY_DIR}/src) + endif() + + fetchcontent_getproperties(Arrow) Review Comment: Do we need this? I think that we don't need this because we call `fetchcontent_makeavailable()`. ########## src/CMakeLists.txt: ########## @@ -15,5 +15,20 @@ # specific language governing permissions and limitations # under the License. +add_subdirectory(arrow) add_subdirectory(core) add_subdirectory(puffin) + +include(CMakePackageConfigHelpers) + +configure_package_config_file("${CMAKE_CURRENT_SOURCE_DIR}/config.cmake.in" + "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config.cmake" Review Comment: If we use `Iceberg` as a CMake package name, `IcebergConfig.cmake` will be better for consistency. (`iceberg-config.cmake` works too.) ########## 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) + + fetchcontent_makeavailable(Arrow) + + if(NOT TARGET Arrow::arrow_static) + add_library(Arrow::arrow_static INTERFACE IMPORTED) + target_link_libraries(Arrow::arrow_static INTERFACE arrow_static) + target_include_directories(Arrow::arrow_static INTERFACE ${arrow_SOURCE_DIR}/cpp/src + ${arrow_BINARY_DIR}/src) Review Comment: This order works but, in general, build directory -> source directory order is better: ```suggestion target_include_directories(Arrow::arrow_static INTERFACE ${arrow_BINARY_DIR}/src ${arrow_SOURCE_DIR}/cpp/src) ``` ########## README.md: ########## @@ -33,7 +33,29 @@ C++ implementation of [Apache Iceberg™](https://iceberg.apache.org/). ```bash cd iceberg-cpp mkdir build && cd build -cmake .. -DCMAKE_INSTALL_PREFIX=/tmp/iceberg -DICEBERG_BUILD_STATIC=ON -DICEBERG_BUILD_SHARED=ON +cmake .. -DCMAKE_INSTALL_PREFIX=/path/to/install -DICEBERG_BUILD_STATIC=ON -DICEBERG_BUILD_SHARED=ON +cmake --build . +cmake --install . +``` + +### Build and Install Iceberg Arrow Library + +#### Vendored Apache Arrow (default) + +```bash +cd iceberg-cpp/src/arrow +mkdir build && cd build +cmake .. -DCMAKE_INSTALL_PREFIX=/path/to/install -DICEBERG_ARROW=ON +cmake --build . +cmake --install . Review Comment: FYI: We don't need to create a build directory explicitly with recent CMake: ```suggestion cmake -S . -B build -DCMAKE_INSTALL_PREFIX=/path/to/install -DICEBERG_ARROW=ON cmake --build build cmake --install build ``` ########## 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) + + fetchcontent_makeavailable(Arrow) + + if(NOT TARGET Arrow::arrow_static) + add_library(Arrow::arrow_static INTERFACE IMPORTED) + target_link_libraries(Arrow::arrow_static INTERFACE arrow_static) + target_include_directories(Arrow::arrow_static INTERFACE ${arrow_SOURCE_DIR}/cpp/src + ${arrow_BINARY_DIR}/src) + endif() Review Comment: Should we move this to the below `if(arrow_SOURCE_DIR)` block? ########## cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -0,0 +1,139 @@ +# 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) + + fetchcontent_declare(Arrow Review Comment: FYI: Apache Arrow doesn't support `FetchContent` yet...: https://github.com/apache/arrow/issues/43668#issuecomment-2287736103 -- 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
