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 <dewey@voltrondata.com> Co-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de> Co-authored-by: Dewey Dunnington <dewey@fishandwhistle.net> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
apache · Feb 8, 2024 · 66b41c4 · 66b41c4
1 parent f609bb1
commit 66b41c4
Show file tree

Hide file tree

Showing 5 changed files with 116 additions and 26 deletions.
diff --git a/.github/workflows/r.yml b/.github/workflows/r.yml
@@ -54,6 +54,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
@@ -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
@@ -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
@@ -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
@@ -24,39 +24,31 @@ 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(
     expect_error(
       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(
     expect_error(
       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")
+  )
+})