lidavidm commented on a change in pull request #11889:
URL: https://github.com/apache/arrow/pull/11889#discussion_r820987257
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -3403,15 +3403,32 @@ macro(build_absl_once)
set_property(TARGET absl::throw_delegate
PROPERTY INTERFACE_LINK_LIBRARIES absl::config
absl::raw_logging_internal)
- set_property(TARGET absl::time
- PROPERTY INTERFACE_LINK_LIBRARIES
- absl::base
- absl::civil_time
- absl::core_headers
- absl::int128
- absl::raw_logging_internal
- absl::strings
- absl::time_zone)
+ if(APPLE)
+ # This is due to upstream absl::cctz issue
+ # https://github.com/abseil/abseil-cpp/issues/283
+ find_library(CoreFoundation CoreFoundation)
+ set_property(TARGET absl::time
+ PROPERTY INTERFACE_LINK_LIBRARIES
+ absl::base
+ absl::civil_time
+ absl::core_headers
+ absl::int128
+ absl::raw_logging_internal
+ absl::strings
+ absl::time_zone
+ ${CoreFoundation})
+ else()
+ find_library(CoreFoundation CoreFoundation)
Review comment:
This won't work on non-Apple platforms will it?
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -3403,15 +3403,32 @@ macro(build_absl_once)
set_property(TARGET absl::throw_delegate
PROPERTY INTERFACE_LINK_LIBRARIES absl::config
absl::raw_logging_internal)
- set_property(TARGET absl::time
- PROPERTY INTERFACE_LINK_LIBRARIES
- absl::base
- absl::civil_time
- absl::core_headers
- absl::int128
- absl::raw_logging_internal
- absl::strings
- absl::time_zone)
+ if(APPLE)
+ # This is due to upstream absl::cctz issue
+ # https://github.com/abseil/abseil-cpp/issues/283
+ find_library(CoreFoundation CoreFoundation)
+ set_property(TARGET absl::time
Review comment:
Can we append to the property instead of having to list out all the
dependencies?
##########
File path: cpp/examples/arrow/CMakeLists.txt
##########
@@ -42,8 +42,12 @@ if(ARROW_FLIGHT)
message(FATAL_ERROR "Statically built gRPC requires ARROW_BUILD_STATIC=ON")
else()
set(FLIGHT_EXAMPLES_LINK_LIBS arrow_flight_static)
- set(GRPC_REFLECTION_LINK_LIBS -Wl,--whole-archive gRPC::grpc++_reflection
- -Wl,--no-whole-archive)
+ if(APPLE)
+ set(GRPC_REFLECTION_LINK_LIBS -Wl,-force_load gRPC::grpc++_reflection
-Wl)
Review comment:
Shouldn't this end with `-Wl,--no-force-load` or something?
Related, does it take `-force-load` or `--force-load`?
##########
File path: cpp/cmake_modules/BuildUtils.cmake
##########
@@ -240,7 +240,9 @@ function(ADD_ARROW_LIB LIB_NAME)
DEPENDENCIES
SHARED_INSTALL_INTERFACE_LIBS
STATIC_INSTALL_INTERFACE_LIBS
- OUTPUT_PATH)
+ OUTPUT_PATH
+ SHARED_INSTALL_EXTRA_TARGETS
+ STATIC_INSTALL_EXTRA_TARGETS)
Review comment:
I'm still confused here, now the arguments were added back but we don't
use them?
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -3403,15 +3403,32 @@ macro(build_absl_once)
set_property(TARGET absl::throw_delegate
PROPERTY INTERFACE_LINK_LIBRARIES absl::config
absl::raw_logging_internal)
- set_property(TARGET absl::time
- PROPERTY INTERFACE_LINK_LIBRARIES
- absl::base
- absl::civil_time
- absl::core_headers
- absl::int128
- absl::raw_logging_internal
- absl::strings
- absl::time_zone)
+ if(APPLE)
+ # This is due to upstream absl::cctz issue
+ # https://github.com/abseil/abseil-cpp/issues/283
+ find_library(CoreFoundation CoreFoundation)
+ set_property(TARGET absl::time
+ PROPERTY INTERFACE_LINK_LIBRARIES
+ absl::base
+ absl::civil_time
+ absl::core_headers
+ absl::int128
+ absl::raw_logging_internal
+ absl::strings
+ absl::time_zone
+ ${CoreFoundation})
+ else()
+ find_library(CoreFoundation CoreFoundation)
Review comment:
Do we need this branch at all? Presumably absl::time's deps are correct
on non-Apple platforms and we don't need to overwrite them?
##########
File path: cpp/src/arrow/CMakeLists.txt
##########
@@ -533,8 +533,8 @@ else()
set(ARROW_BUILD_BUNDLED_DEPENDENCIES FALSE)
endif()
-if(ARROW_BUILD_BUNDLED_DEPENDENCIES)
- string(APPEND ARROW_PC_LIBS_PRIVATE " -larrow_bundled_dependencies")
+if(NOT ARROW_BUILD_BUNDLED_DEPENDENCIES)
Review comment:
Is this back to `NOT`? I feel like it flip-flops on every commit…was
there a rebase error or something? It might be easier to squash current commits
if 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
