You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "wgtmac (via GitHub)" <gi...@apache.org> on 2024/12/07 16:29:49 UTC
[PR] Add iceberg_arrow library [iceberg-cpp]
wgtmac opened a new pull request, #6:
URL: https://github.com/apache/iceberg-cpp/pull/6
- Add a new library iceberg_arrow to leverage arrow-backed implementations (e.g. filesystem and parquet).
- Add Apache Arrow 18.1.0 as a third-party dependency to libiceberg_arrow.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1897934087
##########
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"
+ INSTALL_DESTINATION "${ICEBERG_INSTALL_CMAKEDIR}/Iceberg")
+
+write_basic_package_version_file(
+ "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config-version.cmake"
+ COMPATIBILITY SameMajorVersion)
+
+install(FILES "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config.cmake"
Review Comment:
We need to install the arrow static lib only when we have vendored Arrow in the build process. There are several cases:
1. If users run `cmake .. -DICEBERG_ARROW=OFF`, then we get rid of Arrow at all.
2. If users run `cmake .. -DICEBERG_ARROW=ON`, then we vendor and install static Arrow lib.
3. If users have installed Arrow already, then they simply run `cmake .. -DCMAKE_PREFIX_PATH=/path/to/arrow -DICEBERG_ARROW=ON ` to point to that location and `FetchContent_MakeAvaiable` is smart enough to locate it. In this case, we don't install Arrow lib and it is the preferred use case in production.
BTW, I have added components support in this PR as well. If users want to make sure iceberg_arrow is available, they can call `find_package(Iceberg REQUIRED CONFIG COMPONENTS Core Arrow)`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900339989
##########
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:
Thanks.
Hmm... `libiceberg_arrow.lib` for `libiceberg_arrow.dll` wasn't installed... I think that it's installed by default... I'll take a look at it later...
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2526460512
e.g. it seems we could integrate find_package and FetchContent more naturally and not rely on the pile of code Arrow uses: https://stackoverflow.com/questions/73621403/cmake-integrating-fetchcontent-with-find-package
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1899930371
##########
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:
No we can't. I have used the same name of `BuildUtils.cmake` and `ThirdpartyToolchain.cmake` in iceberg-cpp. This line is to resolve the naming conflict.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2566218953
Thanks @kou for your suggestion! Now the CMake implementation is greatly simplified. FYI, the installed directory looks like below:
```
├── include/
│ ├── iceberg/
│ │ ├── puffin.h
│ │ ├── table.h
│ │ └── demo_arrow.h
├── lib/
│ ├── libiceberg_vendored_arrow.a
│ ├── libiceberg_arrow.a
│ ├── libiceberg_core.a
│ ├── libiceberg_puffin.a
│ ├── cmake/
│ │ ├── IcebergConfig.cmake
│ │ ├── IcebergConfigVersion.cmake
│ │ ├── IcebergTargets.cmake
│ │ └── IcebergTargets-debug.cmake
└── share/
└── doc/
└── Iceberg/
├── LICENSE
└── NOTICE
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2580156079
Thanks @Xuanwo!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2543168693
Well, it seems that I cannot make the Windows CI happy. It complains unresolved Arrow symbols during linking. I think I need a Windows PC to debug it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900355716
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
We need to prepare this for each shared library. (We need `ICEBERG_CORE_EXPORT`/`ICEBERG_PUFFIN_EXPORT`/`ICEBERG_ARROW_EXPORT`.)
Because `libiceberg_arrow.dll` will use exported symbols in `libiceberg_core.dll`. In this case, `libiceberg_arrow.dll` uses `_declspec(dllexport)` for `libiceberg_arrow.dll` symbols and `_declspec(dllimport)` for `libiceberg_core.dll` symbols.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2558785835
> @zhjwpku Yes, I am already working on simplifying it by integrating `FetchContent_declare` with `FIND_PACKAGE_ARGS`. They are still in my local copy and I'm polishing them. It is pretty easy to import third-party libraries. The dirty part is to export targets and config files. I'm too busy these days so it may take a while to complete.
Sure, thanks. Take your time :)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1899827873
##########
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)
Review Comment:
Can we remove this?
##########
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:
Should we move this to the below `if(arrow_SOURCE_DIR)` block?
##########
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)
+ if(arrow_SOURCE_DIR)
+ set(ARROW_VENDORED TRUE)
+ install(TARGETS arrow_static
+ RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
Review Comment:
```suggestion
RUNTIME DESTINATION "${ICEBERG_INSTALL_BINDIR}"
```
##########
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:
Does `libiceberg_arrow.a` depend on `libiceberg_core.a` and `libiceberg_puffine.a` in the future?
If so, `Iceberg::iceberg_arrow_static` should have `Iceberg::iceberg_puffine_static` and `Iceberg::iceberg_core_static` dependencies.
##########
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)
+ if(arrow_SOURCE_DIR)
+ set(ARROW_VENDORED TRUE)
+ install(TARGETS arrow_static
+ RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
+ ARCHIVE DESTINATION "${ICEBERG_INSTALL_LIBDIR}"
+ LIBRARY DESTINATION "${ICEBERG_INSTALL_LIBDIR}")
+
+ get_target_property(ARROW_STATIC_LIB arrow_static OUTPUT_NAME)
+ set(ARROW_STATIC_LIB_NAME
+ "${CMAKE_STATIC_LIBRARY_PREFIX}${ARROW_STATIC_LIB}${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ list(APPEND ICEBERG_VENDOR_DEPENDENCIES
+ "Iceberg::arrow_vendored|${ARROW_STATIC_LIB_NAME}")
Review Comment:
We may be able to simplify this by using `install(TARGETS arrow_static EXPORT iceberg_arrow_targets ...)`.
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -210,9 +184,12 @@ function(ADD_ICEBERG_LIB LIB_NAME)
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
- endif()
+ string(TOLOWER ${LIB_NAME} LIB_NAME_LOWER_CASE)
+ string(REPLACE "_" "-" LIB_NAME_DASH_SEPARATED_LOWER_CASE ${LIB_NAME_LOWER_CASE})
+ install(EXPORT ${LIB_NAME}_targets
Review Comment:
How about using the same export name (something like `iceberg_targets`) for all targets?
If we use the same export name, we can use `include("${CMAKE_CURRENT_LIST_DIR}/iceberg-targets.cmake")` in `iceberg-config.cmake`.
##########
src/config.cmake.in:
##########
@@ -0,0 +1,123 @@
+# 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_VENDOR_DEPENDENCIES "@ICEBERG_VENDOR_DEPENDENCIES@")
+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)
+ file(GLOB_RECURSE _target_cmake_files "${CMAKE_CURRENT_LIST_DIR}/*-targets.cmake")
+ foreach(_target_cmake_file ${_target_cmake_files})
+ include(${_target_cmake_file})
+ endforeach()
+
+ foreach(comp ${components})
+ string(TOLOWER "${comp}" _comp_lower_case)
+ if(EXISTS "${CMAKE_CURRENT_LIST_DIR}/iceberg-${_comp_lower_case}-targets.cmake")
Review Comment:
How about using `TARGET` instead?
```suggestion
if(TARGET "Iceberg::iceberg_${_comp_lower_case}_shared" OR TARGET "Iceberg::iceberg_${_comp_lower_case}_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)
+
+ 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)
+ if(arrow_SOURCE_DIR)
+ set(ARROW_VENDORED TRUE)
+ install(TARGETS arrow_static
+ RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
+ ARCHIVE DESTINATION "${ICEBERG_INSTALL_LIBDIR}"
+ LIBRARY DESTINATION "${ICEBERG_INSTALL_LIBDIR}")
Review Comment:
Can we rename `libarrow.a` to `libiceberg_vendored_arrow.a` or something so that we can avoid file name conflict?
```cmake
set_target_properties(arrow_static PROPERTIES OUTPUT_NAME "iceberg_vendored_arrow")
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900166523
##########
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:
I removed this because I encountered an error on Windows. It complained that `iceberg_arrow.dll` had referenced `iceberg_arrow.lib` but actually it was named `iceberg_arrow_static.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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1908369593
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
@wgtmac I think let's go with the approach that is the most convenient at this point just to get going. We can re-evaluate later on once we actually do the implementation of Puffin. WDYT?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900344597
##########
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:
I got it. This happens because `libiceberg_arrow.dll` doesn't export any symbols.
Could you try this?
```diff
diff --git a/src/arrow/CMakeLists.txt b/src/arrow/CMakeLists.txt
index 87bc0ff..7c640fb 100644
--- a/src/arrow/CMakeLists.txt
+++ b/src/arrow/CMakeLists.txt
@@ -59,6 +59,8 @@ list(APPEND
add_iceberg_lib(iceberg_arrow
SOURCES
${ICEBERG_ARROW_SOURCES}
+ OUTPUTS
+ ICEBERG_ARROW_LIBRARIES
PRIVATE_INCLUDES
${ICEBERG_ARROW_INCLUDES}
SHARED_LINK_LIBS
@@ -69,6 +71,12 @@ add_iceberg_lib(iceberg_arrow
${ICEBERG_ARROW_STATIC_INSTALL_INTERFACE_LIBS}
SHARED_INSTALL_INTERFACE_LIBS
${ICEBERG_ARROW_SHARED_INSTALL_INTERFACE_LIBS})
+if(ICEBERG_BUILD_STATIC AND WIN32)
+ target_compile_definitions(iceberg_arrow_static PUBLIC ICEBERG_ARROW_STATIC)
+endif()
+foreach(LIB_TARGET ${ICEBERG_ARROW_LIBRARIES})
+ target_compile_definitions(${LIB_TARGET} PRIVATE ICEBERG_ARROW_EXPORTING)
+endforeach()
install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/demo_arrow.h"
DESTINATION "${ICEBERG_INSTALL_INCLUDEDIR}/iceberg/arrow")
diff --git a/src/arrow/demo_arrow.h b/src/arrow/demo_arrow.h
index 69517f6..8fe014a 100644
--- a/src/arrow/demo_arrow.h
+++ b/src/arrow/demo_arrow.h
@@ -21,9 +21,23 @@
#include <string>
+#ifdef _WIN32
+# ifdef ICEBERG_ARROW_STATIC
+# define ICEBERG_ARROW_EXPORT
+# else
+# ifdef ICEBERG_ARROW_EXPORTING
+# define ICEBERG_ARROW_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_ARROW_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_ARROW_EXPORT
+#endif
+
namespace iceberg::arrow {
-class DemoArrow {
+class ICEBERG_ARROW_EXPORT DemoArrow {
public:
std::string print() const;
};
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900363610
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
I'll try it on my Windows PC tonight. Thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2580091025
I think this PR is ready to merge. @Fokko @Xuanwo
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1901824596
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
I have tried to use `PRIVATE` and it failed: https://github.com/apache/iceberg-cpp/actions/runs/12584082126/job/35072964038
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900474985
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
I think having separate directories per library is reasonable.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1901405319
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
No, I added this to fix a linking error. I can try to remove this again to confirm.
Update: It failed at linking again: https://github.com/apache/iceberg-cpp/actions/runs/12591221855/job/35094013478?pr=6
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1897586676
##########
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"
+ INSTALL_DESTINATION "${ICEBERG_INSTALL_CMAKEDIR}/Iceberg")
+
+write_basic_package_version_file(
+ "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config-version.cmake"
+ COMPATIBILITY SameMajorVersion)
+
+install(FILES "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config.cmake"
Review Comment:
FYI, the install directory (with only static library built) looks like below:
```
/tmp/iceberg/
├── lib/
│ ├── libarrow.a
│ ├── libiceberg_arrow.a
│ ├── libiceberg_core.a
│ ├── libiceberg_puffin.a
│ └── cmake/
│ ├── Iceberg/
│ │ ├── iceberg-arrow-targets.cmake
│ │ ├── iceberg-arrow-targets-debug.cmake
│ │ ├── iceberg-core-targets.cmake
│ │ ├── iceberg-core-targets-debug.cmake
│ │ ├── iceberg-puffin-targets.cmake
│ │ ├── iceberg-puffin-targets-debug.cmake
│ │ ├── iceberg-config.cmake
│ │ └── iceberg-config-version.cmake
├── include/
│ └── iceberg/
│ ├── puffin.h
│ ├── table.h
│ └── demo_arrow.h
└── share/
└── doc/
└── iceberg/
├── LICENSE
└── NOTICE
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900361644
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
I think that it causes link errors. If `libiceberg_arrow.dll` uses a `libiceberg_core.dll` symbol, `libiceberg_arrow.dll` uses `__declspec(dllexport)`-ed `libiceberg_core.dll` symbols...
Can we add a simple function to `libiceberg_core.dll` and use it from `libiceberg_arrow.dll`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1874599187
##########
src/arrow/CMakeLists.txt:
##########
@@ -0,0 +1,66 @@
+# 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)
Review Comment:
Why do we need a directory called arrow, ISTM we do need some helper functions to wrapper the usage of arrow, but put it directly under src make people think arrow is part of iceberg's spec. I'd suggest we create a directory
called util and put arrow wrapper under that, what do you think?
##########
src/arrow/IcebergArrowConfig.cmake.in:
##########
@@ -0,0 +1,54 @@
+# 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::
+#
+# IcebergArrow_FOUND - true if IcebergArrow found on the system
+# ICEBERG_ARROW_VERSION - version of the found IcebergArrow
+#
+# This config sets the following targets in your project::
+#
+# IcebergArrow::iceberg_arrow_shared - for linked as shared library if shared library is built
+# IcebergArrow::iceberg_arrow_static - for linked as static library if static library is built
+
+@PACKAGE_INIT@
+
+set(ICEBERG_ARROW_VENDOR_DEPENDENCIES "@ICEBERG_ARROW_VENDOR_DEPENDENCIES@")
+set(ICEBERG_ARROW_SYSTEM_DEPENDENCIES "@ICEBERG_ARROW_SYSTEM_DEPENDENCIES@")
+
+include(CMakeFindDependencyMacro)
+find_dependency(IcebergCore)
+
+if (ICEBERG_BUILD_STATIC)
+ iceberg_find_dependencies("${ICEBERG_ARROW_SYSTEM_DEPENDENCIES}")
+endif()
+
+foreach(dependency ${ICEBERG_ARROW_VENDOR_DEPENDENCIES})
+ string(REPLACE "|" ";" dependency_pair ${dependency})
+ list(LENGTH dependency_pair dependency_pair_length)
+ if(NOT dependency_pair_length EQUAL 2)
+ message(FATAL_ERROR "Invalid vendor dependency: ${dependency}")
+ endif()
+ list(GET dependency_pair 0 target_name)
+ list(GET dependency_pair 1 static_lib_name)
+
+ add_library("${target_name}" STATIC IMPORTED)
+endforeach()
+
+include("${CMAKE_CURRENT_LIST_DIR}/IcebergArrowTargets.cmake")
+check_required_components(IcebergArrow)
+iceberg_show_details(IcebergArrow ICEBERG_ARROW)
Review Comment:
It's always good to include a empty line at the end of a file.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900615479
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,23 +202,29 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
- endif()
-
# Modify variable in calling scope
if(ARG_OUTPUTS)
set(${ARG_OUTPUTS}
${${ARG_OUTPUTS}}
PARENT_SCOPE)
endif()
endfunction()
+
+function(iceberg_set_export_definitions STATIC_TARGET LIB_TARGETS)
+ if(ICEBERG_BUILD_STATIC AND WIN32)
+ target_compile_definitions(${STATIC_TARGET} PUBLIC ICEBERG_STATIC)
+ endif()
+
+ foreach(LIB_TARGET ${LIB_TARGETS})
+ target_compile_definitions(${LIB_TARGET} PRIVATE ICEBERG_EXPORTING)
+ endforeach()
+endfunction()
Review Comment:
After applying generate_export_header, now the installed directory looks like:
```
/tmp/iceberg/
├── lib/
│ ├── libiceberg_vendored_arrow.a
│ ├── libiceberg.dylib
│ ├── libiceberg.a
│ ├── libiceberg_arrow.dylib
│ ├── libiceberg_arrow.a
│ ├── libiceberg_puffin.dylib
│ ├── libiceberg_puffin.a
│ └── cmake/
│ └── Iceberg/
│ ├── IcebergConfig.cmake
│ ├── IcebergConfigVersion.cmake
│ ├── IcebergTargets.cmake
│ └── IcebergTargets-debug.cmake
├── include/
│ └── iceberg/
│ ├── demo_table.h
│ ├── puffin.h
│ ├── table.h
│ ├── iceberg_export.h
│ └── arrow/
│ ├── demo_arrow.h
│ └── iceberg_arrow_export.h
│ └── puffin/
│ ├── demo_puffin.h
│ └── iceberg_puffin_export.h
└── share/
└── doc/
└── Iceberg/
├── LICENSE
└── NOTICE
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1901768298
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
https://github.com/apache/iceberg-cpp/actions/runs/12591221855/job/35094013478#step:3:422
```text
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: virtual __cdecl iceberg::Puffin::~Puffin(void)" (__imp_??1Puffin@iceberg@@UEAA@XZ) referenced in function "public: virtual __cdecl iceberg::puffin::DemoPuffin::~DemoPuffin(void)" (??1DemoPuffin@puffin@iceberg@@UEAA@XZ) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __cdecl iceberg::Puffin::Puffin(void)" (__imp_??0Puffin@iceberg@@QEAA@XZ) referenced in function "public: __cdecl iceberg::puffin::DemoPuffin::DemoPuffin(void)" (??0DemoPuffin@puffin@iceberg@@QEAA@XZ) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __cdecl iceberg::Puffin::Puffin(class iceberg::Puffin const &)" (__imp_??0Puffin@iceberg@@QEAA@AEBV01@@Z) referenced in function "public: __cdecl iceberg::puffin::DemoPuffin::DemoPuffin(class iceberg::puffin::DemoPuffin const &)" (??0DemoPuffin@puffin@iceberg@@QEAA@AEBV012@@Z) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: class iceberg::Puffin & __cdecl iceberg::Puffin::operator=(class iceberg::Puffin const &)" (__imp_??4Puffin@iceberg@@QEAAAEAV01@AEBV01@@Z) referenced in function "public: class iceberg::puffin::DemoPuffin & __cdecl iceberg::puffin::DemoPuffin::operator=(class iceberg::puffin::DemoPuffin const &)" (??4DemoPuffin@puffin@iceberg@@QEAAAEAV012@AEBV012@@Z) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
```
OK. It's an implementation problem. We should remove this line. (Or we should use `PRIVATE` not `PUBLIC`.)
`iceberg::Puffin` should exist in `libiceberg_core.dll` not `libiceberg_puffin.dll`. Can we define `iceberg::Puffine` in `libiceberg_core.dll` something like the following?
```diff
diff --git a/src/iceberg/demo_table.cc b/src/iceberg/demo_table.cc
index 9e46d7a..d960f1b 100644
--- a/src/iceberg/demo_table.cc
+++ b/src/iceberg/demo_table.cc
@@ -18,6 +18,7 @@
*/
#include "iceberg/demo_table.h"
+#include "iceberg/puffin.h"
namespace iceberg {
```
BTW, is it OK that `iceberg::Puffin` exists in `libiceberg_core.dll` not `libiceberg_puffin.dll`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900615479
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,23 +202,29 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
- endif()
-
# Modify variable in calling scope
if(ARG_OUTPUTS)
set(${ARG_OUTPUTS}
${${ARG_OUTPUTS}}
PARENT_SCOPE)
endif()
endfunction()
+
+function(iceberg_set_export_definitions STATIC_TARGET LIB_TARGETS)
+ if(ICEBERG_BUILD_STATIC AND WIN32)
+ target_compile_definitions(${STATIC_TARGET} PUBLIC ICEBERG_STATIC)
+ endif()
+
+ foreach(LIB_TARGET ${LIB_TARGETS})
+ target_compile_definitions(${LIB_TARGET} PRIVATE ICEBERG_EXPORTING)
+ endforeach()
+endfunction()
Review Comment:
I cannot make the Windows CI happy after using generate_export_header: https://github.com/apache/iceberg-cpp/actions/runs/12579586949/job/35060023773
```
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: virtual __cdecl iceberg::Puffin::~Puffin(void)" (__imp_??1Puffin@iceberg@@UEAA@XZ) referenced in function "public: virtual __cdecl iceberg::puffin::DemoPuffin::~DemoPuffin(void)" (??1DemoPuffin@puffin@iceberg@@UEAA@XZ) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __cdecl iceberg::Puffin::Puffin(void)" (__imp_??0Puffin@iceberg@@QEAA@XZ) referenced in function "public: __cdecl iceberg::puffin::DemoPuffin::DemoPuffin(void)" (??0DemoPuffin@puffin@iceberg@@QEAA@XZ) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __cdecl iceberg::Puffin::Puffin(class iceberg::Puffin const &)" (__imp_??0Puffin@iceberg@@QEAA@AEBV01@@Z) referenced in function "public: __cdecl iceberg::puffin::DemoPuffin::DemoPuffin(class iceberg::puffin::DemoPuffin const &)" (??0DemoPuffin@puffin@iceberg@@QEAA@AEBV012@@Z) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: class iceberg::Puffin & __cdecl iceberg::Puffin::operator=(class iceberg::Puffin const &)" (__imp_??4Puffin@iceberg@@QEAAAEAV01@AEBV01@@Z) referenced in function "public: class iceberg::puffin::DemoPuffin & __cdecl iceberg::puffin::DemoPuffin::operator=(class iceberg::puffin::DemoPuffin const &)" (??4DemoPuffin@puffin@iceberg@@QEAAAEAV012@AEBV012@@Z) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\Debug\iceberg_puffin.dll : fatal error LNK1120: 4 unresolved externals [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1901365957
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
+ if(BUILD_STATIC)
+ target_compile_definitions(${LIB_NAME}_static
+ PUBLIC ${LIB_NAME_UPPER}_STATIC_DEFINE)
+ endif()
+ elseif(BUILD_STATIC)
+ generate_export_header(${LIB_NAME}_static BASE_NAME ${LIB_NAME_UPPER})
Review Comment:
```suggestion
generate_export_header(${LIB_NAME}_static BASE_NAME ${LIB_NAME})
```
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
Can we remove this?
It seems that this is defined automatically: https://cmake.org/cmake/help/latest/prop_tgt/DEFINE_SYMBOL.html
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
Review Comment:
We can use `LIB_NAME` directly:
```suggestion
generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME})
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1904828898
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME})
+ # target_compile_definitions(${LIB_NAME}_shared PRIVATE ${LIB_NAME}_shared_EXPORTS)
Review Comment:
```suggestion
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900359799
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
Got it. What if we define `ICEBERG_EXPORTING` to all of `libiceberg_core.dll`, `libiceberg_arrow.dll` and `libiceberg_puffin.dll`? I think we can simply keep the same visibility to all libraries for now.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900359799
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
Got it. What if we define `ICEBERG_EXPORTING` to all of `libiceberg_core.dll`, `libiceberg_arrow.dll` and `libiceberg_puffin.dll`? I think we can simply keep the same visibility to all libraries for now because the `core` library provides the API and core logic while `puffin` and `arrow` provide concrete implementations of the `core` library.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900516441
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,23 +202,29 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
- endif()
-
# Modify variable in calling scope
if(ARG_OUTPUTS)
set(${ARG_OUTPUTS}
${${ARG_OUTPUTS}}
PARENT_SCOPE)
endif()
endfunction()
+
+function(iceberg_set_export_definitions STATIC_TARGET LIB_TARGETS)
+ if(ICEBERG_BUILD_STATIC AND WIN32)
+ target_compile_definitions(${STATIC_TARGET} PUBLIC ICEBERG_STATIC)
+ endif()
+
+ foreach(LIB_TARGET ${LIB_TARGETS})
+ target_compile_definitions(${LIB_TARGET} PRIVATE ICEBERG_EXPORTING)
+ endforeach()
+endfunction()
Review Comment:
Sorry. I didn't know that CMake provides a convenient feature for this: https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html#module:GenerateExportHeader
We may want to use this feature instead of creating `visibility.h` manually.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1897581776
##########
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:
@kou Sorry to bother you. I tried to add a minimal dependency on Apache Arrow to this project. The CI fails only on Windows:
```
CMake Error at build/_deps/arrow-src/cpp/src/arrow/CMakeLists.txt:587 (target_link_libraries):
Target "arrow_util_static" links to:
Boost::headers
CMake Error at build/_deps/arrow-src/cpp/src/arrow/CMakeLists.txt:804 (target_link_libraries):
Target "arrow_compute_static" links to:
Boost::headers
but the target was not found. Possible reasons include:
* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.
```
It seems that Boost is enabled due to missing native support of INT128 but not built via resolve_dependency.
```
-- Looking for _M_ARM64
-- Looking for _M_ARM64 - not found
-- Looking for __SIZEOF_INT128__
-- Looking for __SIZEOF_INT128__ - not found
-- Boost include dir:
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2558783070
@zhjwpku Yes, I am already working on simplifying it by integrating `FetchContent_declare` with `FIND_PACKAGE_ARGS`. They are still in my local copy and I'm polishing them. I'm too busy these days so it may take a while to complete.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1904813642
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
Thanks for the explanation! @kou
> is it OK that iceberg::Puffin exists in libiceberg_core.dll not libiceberg_puffin.dll?
I think this depends on how we implement the puffin interfaces. It seems to me there are at least two different interpretations of `libiceberg_puffin`:
1. `libiceberg_core` provides only the interface of `iceberg::Puffin` reader and writer. `libiceberg_puffin` provides a concrete implementation of `iceberg::Puffin` together with roaring bitmap serialization and deserialization. This means that `libiceberg_puffin` cannot be used alone and should always depend on `libiceberg_core`.
2. `libiceberg_puffin` provide the minimal building block of the puffin file format and does not depend on anything. In this case, `libiceberg_core` depends on `libiceberg_puffin` to provide a concrete implementation.
As the goal of `libiceberg_core` is to provide a lightweight library with minimal dependency to implement the core logic of Apache Iceberg and puffin is a required component (same as Parquet and Avro), I feel a little bit suspicious about the point of splitting an individual `libiceberg_puffin` library at this moment. As this was a request from @gaborkaszab, could you please chime in to provide more context about your perspective? Thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2527194639
> e.g. it seems we could integrate find_package and FetchContent more naturally and not rely on the pile of code Arrow uses
That would certainly be preferrable. The "Arrow way" of handling this has been a huge time sink.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2525419398
I have added a `iceberg-arrow` library to opt in an Arrow-backed implementation in the future. I follow a similar pattern of arrow-cpp to manage third-party libraries. Let me know what you think. @lidavidm @pitrou @raulcd @Fokko @Xuanwo @gaborkaszab
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900516103
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
> I'd propose to remove the current `api` folder and let each library manage its own header files.
+1
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900363610
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
I'll try it on my Windows PC tonight. Thanks!
EDIT: I have added a new function `DemoTable::name()` in demo_table.h and called it in demo_arrow.cc. It does not have linking error. @kou
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1897934087
##########
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"
+ INSTALL_DESTINATION "${ICEBERG_INSTALL_CMAKEDIR}/Iceberg")
+
+write_basic_package_version_file(
+ "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config-version.cmake"
+ COMPATIBILITY SameMajorVersion)
+
+install(FILES "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config.cmake"
Review Comment:
We need to install the arrow static lib only when we provided vendored arrow in the build process. There are several cases:
1. If users run `cmake .. -DICEBERG_ARROW=OFF`, then we get rid of Arrow at all.
2. If users run `cmake .. -DICEBERG_ARROW=ON`, then we vendor and install static Arrow lib.
3. If users have installed arrow already, then they simply run `cmake .. -DCMAKE_PREFIX_PATH=/path/to/arrow -DICEBERG_ARROW=ON ` to point to that location and `FetchContent_MakeAvaiable` is smart enough to locate it. In this case, we don't install Arrow lib and it is the preferred use case in production.
BTW, I have added components support in this PR as well. If users want to make sure iceberg_arrow is available, they can call `find_package(Iceberg REQUIRED CONFIG COMPONENTS Core Arrow)`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1901768298
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
https://github.com/apache/iceberg-cpp/actions/runs/12591221855/job/35094013478#step:3:422
```text
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: virtual __cdecl iceberg::Puffin::~Puffin(void)" (__imp_??1Puffin@iceberg@@UEAA@XZ) referenced in function "public: virtual __cdecl iceberg::puffin::DemoPuffin::~DemoPuffin(void)" (??1DemoPuffin@puffin@iceberg@@UEAA@XZ) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __cdecl iceberg::Puffin::Puffin(void)" (__imp_??0Puffin@iceberg@@QEAA@XZ) referenced in function "public: __cdecl iceberg::puffin::DemoPuffin::DemoPuffin(void)" (??0DemoPuffin@puffin@iceberg@@QEAA@XZ) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __cdecl iceberg::Puffin::Puffin(class iceberg::Puffin const &)" (__imp_??0Puffin@iceberg@@QEAA@AEBV01@@Z) referenced in function "public: __cdecl iceberg::puffin::DemoPuffin::DemoPuffin(class iceberg::puffin::DemoPuffin const &)" (??0DemoPuffin@puffin@iceberg@@QEAA@AEBV012@@Z) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
demo_puffin.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: class iceberg::Puffin & __cdecl iceberg::Puffin::operator=(class iceberg::Puffin const &)" (__imp_??4Puffin@iceberg@@QEAAAEAV01@AEBV01@@Z) referenced in function "public: class iceberg::puffin::DemoPuffin & __cdecl iceberg::puffin::DemoPuffin::operator=(class iceberg::puffin::DemoPuffin const &)" (??4DemoPuffin@puffin@iceberg@@QEAAAEAV012@AEBV012@@Z) [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
```
OK. It's an implementation problem. We should remove this line. (Or we should use `PRIVATE` not `PUBLIC`.)
`iceberg::Puffin` should exist in `libiceberg_core.dll` not `libiceberg_puffin.dll`. Can we define `iceberg::Puffin` in `libiceberg_core.dll` something like the following?
```diff
diff --git a/src/iceberg/demo_table.cc b/src/iceberg/demo_table.cc
index 9e46d7a..d960f1b 100644
--- a/src/iceberg/demo_table.cc
+++ b/src/iceberg/demo_table.cc
@@ -18,6 +18,7 @@
*/
#include "iceberg/demo_table.h"
+#include "iceberg/puffin.h"
namespace iceberg {
```
BTW, is it OK that `iceberg::Puffin` exists in `libiceberg_core.dll` not `libiceberg_puffin.dll`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900327030
##########
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:
https://github.com/apache/arrow/issues/45142
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900325544
##########
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:
It is installed, but to a different location (bin vs lib)
- C:/Users/runneradmin/AppData/Local/Temp/iceberg/bin/iceberg_arrow.dll
- C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_arrow.lib
If I use `${LIB_NAME_STATIC}`, it will be
- C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_arrow_static.lib
I don't know how to let `iceberg_arrow.dll` reference to `iceberg_arrow_static.lib` but not the default `iceberg_arrow.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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1899927285
##########
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:
Yes, I know that I cannot simply override Apache Arrow's third-party dependencies at this moment.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1904773431
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
If we include `iceberg/puffin.h` in `demo_table.cc`, `demo_table.cc` exports `iceberg::Puffin` in `puffin.h`. So `libiceberg_core.dll` has `iceberg::Puffin`.
BTW, is it OK that `iceberg::Puffin` exists in `libiceberg_core.dll` not `libiceberg_puffin.dll`? If `iceberg::Puffin` should exist in `libiceberg_puffin.dll`, `iceberg::Puffin` should be `iceberg::puffin::Puffin`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1874613441
##########
src/arrow/CMakeLists.txt:
##########
@@ -0,0 +1,66 @@
+# 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)
Review Comment:
This is related to the discussion: https://github.com/apache/iceberg-cpp/issues/2#issuecomment-2522063550. In short, we intend to make the core library as lite as possible to depend on a minimal set of third-party libraries. Arrow is one of the choose to provide concrete I/O and Parquet support.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2558769771
This seems real complicated, ISTM that `find_package and FetchContent` mentioned by @lidavidm should be an easy way, I tried that in #13, maybe you can check that.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1902525180
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
> 79bba23
Thanks.
We should not define `XXX_STATIC_DEFINE` for a shared library:
```diff
diff --git a/cmake_modules/BuildUtils.cmake b/cmake_modules/BuildUtils.cmake
index bc848bf..d1bc7ca 100644
--- a/cmake_modules/BuildUtils.cmake
+++ b/cmake_modules/BuildUtils.cmake
@@ -216,8 +216,7 @@ function(add_iceberg_lib LIB_NAME)
if(BUILD_SHARED)
generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
target_compile_definitions(${LIB_NAME}_shared
- PRIVATE ${LIB_NAME}_shared_EXPORTS
- PUBLIC ${LIB_NAME_UPPER}_STATIC_DEFINE)
+ PRIVATE ${LIB_NAME}_shared_EXPORTS)
if(BUILD_STATIC)
target_compile_definitions(${LIB_NAME}_static
PUBLIC ${LIB_NAME_UPPER}_STATIC_DEFINE)
```
> However I cannot make it work :(
Could you try this?
```diff
diff --git a/cmake_modules/BuildUtils.cmake b/cmake_modules/BuildUtils.cmake
index c915b3e..8a30753 100644
--- a/cmake_modules/BuildUtils.cmake
+++ b/cmake_modules/BuildUtils.cmake
@@ -214,7 +214,6 @@ function(add_iceberg_lib LIB_NAME)
# generate export header file
if(BUILD_SHARED)
generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME})
- target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
if(BUILD_STATIC)
string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
target_compile_definitions(${LIB_NAME}_static
diff --git a/src/iceberg/demo_table.cc b/src/iceberg/demo_table.cc
index 9e46d7a..d960f1b 100644
--- a/src/iceberg/demo_table.cc
+++ b/src/iceberg/demo_table.cc
@@ -18,6 +18,7 @@
*/
#include "iceberg/demo_table.h"
+#include "iceberg/puffin.h"
namespace iceberg {
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1902260652
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
https://github.com/apache/iceberg-cpp/actions/runs/12584082126/job/35072964038#step:3:410
```text
LINK : fatal error LNK1104: cannot open file '..\Debug\iceberg.lib' [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\arrow\iceberg_arrow_shared.vcxproj]
Building Custom Rule D:/a/iceberg-cpp/iceberg-cpp/src/iceberg/CMakeLists.txt
demo_table.cc
iceberg_static.vcxproj -> D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\Debug\iceberg_static.lib
Building Custom Rule D:/a/iceberg-cpp/iceberg-cpp/src/iceberg/arrow/CMakeLists.txt
demo_arrow.cc
iceberg_arrow_static.vcxproj -> D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\arrow\Debug\iceberg_arrow_static.lib
Building Custom Rule D:/a/iceberg-cpp/iceberg-cpp/src/iceberg/puffin/CMakeLists.txt
demo_puffin.cc
LINK : fatal error LNK1104: cannot open file '..\Debug\iceberg.lib' [D:\a\iceberg-cpp\iceberg-cpp\build\src\iceberg\puffin\iceberg_puffin_shared.vcxproj]
```
I think that it's a different problem. Could you share the URL of the commit?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1904828726
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
Thanks for explaining the context.
How about discussing it as a follow-up task?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900365823
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
I believe this is relevant to how we manage the public headers from different libraries. As per previous discussion, we use a separate `api` folder holds all public header files from all libraries (i.e. core, puffin, arrow, etc.). IIUC, the current approach makes it difficult to manage the visibility of the header file across libraries. I'd propose to remove the current `api` folder and let each library manage its own header files. WDYT? @kou @raulcd @zhjwpku @gaborkaszab @lidavidm @pitrou
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1904261495
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
Thanks! These suggestions seem to work. Why including `iceberg/puffin.h` fixes this?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900516042
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
> I have added a new function `DemoTable::name()` in demo_table.h and called it in demo_arrow.cc. It does not have linking error.
Hmm. It may have a real world problem because `libiceberg_arrow.lib` is always used with `libiceberg_core.lib`...
FYI: If we use the same `ICEBERG_EXPORTING` for all libraries, `libiceberg_arrow.lib` also exports symbols in `libiceberg_core.dll` but `libiceberg_arrow.dll` doesn't have them:
```console
$ nm iceberg_arrow.lib | grep Demo
0000000000000000 T ??0DemoTable@iceberg@@QEAA@AEBV01@@Z
0000000000000000 I __imp_??0DemoTable@iceberg@@QEAA@AEBV01@@Z
0000000000000000 T ??0DemoTable@iceberg@@QEAA@XZ
0000000000000000 I __imp_??0DemoTable@iceberg@@QEAA@XZ
0000000000000000 T ??1DemoTable@iceberg@@UEAA@XZ
0000000000000000 I __imp_??1DemoTable@iceberg@@UEAA@XZ
0000000000000000 T ??4DemoArrow@arrow@iceberg@@QEAAAEAV012@$$QEAV012@@Z
0000000000000000 I __imp_??4DemoArrow@arrow@iceberg@@QEAAAEAV012@$$QEAV012@@Z
0000000000000000 T ??4DemoArrow@arrow@iceberg@@QEAAAEAV012@AEBV012@@Z
0000000000000000 I __imp_??4DemoArrow@arrow@iceberg@@QEAAAEAV012@AEBV012@@Z
0000000000000000 T ??4DemoTable@iceberg@@QEAAAEAV01@AEBV01@@Z
0000000000000000 I __imp_??4DemoTable@iceberg@@QEAAAEAV01@AEBV01@@Z
0000000000000000 I __imp_??_7DemoTable@iceberg@@6B@
0000000000000000 T ?name@DemoArrow@arrow@iceberg@@QEBA?AV?$basic_string_view@DU?$char_traits@D@std@@@std@@XZ
0000000000000000 I __imp_?name@DemoArrow@arrow@iceberg@@QEBA?AV?$basic_string_view@DU?$char_traits@D@std@@@std@@XZ
0000000000000000 T ?print@DemoArrow@arrow@iceberg@@QEBA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@XZ
0000000000000000 I __imp_?print@DemoArrow@arrow@iceberg@@QEBA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@XZ
```
If we use separated `_EXPORTING` macro for each library, `DemoTable` symbols aren't exported by `libiceberg_arrow.dll`:
```console
$ nm iceberg_arrow.lib | grep Demo
0000000000000000 T ??4DemoArrow@arrow@iceberg@@QEAAAEAV012@$$QEAV012@@Z
0000000000000000 I __imp_??4DemoArrow@arrow@iceberg@@QEAAAEAV012@$$QEAV012@@Z
0000000000000000 T ??4DemoArrow@arrow@iceberg@@QEAAAEAV012@AEBV012@@Z
0000000000000000 I __imp_??4DemoArrow@arrow@iceberg@@QEAAAEAV012@AEBV012@@Z
0000000000000000 T ?name@DemoArrow@arrow@iceberg@@QEBA?AV?$basic_string_view@DU?$char_traits@D@std@@@std@@XZ
0000000000000000 I __imp_?name@DemoArrow@arrow@iceberg@@QEBA?AV?$basic_string_view@DU?$char_traits@D@std@@@std@@XZ
0000000000000000 T ?print@DemoArrow@arrow@iceberg@@QEBA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@XZ
0000000000000000 I __imp_?print@DemoArrow@arrow@iceberg@@QEBA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@XZ
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1899929235
##########
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:
I saw that both `<lowercasePackageName>-config.cmake` and `<PackageName>Config.cmake` are acceptable. It seems that the official CMake documentation uses a lot of the former style as examples and some package managers (e.g. Conan) also use the former one. Therefore I follow the same pattern.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1908414237
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
Sounds good! Let's keep the puffin library as is and revisit its design when you start the implementation.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1904829176
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
I think that we can merge this now.
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
Thanks for explaining the context.
How about discussing it as a follow-up task if it's needed?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1899970387
##########
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:
Finally, I chose to follow what Apache Arrow does...
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900309004
##########
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:
The error log of it is https://github.com/apache/iceberg-cpp/actions/runs/12505613514/job/34889235775#step:4:33 , right?
```text
The imported target "Iceberg::iceberg_arrow_shared" references the file
"C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_arrow.lib"
but this file does not exist.
```
It seems that `.lib` for shared library isn't installed. It's strange. `.lib` for shared library is installed by default.
Could you try reverting this?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900328737
##########
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:
I have reverted ${LIB_NAME_STATIC}: https://github.com/apache/iceberg-cpp/actions/runs/12567791967/job/35034664088?pr=6
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900360320
##########
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:
@kou Thanks for your help on this! Happy new year :)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900361184
##########
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:
Happy new year!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1899965461
##########
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:
Yes, I think both `libiceberg_arrow` and `libiceberg_puffin` should depend on `libiceberg_core` eventually.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1902490200
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
> I think that it's a different problem. Could you share the URL of the commit?
https://github.com/apache/iceberg-cpp/pull/6/commits/79bba2350caf7988a3ca82767804c5cb046e5f47
> If we use PUBLIC here, all of libiceberg*.dll use __declspec(dllexport) for all symbols.
Yeah, I understand that and agree we should not use `PUBLIC` here. However I cannot make it work :(
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1897591862
##########
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"
+ INSTALL_DESTINATION "${ICEBERG_INSTALL_CMAKEDIR}/Iceberg")
+
+write_basic_package_version_file(
+ "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config-version.cmake"
+ COMPATIBILITY SameMajorVersion)
+
+install(FILES "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config.cmake"
Review Comment:
It seems the demo header shouldn't be installed?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1897865046
##########
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"
+ INSTALL_DESTINATION "${ICEBERG_INSTALL_CMAKEDIR}/Iceberg")
+
+write_basic_package_version_file(
+ "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config-version.cmake"
+ COMPATIBILITY SameMajorVersion)
+
+install(FILES "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config.cmake"
Review Comment:
Why do we need to install arrow static lib? How do we suppose a user to use it while arrow headers is not installed? dynamic lib might be useful at linking stage, what the static lib is for? I might be too dumb to follow you 😂
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1897581776
##########
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:
@kou Sorry to bother you. I tried to add a minimal dependency on Apache Arrow to this project. The CI fails only on Windows:
```
CMake Error at build/_deps/arrow-src/cpp/src/arrow/CMakeLists.txt:587 (target_link_libraries):
Target "arrow_util_static" links to:
Boost::headers
CMake Error at build/_deps/arrow-src/cpp/src/arrow/CMakeLists.txt:804 (target_link_libraries):
Target "arrow_compute_static" links to:
Boost::headers
but the target was not found. Possible reasons include:
* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.
```
It seems that Boost is enabled due to missing native support of INT128 but not built via resolve_dependency.
```
-- Looking for _M_ARM64
-- Looking for _M_ARM64 - not found
-- Looking for __SIZEOF_INT128__
-- Looking for __SIZEOF_INT128__ - not found
-- Boost include dir:
```
Is there any setting that can work around this? I'll dig into it on my Windows PC after work.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1897676702
##########
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"
+ INSTALL_DESTINATION "${ICEBERG_INSTALL_CMAKEDIR}/Iceberg")
+
+write_basic_package_version_file(
+ "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config-version.cmake"
+ COMPATIBILITY SameMajorVersion)
+
+install(FILES "${CMAKE_CURRENT_BINARY_DIR}/iceberg-config.cmake"
Review Comment:
The demo_arrow.h is included by the example which consumes the iceberg library via installed location. I add `demo_` prefix by purpose to show this and it will be removed once we have more source files.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1902261165
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
If we use `PUBLIC` here, all of `libiceberg*.dll` use `__declspec(dllexport)` for all symbols. It's the same situation as we use only `ICEBERG_EXPORTING`: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900516042
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2563325643
This is ready for review now.
Thanks @lidavidm and @zhjwpku for approval!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1897986191
##########
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:
OK, I got it compiled. The default values of Arrow `define_string_option are somehow missing so I have to explicitly set ARROW_DEPENDENCY_SOURCE to AUTO/BUNDLED to build Boost. Similar for ARROW_SIMD_LEVEL and ARROW_RUNTIME_SIMD_LEVEL.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1901029911
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
I have removed the `api` folder and move the header files to each library instead.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900615479
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,23 +202,29 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
- endif()
-
# Modify variable in calling scope
if(ARG_OUTPUTS)
set(${ARG_OUTPUTS}
${${ARG_OUTPUTS}}
PARENT_SCOPE)
endif()
endfunction()
+
+function(iceberg_set_export_definitions STATIC_TARGET LIB_TARGETS)
+ if(ICEBERG_BUILD_STATIC AND WIN32)
+ target_compile_definitions(${STATIC_TARGET} PUBLIC ICEBERG_STATIC)
+ endif()
+
+ foreach(LIB_TARGET ${LIB_TARGETS})
+ target_compile_definitions(${LIB_TARGET} PRIVATE ICEBERG_EXPORTING)
+ endforeach()
+endfunction()
Review Comment:
I have switched to `generate_export_header` and CIs are all green.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900350764
##########
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:
Ah, it seems working.
```
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_vendored_arrow.lib
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/include/iceberg
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/include/iceberg/puffin.h
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/include/iceberg/table.h
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/bin/iceberg_core.dll
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_core_static.lib
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/bin/iceberg_puffin.dll
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_puffin_static.lib
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_arrow.lib
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/bin/iceberg_arrow.dll
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_arrow_static.lib
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/include/iceberg/arrow/demo_arrow.h
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/cmake/Iceberg/IcebergConfig.cmake
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/cmake/Iceberg/IcebergConfigVersion.cmake
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/cmake/Iceberg/IcebergTargets.cmake
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/cmake/Iceberg/IcebergTargets-debug.cmake
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/share/doc/Iceberg/LICENSE
-- Installing: C:/Users/runneradmin/AppData/Local/Temp/iceberg/share/doc/Iceberg/NOTICE
...
CMake Error at C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/cmake/Iceberg/IcebergTargets.cmake:130 (message):
The imported target "Iceberg::iceberg_core_shared" references the file
-- Configuring incomplete, errors occurred!
"C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_core.lib"
but this file does not exist. Possible reasons include:
* The file was deleted, renamed, or moved to another location.
* An install or uninstall procedure did not complete successfully.
* The installation package was faulty and contained
"C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/cmake/Iceberg/IcebergTargets.cmake"
but not all the files it references.
Call Stack (most recent call first):
C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/cmake/Iceberg/IcebergConfig.cmake:99 (include)
CMakeLists.txt:25 (find_package)
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1901405319
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
No, I added this to fix a linking error. I can try to remove this again to confirm.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900327762
##########
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:
> It is installed, but to a different location (bin vs lib)
>
> * C:/Users/runneradmin/AppData/Local/Temp/iceberg/bin/iceberg_arrow.dll
>
> * C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_arrow.lib
This is expected.
Users use `lib/iceberg_arrow.lib` when they build their program.
Users use `bin/iceberg_arrow.dll` when they run their program.
> If I use `${LIB_NAME_STATIC}`, it will be
>
> * C:/Users/runneradmin/AppData/Local/Temp/iceberg/lib/iceberg_arrow_static.lib
>
>
> I don't know how to let `iceberg_arrow.dll` reference to `iceberg_arrow_static.lib` but not the default `iceberg_arrow.lib`
If users use static library, users don't use `iceberg_arrow.dll`. Users use only `iceberg_arrow_static.lib` for static linking.
If users use shared library, users use `lib/iceberg_arrow.lib` on building and `bin/iceberg_arrow.dll` on running.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900328737
##########
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:
I have reverted ${LIB_NAME_STATIC}. Let's see.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1900365823
##########
api/iceberg/visibility.h:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#if defined(_WIN32) || defined(__CYGWIN__)
+# ifdef ICEBERG_STATIC
+# define ICEBERG_EXPORT
+# else
+# ifdef ICEBERG_EXPORTING
+# define ICEBERG_EXPORT __declspec(dllexport)
+# else
+# define ICEBERG_EXPORT __declspec(dllimport)
+# endif
+# endif
+#else
+# define ICEBERG_EXPORT
+#endif
Review Comment:
I believe this is relevant to how we manage the public headers from different iceberg libraries. As per previous discussion, we use a separate `api` folder holds all public header files from all libraries (i.e. core, puffin, arrow, etc.). IIUC, the current approach makes it difficult to manage the visibility of the header file across libraries. I'd propose to remove the current `api` folder and let each library manage its own header files. WDYT? @kou @raulcd @zhjwpku @gaborkaszab @lidavidm @pitrou
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#discussion_r1904830435
##########
cmake_modules/BuildUtils.cmake:
##########
@@ -201,17 +202,26 @@ function(ADD_ICEBERG_LIB LIB_NAME)
PUBLIC "$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}
- EXPORT ${LIB_NAME}_targets
+ install(TARGETS ${LIB_NAME}_static
+ EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
LIBRARY DESTINATION ${INSTALL_LIBRARY_DIR}
RUNTIME DESTINATION ${INSTALL_RUNTIME_DIR}
INCLUDES
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif()
- if(ARG_CMAKE_PACKAGE_NAME)
- iceberg_install_cmake_package(${ARG_CMAKE_PACKAGE_NAME} ${LIB_NAME}_targets)
+ # generate export header file
+ string(TOUPPER ${LIB_NAME} LIB_NAME_UPPER)
+ if(BUILD_SHARED)
+ generate_export_header(${LIB_NAME}_shared BASE_NAME ${LIB_NAME_UPPER})
+ target_compile_definitions(${LIB_NAME}_shared PUBLIC ${LIB_NAME}_shared_EXPORTS)
Review Comment:
Thanks for your help!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add iceberg_arrow library [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #6:
URL: https://github.com/apache/iceberg-cpp/pull/6#issuecomment-2530677420
That makes sense! Let me spend some time to simplify it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org