You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/06/26 01:15:44 UTC
[GitHub] [arrow] kou commented on a diff in pull request #13404: ARROW-16510: [R] Add bindings for GCS filesystem
kou commented on code in PR #13404:
URL: https://github.com/apache/arrow/pull/13404#discussion_r906740843
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4075,11 +4105,39 @@ macro(build_google_cloud_cpp_storage)
CURL::libcurl
Threads::Threads
OpenSSL::SSL
- OpenSSL::Crypto)
+ OpenSSL::Crypto
+ ZLIB::ZLIB)
add_dependencies(google-cloud-cpp::storage google_cloud_cpp_ep)
- list(APPEND ARROW_BUNDLED_STATIC_LIBS google-cloud-cpp::storage
+ list(APPEND
+ ARROW_BUNDLED_STATIC_LIBS
+ google-cloud-cpp::rest-internal
+ google-cloud-cpp::storage
Review Comment:
This may not be a problem but could you put `google-cloud-cpp::rest-internal` after `google-cloud-cpp::storage` because `google-cloud-cpp::storage` depends on `google-cloud-cpp::rest-internal`?
```suggestion
google-cloud-cpp::storage
google-cloud-cpp::rest-internal
```
##########
r/configure:
##########
@@ -229,44 +229,48 @@ if [ $? -eq 0 ]; then
# Check for features
LIB_DIR=`echo $PKG_DIRS | sed -e 's/^-L//'`
ARROW_OPTS_CMAKE="$LIB_DIR/cmake/arrow/ArrowOptions.cmake"
- # Check for Parquet
- grep -i 'set(ARROW_PARQUET "ON")' $ARROW_OPTS_CMAKE >/dev/null 2>&1
- if [ $? -eq 0 ]; then
+
+ arrow_built_with() {
+ # Function to check cmake options for features
+ grep -i 'set('"$1"' "ON")' $ARROW_OPTS_CMAKE >/dev/null 2>&1
Review Comment:
FYI: We can use `grep -q ...` instead of `grep ... >/dev/null 2>&1`.
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4046,25 +4054,47 @@ macro(build_google_cloud_cpp_storage)
"${GOOGLE_CLOUD_CPP_STATIC_LIBRARY_COMMON}"
INTERFACE_INCLUDE_DIRECTORIES
"${GOOGLE_CLOUD_CPP_INCLUDE_DIR}")
+ # Refer to https://github.com/googleapis/google-cloud-cpp/blob/main/google/cloud/google_cloud_cpp_common.cmake
+ # (subsitute `main` for the SHA of the version we use)
+ # Version 1.39.0 is at a different place (they refactored after):
+ # https://github.com/googleapis/google-cloud-cpp/blob/29e5af8ca9b26cec62106d189b50549f4dc1c598/google/cloud/CMakeLists.txt#L146-L155
set_property(TARGET google-cloud-cpp::common
PROPERTY INTERFACE_LINK_LIBRARIES
- absl::any
- absl::flat_hash_map
+ absl::base
absl::memory
absl::optional
+ absl::span
absl::time
+ absl::variant
Threads::Threads
OpenSSL::Crypto)
+ add_library(google-cloud-cpp::rest-internal STATIC IMPORTED)
+ set_target_properties(google-cloud-cpp::rest-internal
+ PROPERTIES IMPORTED_LOCATION
+ "${GOOGLE_CLOUD_CPP_STATIC_LIBRARY_REST_INTERNAL}"
+ INTERFACE_INCLUDE_DIRECTORIES
+ "${GOOGLE_CLOUD_CPP_INCLUDE_DIR}")
+ set_property(TARGET google-cloud-cpp::rest-internal
+ PROPERTY INTERFACE_LINK_LIBRARIES
+ absl::span
+ google-cloud-cpp::common
+ CURL::libcurl
+ nlohmann_json::nlohmann_json
+ OpenSSL::SSL
+ OpenSSL::Crypto)
+
add_library(google-cloud-cpp::storage STATIC IMPORTED)
set_target_properties(google-cloud-cpp::storage
PROPERTIES IMPORTED_LOCATION
"${GOOGLE_CLOUD_CPP_STATIC_LIBRARY_STORAGE}"
INTERFACE_INCLUDE_DIRECTORIES
"${GOOGLE_CLOUD_CPP_INCLUDE_DIR}")
+ # Update this from https://github.com/googleapis/google-cloud-cpp/blob/main/google/cloud/storage/google_cloud_cpp_storage.cmake
set_property(TARGET google-cloud-cpp::storage
PROPERTY INTERFACE_LINK_LIBRARIES
google-cloud-cpp::common
Review Comment:
We can remove this because `google-cloud-cpp-rest-internal` depends on `google-cloud-cpp::common`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org