This is an automated email from the ASF dual-hosted git repository. thisisnic pushed a commit to branch maint-15.0.0-r in repository https://gitbox.apache.org/repos/asf/arrow.git
commit 5574f582bc3c12a45d4673155e9bc3ebad4e2f55 Author: Dewey Dunnington <[email protected]> AuthorDate: Wed Feb 7 20:21:07 2024 -0400 GH-39738: [R] Support build against the last three released versions of Arrow (#39739) ### Rationale for this change Development velocity of the R package has slowed considerably since early versions of Arrow such that the commit-level integration that we once relied on is no longer necessary. The ability to build against older versions of Arrow also opens up more options for our CRAN submissions, since we may be able to work with CRAN to build a version of Arrow C++ they are happy with. This change doesn't require us to *do* anything about it...it just adds a check so that we are aware of the first PR that breaks the ability to build against a previous version. There is a possibility that an accidentally but previously installed version will end up being used via pkg-config, which I believe is how the version checking came into existence in the first place. ### What changes are included in this PR? - An `#if` to guard code that was added to support the string view/binary view - Changes to the version checker script to not error for supported Arrow C++ versions - CI job that checks build against supported Arrow versions ### Are these changes tested? Yes, a CI job was added ### Are there any user-facing changes? Yes, but I'll wait until there's consensus on this before documenting what our intended support policy will be. * Closes: #39738 Lead-authored-by: Dewey Dunnington <[email protected]> Co-authored-by: Jacob Wujciak-Jens <[email protected]> Co-authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Jacob Wujciak-Jens <[email protected]> --- .github/workflows/r.yml | 57 +++++++++++++++++++++++++++++++++++++++++++ r/PACKAGING.md | 1 + r/src/r_to_arrow.cpp | 9 +++++++ r/tools/check-versions.R | 35 ++++++++++++++++---------- r/tools/test-check-versions.R | 40 ++++++++++++++++++++---------- 5 files changed, 116 insertions(+), 26 deletions(-) diff --git a/.github/workflows/r.yml b/.github/workflows/r.yml index 4fc308a28d..90309f0f9c 100644 --- a/.github/workflows/r.yml +++ b/.github/workflows/r.yml @@ -52,6 +52,63 @@ env: DOCKER_VOLUME_PREFIX: ".docker/" jobs: + ubuntu-minimum-cpp-version: + name: Check minimum supported Arrow C++ Version (${{ matrix.cpp_version }}) + runs-on: ubuntu-latest + strategy: + matrix: + include: + - cpp_version: "13.0.0" + steps: + - name: Checkout Arrow + uses: actions/checkout@3df4ab11eba7bda6032a0b82a6bb43b11571feac # v4.0.0 + with: + path: src + submodules: recursive + + - name: Install Arrow C++ (${{ matrix.cpp_version }}) + run: | + sudo apt update + sudo apt install -y -V ca-certificates lsb-release wget + wget https://apache.jfrog.io/artifactory/arrow/$(lsb_release --id --short | tr 'A-Z' 'a-z')/apache-arrow-apt-source-latest-$(lsb_release --codename --short).deb + sudo apt install -y -V ./apache-arrow-apt-source-latest-$(lsb_release --codename --short).deb + sudo apt update + # We have to list all packages to avoid version conflicts. + sudo apt install -y -V libarrow-dev=${{ matrix.cpp_version }}-1 \ + libarrow-acero-dev=${{ matrix.cpp_version }}-1 \ + libparquet-dev=${{ matrix.cpp_version }}-1 \ + libarrow-dataset-dev=${{ matrix.cpp_version }}-1 + + - name: Install checkbashisms + run: | + sudo apt-get install devscripts + + - uses: r-lib/actions/setup-r@v2 + with: + use-public-rspm: true + install-r: false + + - uses: r-lib/actions/setup-r-dependencies@v2 + with: + extra-packages: any::rcmdcheck + needs: check + working-directory: src/r + + - uses: r-lib/actions/check-r-package@v2 + with: + working-directory: src/r + env: + LIBARROW_BINARY: "false" + LIBARROW_BUILD: "false" + ARROW_R_VERBOSE_TEST: "true" + ARROW_R_ALLOW_CPP_VERSION_MISMATCH: "true" + + - name: Show install output + if: always() + run: find src/r/check -name '00install.out*' -exec cat '{}' \; || true + shell: bash + + ubuntu: name: AMD64 Ubuntu ${{ matrix.ubuntu }} R ${{ matrix.r }} Force-Tests ${{ matrix.force-tests }} runs-on: ubuntu-latest diff --git a/r/PACKAGING.md b/r/PACKAGING.md index 7f42ecf562..4edeb4f213 100644 --- a/r/PACKAGING.md +++ b/r/PACKAGING.md @@ -26,6 +26,7 @@ For a high-level overview of the release process see the ## Before the release candidate is cut - [ ] [Create a GitHub issue](https://github.com/apache/arrow/issues/new/) entitled `[R] CRAN packaging checklist for version X.X.X` and copy this checklist to the issue. +- [ ] Review deprecated functions to advance their deprecation status, including removing preprocessor directives that no longer apply (search for `ARROW_VERSION_MAJOR` in r/src). - [ ] Evaluate the status of any failing [nightly tests and nightly packaging builds](http://crossbow.voltrondata.com). These checks replicate most of the checks that CRAN runs, so we need them all to be passing or to understand that the failures may (though won't necessarily) result in a rejection from CRAN. - [ ] Check [current CRAN check results](https://cran.rstudio.org/web/checks/check_results_arrow.html) - [ ] Ensure the contents of the README are accurate and up to date. diff --git a/r/src/r_to_arrow.cpp b/r/src/r_to_arrow.cpp index d2db11e14a..a81210f0ad 100644 --- a/r/src/r_to_arrow.cpp +++ b/r/src/r_to_arrow.cpp @@ -1050,6 +1050,7 @@ class RDictionaryConverter<ValueType, enable_if_has_string_view<ValueType>> template <typename T, typename Enable = void> struct RConverterTrait; +#if ARROW_VERSION_MAJOR >= 15 template <typename T> struct RConverterTrait< T, enable_if_t<!is_nested_type<T>::value && !is_interval_type<T>::value && @@ -1061,6 +1062,14 @@ template <typename T> struct RConverterTrait<T, enable_if_binary_view_like<T>> { // not implemented }; +#else +template <typename T> +struct RConverterTrait< + T, enable_if_t<!is_nested_type<T>::value && !is_interval_type<T>::value && + !is_extension_type<T>::value>> { + using type = RPrimitiveConverter<T>; +}; +#endif template <typename T> struct RConverterTrait<T, enable_if_list_like<T>> { diff --git a/r/tools/check-versions.R b/r/tools/check-versions.R index 3d8cbf02a1..34b2ef680c 100644 --- a/r/tools/check-versions.R +++ b/r/tools/check-versions.R @@ -20,6 +20,20 @@ args <- commandArgs(TRUE) # TESTING is set in test-check-version.R; it won't be set when called from configure test_mode <- exists("TESTING") +release_version_supported <- function(r_version, cpp_version) { + r_version <- package_version(r_version) + cpp_version <- package_version(cpp_version) + major <- function(x) as.numeric(x[1, 1]) + minimum_cpp_version <- package_version("13.0.0") + + allow_mismatch <- identical(tolower(Sys.getenv("ARROW_R_ALLOW_CPP_VERSION_MISMATCH", "false")), "true") + # If we allow a version mismatch we still need to cover the minimum version (13.0.0 for now) + # we don't allow newer C++ versions as new features without additional feature gates are likely to + # break the R package + version_valid <- cpp_version >= minimum_cpp_version && major(cpp_version) <= major(r_version) + allow_mismatch && version_valid || major(r_version) == major(cpp_version) +} + check_versions <- function(r_version, cpp_version) { r_parsed <- package_version(r_version) r_dev_version <- r_parsed[1, 4] @@ -39,20 +53,10 @@ check_versions <- function(r_version, cpp_version) { "*** > or retry with FORCE_BUNDLED_BUILD=true" ) cat(paste0(msg, "\n", collapse = "")) - } else if (r_is_patch && as.character(r_parsed[1, 1:3]) == cpp_version) { - # Patch releases we do for CRAN feedback get an extra x.y.z.1 version. - # These should work with the x.y.z C++ library (which never has .1 added) - cat( - sprintf( - "*** > Using C++ library version %s with R package %s\n", - cpp_version, - r_version - ) - ) - } else if (r_version != cpp_version) { + } else if (cpp_is_dev || !release_version_supported(r_version, cpp_parsed)) { cat( sprintf( - "**** Not using: C++ library version (%s) does not match R package (%s)\n", + "**** Not using: C++ library version (%s): not supported by R package version %s\n", cpp_version, r_version ) @@ -61,7 +65,12 @@ check_versions <- function(r_version, cpp_version) { # Add ALLOW_VERSION_MISMATCH env var to override stop()? (Could be useful for debugging) } else { # OK - cat(sprintf("**** C++ and R library versions match: %s\n", cpp_version)) + cat( + sprintf( + "**** C++ library version %s is supported by R version %s\n", + cpp_version, r_version + ) + ) } } diff --git a/r/tools/test-check-versions.R b/r/tools/test-check-versions.R index 9c284507b8..f558648bed 100644 --- a/r/tools/test-check-versions.R +++ b/r/tools/test-check-versions.R @@ -24,10 +24,10 @@ TESTING <- TRUE source("check-versions.R", local = TRUE) -test_that("check_versions", { +test_that("check_versions without mismatch", { expect_output( check_versions("10.0.0", "10.0.0"), - "**** C++ and R library versions match: 10.0.0", + "**** C++ library version 10.0.0 is supported by R version 10.0.0", fixed = TRUE ) expect_output( @@ -35,7 +35,7 @@ test_that("check_versions", { check_versions("10.0.0", "10.0.0-SNAPSHOT"), "version mismatch" ), - "**** Not using: C++ library version (10.0.0-SNAPSHOT) does not match R package (10.0.0)", + "**** Not using: C++ library version (10.0.0-SNAPSHOT): not supported by R package version 10.0.0", fixed = TRUE ) expect_output( @@ -43,20 +43,12 @@ test_that("check_versions", { check_versions("10.0.0.9000", "10.0.0-SNAPSHOT"), "version mismatch" ), - "**** Not using: C++ library version (10.0.0-SNAPSHOT) does not match R package (10.0.0.9000)", - fixed = TRUE - ) - expect_output( - expect_error( - check_versions("10.0.0.9000", "10.0.0"), - "version mismatch" - ), - "**** Not using: C++ library version (10.0.0) does not match R package (10.0.0.9000)", + "**** Not using: C++ library version (10.0.0-SNAPSHOT): not supported by R package version 10.0.0.9000", fixed = TRUE ) expect_output( check_versions("10.0.0.3", "10.0.0"), - "*** > Using C++ library version 10.0.0 with R package 10.0.0.3", + "**** C++ library version 10.0.0 is supported by R version 10.0.0.3", fixed = TRUE ) expect_output( @@ -65,3 +57,25 @@ test_that("check_versions", { fixed = TRUE ) }) + +test_that("check_versions with mismatch", { + withr::local_envvar(.new = c(ARROW_R_ALLOW_CPP_VERSION_MISMATCH = "false")) + + expect_false( + release_version_supported("15.0.0", "13.0.0") + ) + + withr::local_envvar(.new = c(ARROW_R_ALLOW_CPP_VERSION_MISMATCH = "true")) + + expect_true( + release_version_supported("15.0.0", "13.0.0") + ) + + expect_false( + release_version_supported("15.0.0", "16.0.0") + ) + + expect_false( + release_version_supported("15.0.0", "12.0.0") + ) +})
