From 13cf74e58cb0c72e0d6bdefab51e1a94ea69018f Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Fri, 26 Mar 2021 10:40:12 -0700 Subject: [PATCH 1/8] Followup to ARROW-11736: move all skip_if_not_available to use arrow_info() and register string libs there --- r/R/arrow-package.R | 3 +++ r/tests/testthat/helper-skip.R | 22 ++++------------------ 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/r/R/arrow-package.R b/r/R/arrow-package.R index 7696c47d98b..2515e7ac920 100644 --- a/r/R/arrow-package.R +++ b/r/R/arrow-package.R @@ -117,11 +117,14 @@ arrow_info <- function() { if (out$libarrow) { pool <- default_memory_pool() runtimeinfo <- runtime_info() + compute_funcs <- list_compute_functions() out <- c(out, list( capabilities = c( dataset = arrow_with_dataset(), parquet = arrow_with_parquet(), s3 = arrow_with_s3(), + utf8proc = "utf8_upper" %in% compute_funcs, + re2 = "replace_substring_regex" %in% compute_funcs, vapply(tolower(names(CompressionType)[-1]), codec_is_available, logical(1)) ), memory_pool = list( diff --git a/r/tests/testthat/helper-skip.R b/r/tests/testthat/helper-skip.R index 2c885cafd97..984889631e8 100644 --- a/r/tests/testthat/helper-skip.R +++ b/r/tests/testthat/helper-skip.R @@ -15,25 +15,11 @@ # specific language governing permissions and limitations # under the License. +build_features <- arrow_info()$capabilities + skip_if_not_available <- function(feature) { - if (feature == "dataset") { - skip_if_not(arrow_with_dataset()) - } else if (feature == "parquet") { - skip_if_not(arrow_with_parquet()) - } else if (feature %in% c("string", "utf8proc")) { - skip_if_not( - "utf8_upper" %in% list_compute_functions(), - "Arrow C++ library not built with utf8proc dependency" - ) - } else if (feature %in% c("regex", "re2")) { - skip_if_not( - "replace_substring_regex" %in% list_compute_functions(), - "Arrow C++ library not built with re2 dependency" - ) - } else if (feature == "s3") { - skip_if_not(arrow_with_s3()) - } else if (!codec_is_available(feature)) { - skip(paste("Arrow C++ not built with support for", feature)) + if (!isTRUE(build_features[feature])) { + skip(paste("Arrow C++ not built with", feature)) } } From b5c41402b6bb370e8e9c5bbfa06b25191fd25019 Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Fri, 26 Mar 2021 10:42:52 -0700 Subject: [PATCH 2/8] Handle one more skip case --- r/tests/testthat/helper-skip.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/r/tests/testthat/helper-skip.R b/r/tests/testthat/helper-skip.R index 984889631e8..c70b429f623 100644 --- a/r/tests/testthat/helper-skip.R +++ b/r/tests/testthat/helper-skip.R @@ -18,7 +18,8 @@ build_features <- arrow_info()$capabilities skip_if_not_available <- function(feature) { - if (!isTRUE(build_features[feature])) { + # Special handling for "uncompressed", for test that iterate over compressions + if (!(feature %in% "uncompressed") && !isTRUE(build_features[feature])) { skip(paste("Arrow C++ not built with", feature)) } } From afbccdaed9a8e3c641639c3a5111d0a285f31a58 Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Fri, 26 Mar 2021 10:54:03 -0700 Subject: [PATCH 3/8] Try to work around re2 failure on fedora-clang-devel --- r/tools/nixlibs.R | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/r/tools/nixlibs.R b/r/tools/nixlibs.R index ce862d0e45d..7716d24903b 100644 --- a/r/tools/nixlibs.R +++ b/r/tools/nixlibs.R @@ -331,6 +331,12 @@ build_libarrow <- function(src_dir, dst_dir) { # but `ar` fails to build libarrow_bundled_dependencies, so turn them off # so that there are no bundled deps env_vars <- paste(env_vars, "ARROW_JEMALLOC=OFF ARROW_PARQUET=OFF ARROW_DATASET=OFF ARROW_WITH_RE2=OFF ARROW_WITH_UTF8PROC=OFF EXTRA_CMAKE_FLAGS=-DARROW_SIMD_LEVEL=NONE") + } else if grepl("libc++", env_var_list["CXX"], fixed = TRUE) { + # ARROW-12094: re2 seems to fail on the rhub/fedora-clang-devel build, + # which among other features uses libc++ + # See ci/scripts/r_docker_configure.sh for config and references. + # Here, we'll just turn off re2 as a workaround + env_vars <- paste(env_vars, "ARROW_WITH_RE2=OFF") } cat("**** arrow", ifelse(quietly, "", paste("with", env_vars)), "\n") status <- system( From 5c5aacea633176086dc47fa068bee9db5cb54a3e Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Fri, 26 Mar 2021 11:25:13 -0700 Subject: [PATCH 4/8] More :facepalm: --- r/tools/nixlibs.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/r/tools/nixlibs.R b/r/tools/nixlibs.R index 7716d24903b..03e852fedf2 100644 --- a/r/tools/nixlibs.R +++ b/r/tools/nixlibs.R @@ -331,7 +331,7 @@ build_libarrow <- function(src_dir, dst_dir) { # but `ar` fails to build libarrow_bundled_dependencies, so turn them off # so that there are no bundled deps env_vars <- paste(env_vars, "ARROW_JEMALLOC=OFF ARROW_PARQUET=OFF ARROW_DATASET=OFF ARROW_WITH_RE2=OFF ARROW_WITH_UTF8PROC=OFF EXTRA_CMAKE_FLAGS=-DARROW_SIMD_LEVEL=NONE") - } else if grepl("libc++", env_var_list["CXX"], fixed = TRUE) { + } else if (grepl("libc++", env_var_list["CXX"], fixed = TRUE)) { # ARROW-12094: re2 seems to fail on the rhub/fedora-clang-devel build, # which among other features uses libc++ # See ci/scripts/r_docker_configure.sh for config and references. From fc3cfbd53cc302d4cc9274e2d325557058e95f2b Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Sun, 28 Mar 2021 08:02:08 -0700 Subject: [PATCH 5/8] Revert R build script special case --- r/tools/nixlibs.R | 6 ------ 1 file changed, 6 deletions(-) diff --git a/r/tools/nixlibs.R b/r/tools/nixlibs.R index 03e852fedf2..ce862d0e45d 100644 --- a/r/tools/nixlibs.R +++ b/r/tools/nixlibs.R @@ -331,12 +331,6 @@ build_libarrow <- function(src_dir, dst_dir) { # but `ar` fails to build libarrow_bundled_dependencies, so turn them off # so that there are no bundled deps env_vars <- paste(env_vars, "ARROW_JEMALLOC=OFF ARROW_PARQUET=OFF ARROW_DATASET=OFF ARROW_WITH_RE2=OFF ARROW_WITH_UTF8PROC=OFF EXTRA_CMAKE_FLAGS=-DARROW_SIMD_LEVEL=NONE") - } else if (grepl("libc++", env_var_list["CXX"], fixed = TRUE)) { - # ARROW-12094: re2 seems to fail on the rhub/fedora-clang-devel build, - # which among other features uses libc++ - # See ci/scripts/r_docker_configure.sh for config and references. - # Here, we'll just turn off re2 as a workaround - env_vars <- paste(env_vars, "ARROW_WITH_RE2=OFF") } cat("**** arrow", ifelse(quietly, "", paste("with", env_vars)), "\n") status <- system( From f402130451c6e51088c3836ea1f45bc08f762a63 Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Sun, 28 Mar 2021 08:03:25 -0700 Subject: [PATCH 6/8] ARG1 cmake solution from @kou --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index e1954418263..ef2ccfd0a6d 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -620,8 +620,8 @@ endif() # ---------------------------------------------------------------------- # ExternalProject options -set(EP_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}}") -set(EP_C_FLAGS "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${UPPERCASE_BUILD_TYPE}}") +set(EP_CXX_FLAGS "${CMAKE_CXX_COMPILER_ARG1} ${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}}") +set(EP_C_FLAGS "${CMAKE_C_COMPILER_ARG1} ${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${UPPERCASE_BUILD_TYPE}}") if(NOT MSVC_TOOLCHAIN) # Set -fPIC on all external projects From f52c1051104ad66d0475fb350fad93bf2dd458f1 Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Sun, 28 Mar 2021 16:46:35 +0000 Subject: [PATCH 7/8] Autoformat/render all the things [automated commit] --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index ef2ccfd0a6d..05cc642417a 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -620,8 +620,12 @@ endif() # ---------------------------------------------------------------------- # ExternalProject options -set(EP_CXX_FLAGS "${CMAKE_CXX_COMPILER_ARG1} ${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}}") -set(EP_C_FLAGS "${CMAKE_C_COMPILER_ARG1} ${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${UPPERCASE_BUILD_TYPE}}") +set( + EP_CXX_FLAGS + "${CMAKE_CXX_COMPILER_ARG1} ${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}}" + ) +set(EP_C_FLAGS + "${CMAKE_C_COMPILER_ARG1} ${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${UPPERCASE_BUILD_TYPE}}") if(NOT MSVC_TOOLCHAIN) # Set -fPIC on all external projects From 342ea03569bd8680db60efbdfd23d621f66a26ed Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Sun, 28 Mar 2021 17:20:55 -0700 Subject: [PATCH 8/8] Allow new skip_if_not_available to work on R < 3.5 --- r/tests/testthat/helper-skip.R | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/r/tests/testthat/helper-skip.R b/r/tests/testthat/helper-skip.R index c70b429f623..6fb97da000d 100644 --- a/r/tests/testthat/helper-skip.R +++ b/r/tests/testthat/helper-skip.R @@ -15,11 +15,15 @@ # specific language governing permissions and limitations # under the License. -build_features <- arrow_info()$capabilities +build_features <- c( + arrow_info()$capabilities, + # Special handling for "uncompressed", for tests that iterate over compressions + uncompressed = TRUE +) skip_if_not_available <- function(feature) { - # Special handling for "uncompressed", for test that iterate over compressions - if (!(feature %in% "uncompressed") && !isTRUE(build_features[feature])) { + yes <- feature %in% names(build_features) && build_features[feature] + if (!yes) { skip(paste("Arrow C++ not built with", feature)) } }