Copilot commented on code in PR #1021:
URL: https://github.com/apache/iceberg-cpp/pull/1021#discussion_r4280304247
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -233,20 +233,18 @@ function(resolve_arrow_dependency)
"https://archive.apache.org/dist/thrift/0.22.0/thrift-0.22.0.tar.gz";)
endif()
- fetchcontent_declare(VendoredArrow
+ 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)
- fetchcontent_makeavailable(VendoredArrow)
+ fetchcontent_makeavailable(Arrow)
- if(vendoredarrow_SOURCE_DIR)
+ if(arrow_SOURCE_DIR)
Review Comment:
CMake variables are case-sensitive, and FetchContent typically exposes
`<Name>_SOURCE_DIR` / `<Name>_BINARY_DIR` using the declared content name
(here: `Arrow_SOURCE_DIR` and `Arrow_BINARY_DIR`). Using `arrow_SOURCE_DIR` /
`arrow_BINARY_DIR` can leave these empty, causing the vendored-Arrow branch to
be skipped and the include directories to point to non-existent paths
(impacting the Parquet encryption include fix). Recommend switching to
`Arrow_SOURCE_DIR` / `Arrow_BINARY_DIR`, or retrieving them via
`FetchContent_GetProperties(Arrow)` and checking `Arrow_POPULATED`.
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -270,16 +268,16 @@ function(resolve_arrow_dependency)
add_library(Arrow::arrow_static INTERFACE IMPORTED)
target_link_libraries(Arrow::arrow_static INTERFACE arrow_static)
target_include_directories(Arrow::arrow_static
- INTERFACE ${vendoredarrow_BINARY_DIR}/src
- ${vendoredarrow_SOURCE_DIR}/cpp/src)
+ INTERFACE ${arrow_BINARY_DIR}/src
+ ${arrow_SOURCE_DIR}/cpp/src)
endif()
if(NOT TARGET Parquet::parquet_static)
add_library(Parquet::parquet_static INTERFACE IMPORTED)
target_link_libraries(Parquet::parquet_static INTERFACE parquet_static)
target_include_directories(Parquet::parquet_static
- INTERFACE ${vendoredarrow_BINARY_DIR}/src
- ${vendoredarrow_SOURCE_DIR}/cpp/src)
+ INTERFACE ${arrow_BINARY_DIR}/src
+ ${arrow_SOURCE_DIR}/cpp/src)
Review Comment:
CMake variables are case-sensitive, and FetchContent typically exposes
`<Name>_SOURCE_DIR` / `<Name>_BINARY_DIR` using the declared content name
(here: `Arrow_SOURCE_DIR` and `Arrow_BINARY_DIR`). Using `arrow_SOURCE_DIR` /
`arrow_BINARY_DIR` can leave these empty, causing the vendored-Arrow branch to
be skipped and the include directories to point to non-existent paths
(impacting the Parquet encryption include fix). Recommend switching to
`Arrow_SOURCE_DIR` / `Arrow_BINARY_DIR`, or retrieving them via
`FetchContent_GetProperties(Arrow)` and checking `Arrow_POPULATED`.
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -270,16 +268,16 @@ function(resolve_arrow_dependency)
add_library(Arrow::arrow_static INTERFACE IMPORTED)
target_link_libraries(Arrow::arrow_static INTERFACE arrow_static)
target_include_directories(Arrow::arrow_static
- INTERFACE ${vendoredarrow_BINARY_DIR}/src
- ${vendoredarrow_SOURCE_DIR}/cpp/src)
+ INTERFACE ${arrow_BINARY_DIR}/src
+ ${arrow_SOURCE_DIR}/cpp/src)
Review Comment:
CMake variables are case-sensitive, and FetchContent typically exposes
`<Name>_SOURCE_DIR` / `<Name>_BINARY_DIR` using the declared content name
(here: `Arrow_SOURCE_DIR` and `Arrow_BINARY_DIR`). Using `arrow_SOURCE_DIR` /
`arrow_BINARY_DIR` can leave these empty, causing the vendored-Arrow branch to
be skipped and the include directories to point to non-existent paths
(impacting the Parquet encryption include fix). Recommend switching to
`Arrow_SOURCE_DIR` / `Arrow_BINARY_DIR`, or retrieving them via
`FetchContent_GetProperties(Arrow)` and checking `Arrow_POPULATED`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
