GH-50581: [C++][R] Use bundled simdjson for C++ wrapper builds#50587
Conversation
|
|
|
@github-actions crossbow submit -g r |
This comment was marked as outdated.
This comment was marked as outdated.
|
@kou It looks like the remaining failures are related to the ongoing simdjson version discussion. If we go with requiring simdjson 4.0+ and falling back to the bundled version when the system package is too old, I think that should address this issue. |
|
I don't think this is isolated to R, e.g. see windows python nightly failure and you can check other nightlies. |
|
Could you try this? diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 1677b188b9..dc8629669f 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2828,7 +2828,7 @@ function(build_simdjson)
TRUE
PARENT_SCOPE)
- list(APPEND ARROW_BUNDLED_STATIC_LIBS simdjson::simdjson)
+ list(APPEND ARROW_BUNDLED_STATIC_LIBS simdjson)
set(ARROW_BUNDLED_STATIC_LIBS
"${ARROW_BUNDLED_STATIC_LIBS}"
PARENT_SCOPE)
@@ -2845,6 +2845,11 @@ if(ARROW_WITH_SIMDJSON)
${ARROW_SIMDJSON_REQUIRED_VERSION}
IS_RUNTIME_DEPENDENCY
FALSE)
+ if(SIMDJSON_VENDORED)
+ add_library(arrow::simdjson ALIAS simdjson)
+ else()
+ add_library(arrow::simdjson ALIAS simdjson::simdjson)
+ endif()
endif()
function(build_rapidjson)
diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt
index e792574a18..0b21fec605 100644
--- a/cpp/src/arrow/CMakeLists.txt
+++ b/cpp/src/arrow/CMakeLists.txt
@@ -1044,7 +1044,7 @@ if(ARROW_JSON)
json/parser.cc
json/reader.cc)
foreach(ARROW_JSON_TARGET ${ARROW_JSON_TARGETS})
- target_link_libraries(${ARROW_JSON_TARGET} PRIVATE RapidJSON simdjson::simdjson)
+ target_link_libraries(${ARROW_JSON_TARGET} PRIVATE RapidJSON arrow::simdjson)
endforeach()
else()
set(ARROW_JSON_TARGET_SHARED)This will fix the following case: https://github.com/ursacomputing/crossbow/actions/runs/29887151358/job/88819918379#step:6:8835 |
|
@github-actions crossbow submit -g r |
|
|
@github-actions crossbow submit -g r |
This comment was marked as outdated.
This comment was marked as outdated.
|
@kou I suspect the remaining Crossbow failures may be related to changes in the JsonWriter PR that haven't been merged yet (for example, it adds |
|
test-r-macos-as-cran: https://github.com/ursacomputing/crossbow/actions/runs/29981142927/job/89122965972#step:9:1373 It seems that we can't build simdjson with AppleClang 21: https://github.com/ursacomputing/crossbow/actions/runs/29981142927/job/89122965972#step:9:56 |
|
test-r-depsource-system: https://github.com/ursacomputing/crossbow/actions/runs/29981142748/job/89122965384#step:6:3165 https://github.com/ursacomputing/crossbow/actions/runs/29981142748/job/89122965384#step:7:361 It seems that Could you try this? diff --git a/r/inst/build_arrow_static.sh b/r/inst/build_arrow_static.sh
index e7f453bb64..8c8d9d6b95 100755
--- a/r/inst/build_arrow_static.sh
+++ b/r/inst/build_arrow_static.sh
@@ -107,8 +107,9 @@ ${CMAKE_WRAPPER} ${CMAKE} -DARROW_BOOST_USE_SHARED=OFF \
-DCMAKE_FIND_PACKAGE_NO_PACKAGE_REGISTRY=ON \
-DCMAKE_UNITY_BUILD=${CMAKE_UNITY_BUILD:-OFF} \
-DOPENSSL_ROOT_DIR=${OPENSSL_ROOT_DIR} \
- -Dre2_SOURCE=${re2_SOURCE:-BUNDLED} \
-Dabsl_SOURCE=${absl_SOURCE:-BUNDLED} \
+ -Dre2_SOURCE=${re2_SOURCE:-BUNDLED} \
+ -Dsimdjson_SOURCE=${simdjson_SOURCE:-} \
-Dxsimd_SOURCE=${xsimd_SOURCE:-} \
-Dzstd_SOURCE=${zstd_SOURCE:-} \
${EXTRA_CMAKE_FLAGS} \ |
|
It seems that r-recheck-most failures are unrelated. |
|
Thanks @Reranko05 @rok I think that if we update the image revision for the Windows wheels, using |
|
Thanks @raulcd, I will revert the workaround and update the image revision instead. |
|
@raulcd I have reverted the previous workaround and bumped the Windows wheel image revision. Could you please re-run the Crossbow jobs. |
|
@github-actions crossbow submit wheel-windows-cp314-cp314t-* |
|
Revision: e470cf0 Submitted crossbow builds: ursacomputing/crossbow @ actions-ebacba2792
|
|
@github-actions crossbow submit wheel-windows-cp314-cp314-amd64 |
|
Revision: e470cf0 Submitted crossbow builds: ursacomputing/crossbow @ actions-03d0412c61
|
rok
left a comment
rok
left a comment
There was a problem hiding this comment.
CI and windows wheel jobs pass. If there's no objections I'll merge this.
|
@rok Thanks for the review and for rerunning the Crossbow jobs :) |
github-actions
Bot
added
awaiting changes
github-actions
Bot
added
awaiting change review
rok
left a comment
rok
left a comment
There was a problem hiding this comment.
Thanks for the quick effort @Reranko05 !
github-actions
Bot
added
awaiting merge
| # The macOS 11.3 SDK has incomplete C++20 concepts support, which prevents | ||
| # simdjson headers from compiling. Disable simdjson concepts for this SDK. | ||
| if(ARROW_JSON | ||
| AND CMAKE_OSX_SYSROOT | ||
| AND CMAKE_OSX_SYSROOT MATCHES "MacOSX11\\.3\\.sdk$") | ||
| message(STATUS "Disabling simdjson concepts for macOS SDK 11.3") | ||
| add_compile_definitions(SIMDJSON_CONCEPT_DISABLED=1) | ||
| endif() |
There was a problem hiding this comment.
@Reranko05 Could you move this to build_simdjson() and use target_compile_options() instead of add_compile_definitions() as a follow-up task?
Could you report this to upstream? Upstream also checks clang version but we need to SDK version not clang version.
https://github.com/simdjson/simdjson/blob/8e6bac94877f2d3d026000d36ce81e0aaf38d26f/include/simdjson/compiler_check.h#L83-L87
Rationale
The R build may pick up a system-installed simdjson when
simdjson_SOURCEis not explicitly set. This causes build failures in environments where the system version is incompatible or unsuitable for the build.What changes are included?
This change defaults
simdjson_SOURCEtoBUNDLEDwhen it is not already set, ensuring C++ wrapper builds invoked throughbuild_arrow_static.shuse the bundled simdjson by default, while still allowing users to override it explicitly.This addresses the macOS CRAN release and ubuntu-r-only-r (AUTO deps) failures reported in GH-50581.
Related: #50581