Copilot commented on code in PR #48179:
URL: https://github.com/apache/arrow/pull/48179#discussion_r2544822638
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2799,37 +2799,64 @@ if(ARROW_WITH_ZSTD)
endif()
# ----------------------------------------------------------------------
-# RE2 (required for Gandiva)
+# RE2 (required for Gandiva and gRPC)
-macro(build_re2)
- message(STATUS "Building RE2 from source")
- set(RE2_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/re2_ep-install")
- set(RE2_STATIC_LIB
-
"${RE2_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}re2${CMAKE_STATIC_LIBRARY_SUFFIX}")
+function(build_re2)
+ list(APPEND CMAKE_MESSAGE_INDENT "RE2: ")
+ message(STATUS "Building RE2 from source using FetchContent")
+ set(RE2_VENDORED
+ TRUE
+ PARENT_SCOPE)
+ set(RE2_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/re2_fc-install")
+ set(RE2_PREFIX
+ "${RE2_PREFIX}"
+ PARENT_SCOPE)
- set(RE2_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS}
"-DCMAKE_INSTALL_PREFIX=${RE2_PREFIX}")
+ fetchcontent_declare(re2
+ URL ${RE2_SOURCE_URL}
+ URL_HASH "SHA256=${ARROW_RE2_BUILD_SHA256_CHECKSUM}")
+ prepare_fetchcontent()
- externalproject_add(re2_ep
- ${EP_COMMON_OPTIONS}
- INSTALL_DIR ${RE2_PREFIX}
- URL ${RE2_SOURCE_URL}
- URL_HASH "SHA256=${ARROW_RE2_BUILD_SHA256_CHECKSUM}"
- CMAKE_ARGS ${RE2_CMAKE_ARGS}
- BUILD_BYPRODUCTS "${RE2_STATIC_LIB}")
-
- file(MAKE_DIRECTORY "${RE2_PREFIX}/include")
- add_library(re2::re2 STATIC IMPORTED)
- set_target_properties(re2::re2 PROPERTIES IMPORTED_LOCATION
"${RE2_STATIC_LIB}")
- target_include_directories(re2::re2 BEFORE INTERFACE "${RE2_PREFIX}/include")
-
- add_dependencies(re2::re2 re2_ep)
- set(RE2_VENDORED TRUE)
- # Set values so that FindRE2 finds this too
- set(RE2_LIB ${RE2_STATIC_LIB})
- set(RE2_INCLUDE_DIR "${RE2_PREFIX}/include")
-
- list(APPEND ARROW_BUNDLED_STATIC_LIBS re2::re2)
-endmacro()
+ # Unity build causes some build errors
+ set(CMAKE_UNITY_BUILD OFF)
+ fetchcontent_makeavailable(re2)
+
+ # Install RE2 for gRPC to find via find_package()
+ # Save and disable RE2's install script
+ add_custom_command(OUTPUT "${re2_BINARY_DIR}/cmake_install.cmake.saved"
+ COMMAND ${CMAKE_COMMAND} -E copy_if_different
+ "${re2_BINARY_DIR}/cmake_install.cmake"
+ "${re2_BINARY_DIR}/cmake_install.cmake.saved"
+ COMMAND ${CMAKE_COMMAND} -E echo
+ "# RE2 install disabled to prevent double
installation with Arrow"
+ > "${re2_BINARY_DIR}/cmake_install.cmake"
+ DEPENDS re2::re2
+ COMMENT "Disabling RE2 install to prevent double
installation"
+ VERBATIM)
Review Comment:
The custom command lacks an intermediate target between `re2::re2` and the
install disable logic. The c-ares and Abseil implementations use an
intermediate `<dep>_built` target to ensure all libraries are built before
manipulating install scripts. Consider adding a `re2_built` target that depends
on `re2::re2` to improve clarity and ensure proper sequencing, similar to lines
3008 and 3090-3156 in the codebase.
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2799,37 +2799,64 @@ if(ARROW_WITH_ZSTD)
endif()
# ----------------------------------------------------------------------
-# RE2 (required for Gandiva)
+# RE2 (required for Gandiva and gRPC)
-macro(build_re2)
- message(STATUS "Building RE2 from source")
- set(RE2_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/re2_ep-install")
- set(RE2_STATIC_LIB
-
"${RE2_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}re2${CMAKE_STATIC_LIBRARY_SUFFIX}")
+function(build_re2)
+ list(APPEND CMAKE_MESSAGE_INDENT "RE2: ")
+ message(STATUS "Building RE2 from source using FetchContent")
+ set(RE2_VENDORED
+ TRUE
+ PARENT_SCOPE)
+ set(RE2_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/re2_fc-install")
+ set(RE2_PREFIX
+ "${RE2_PREFIX}"
+ PARENT_SCOPE)
- set(RE2_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS}
"-DCMAKE_INSTALL_PREFIX=${RE2_PREFIX}")
+ fetchcontent_declare(re2
+ URL ${RE2_SOURCE_URL}
+ URL_HASH "SHA256=${ARROW_RE2_BUILD_SHA256_CHECKSUM}")
+ prepare_fetchcontent()
- externalproject_add(re2_ep
- ${EP_COMMON_OPTIONS}
- INSTALL_DIR ${RE2_PREFIX}
- URL ${RE2_SOURCE_URL}
- URL_HASH "SHA256=${ARROW_RE2_BUILD_SHA256_CHECKSUM}"
- CMAKE_ARGS ${RE2_CMAKE_ARGS}
- BUILD_BYPRODUCTS "${RE2_STATIC_LIB}")
-
- file(MAKE_DIRECTORY "${RE2_PREFIX}/include")
- add_library(re2::re2 STATIC IMPORTED)
- set_target_properties(re2::re2 PROPERTIES IMPORTED_LOCATION
"${RE2_STATIC_LIB}")
- target_include_directories(re2::re2 BEFORE INTERFACE "${RE2_PREFIX}/include")
-
- add_dependencies(re2::re2 re2_ep)
- set(RE2_VENDORED TRUE)
- # Set values so that FindRE2 finds this too
- set(RE2_LIB ${RE2_STATIC_LIB})
- set(RE2_INCLUDE_DIR "${RE2_PREFIX}/include")
-
- list(APPEND ARROW_BUNDLED_STATIC_LIBS re2::re2)
-endmacro()
+ # Unity build causes some build errors
+ set(CMAKE_UNITY_BUILD OFF)
+ fetchcontent_makeavailable(re2)
+
+ # Install RE2 for gRPC to find via find_package()
+ # Save and disable RE2's install script
+ add_custom_command(OUTPUT "${re2_BINARY_DIR}/cmake_install.cmake.saved"
+ COMMAND ${CMAKE_COMMAND} -E copy_if_different
+ "${re2_BINARY_DIR}/cmake_install.cmake"
+ "${re2_BINARY_DIR}/cmake_install.cmake.saved"
+ COMMAND ${CMAKE_COMMAND} -E echo
+ "# RE2 install disabled to prevent double
installation with Arrow"
+ > "${re2_BINARY_DIR}/cmake_install.cmake"
+ DEPENDS re2::re2
+ COMMENT "Disabling RE2 install to prevent double
installation"
+ VERBATIM)
+
+ add_custom_target(re2_install_disabled ALL
+ DEPENDS "${re2_BINARY_DIR}/cmake_install.cmake.saved")
+
+ # Install RE2 to RE2_PREFIX for gRPC to find
+ add_custom_command(OUTPUT "${RE2_PREFIX}/.re2_installed"
+ COMMAND ${CMAKE_COMMAND} -E copy_if_different
+ "${re2_BINARY_DIR}/cmake_install.cmake.saved"
+ "${re2_BINARY_DIR}/cmake_install.cmake.tmp"
+ COMMAND ${CMAKE_COMMAND}
-DCMAKE_INSTALL_PREFIX=${RE2_PREFIX}
+ -DCMAKE_INSTALL_CONFIG_NAME=$<CONFIG> -P
+ "${re2_BINARY_DIR}/cmake_install.cmake.tmp"
+ COMMAND ${CMAKE_COMMAND} -E touch
"${RE2_PREFIX}/.re2_installed"
+ DEPENDS re2_install_disabled
+ COMMENT "Installing RE2 to ${RE2_PREFIX} for gRPC"
+ VERBATIM)
Review Comment:
The install command lacks error handling that exists in the c-ares
implementation (line 3032-3033). Consider adding `|| ${CMAKE_COMMAND} -E true`
after the install command to handle cases where the install might fail
gracefully, matching the pattern used for c-ares which also needs to be
installed for gRPC.
--
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]
