-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-41402: [CI][R] Update our backwards compatibility CI any other R 4.4 cleanups #41403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f50c938
fc71c2f
a9923c0
8d6c18b
8b0e626
df56447
ded9675
def1020
b9c27d2
8d91cab
67941ba
8cad637
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,7 @@ ENV LANG=C.UTF-8 | |
| # Build R | ||
| # [1] https://www.digitalocean.com/community/tutorials/how-to-install-r-on-ubuntu-18-04 | ||
| # [2] https://linuxize.com/post/how-to-install-r-on-ubuntu-18-04/#installing-r-packages-from-cran | ||
| ARG r=3.6 | ||
|
||
| ARG r=4.4 | ||
| RUN apt-get update -y && \ | ||
| apt-get install -y \ | ||
| dirmngr \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,7 +46,12 @@ if [ "$ARROW_USE_PKG_CONFIG" != "false" ]; then | |
| export LD_LIBRARY_PATH=${ARROW_HOME}/lib:${LD_LIBRARY_PATH} | ||
| export R_LD_LIBRARY_PATH=${LD_LIBRARY_PATH} | ||
| fi | ||
| export _R_CHECK_COMPILATION_FLAGS_KNOWN_=${ARROW_R_CXXFLAGS} | ||
|
|
||
| export _R_CHECK_COMPILATION_FLAGS_KNOWN_="${_R_CHECK_COMPILATION_FLAGS_KNOWN_} ${ARROW_R_CXXFLAGS}" | ||
| # These should generally be picked up, but are slightly wrong in rhub's containers it appears | ||
| # https://github.com/r-hub/containers/pull/63 | ||
| export _R_CHECK_COMPILATION_FLAGS_KNOWN_="${_R_CHECK_COMPILATION_FLAGS_KNOWN_} -Wno-parentheses -Werror=format-security -Wp,-D_FORTIFY_SOURCE=3" | ||
|
Comment on lines
+50
to
+53
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not 100% certain, but it seems good for us to append our flags to what's already in this var in case it's there. Lines 51-52 are a workaround for what I believe is an rhub issue |
||
|
|
||
| if [ "$ARROW_R_DEV" = "TRUE" ]; then | ||
| # These are sometimes used in the Arrow C++ build and are not a problem | ||
| export _R_CHECK_COMPILATION_FLAGS_KNOWN_="${_R_CHECK_COMPILATION_FLAGS_KNOWN_} -Wno-attributes -msse4.2 -Wno-noexcept-type -Wno-subobject-linkage" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,7 @@ ${R_BIN} CMD INSTALL ${INSTALL_ARGS} arrow*.tar.gz | |
| pushd tests | ||
|
|
||
| # to generate suppression files run: | ||
| # ${R_BIN} --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --gen-suppressions=all --log-file=memcheck.log" -f testthat.supp | ||
| # ${R_BIN} --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --gen-suppressions=all --log-file=memcheck.log" -f testthat.R | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Caught while making the suppression file again |
||
| ${R_BIN} --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --suppressions=/${1}/ci/etc/valgrind-cran.supp" -f testthat.R |& tee testthat.out | ||
|
|
||
| # valgrind --error-exitcode=1 should return an erroring exit code that we can catch, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,7 +84,7 @@ jobs: | |
| {{ macros.github_set_sccache_envvars()|indent(8)}} | ||
| run: | | ||
| cd arrow/r | ||
| R CMD INSTALL --install-tests --no-test-load --no-docs --no-help --no-byte-compile arrow_with_deps.tar.gz | ||
| R CMD INSTALL --install-tests --no-test-load --no-byte-compile arrow_with_deps.tar.gz | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apparently docs are checked in 4.4 regardless and we get an error about corrupt docs. It's not too much extra time to build them though |
||
| - name: Run the tests | ||
| run: R -e 'if(tools::testInstalledPackage("arrow") != 0L) stop("There was a test failure.")' | ||
| - name: Dump test logs | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,7 +227,7 @@ jobs: | |
| working-directory: 'arrow' | ||
| extra-packages: cpp11 | ||
| - name: Set CRAN like openssl | ||
| if: contains(matrix.platform.runs_on, 'arm64') | ||
| if: contains(matrix.platform.name, 'arm64') | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed both here and on main these jobs were complaining about finding ssl And it looks like this might be a typo here, I traced it back to #37684 where I'm not sure how this was passing prior to the recent binary instability (and the windows failure mask when this ssl failure started). But this seems to work though the upload step failed. @assignUser (1) does this change look ok and (2) do you know what's up with the upload error?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe the failure is the change in version number? Or is it a permissions on from-fork builds (I don't think so since it's actually from crossbow, but that could be passed through)?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jonkeane The change looks good but I think the issue actually happened when we moved to gh hosted arm64 runners ( The upload job is failing because |
||
| run: | | ||
| # The arm64 runners contain openssl 1.1.1t in this path that is always included first so we need to override the | ||
| # default setting of the brew --prefix as root dir to avoid version conflicts. | ||
|
|
@@ -300,16 +300,14 @@ jobs: | |
| # an OS that is not in the allowlist, so we have to opt-in to use the | ||
| # binary. Other env vars used in r_docker_configure.sh can be added | ||
| # here (like devtoolset) and wired up in the later steps. | ||
| - {image: "rhub/debian-clang-devel", libarrow_binary: "TRUE"} | ||
| - {image: "rhub/ubuntu-clang", libarrow_binary: "TRUE"} | ||
| # fedora-clang-devel cannot use binaries bc of libc++ (uncomment to see the error) | ||
| # - {image: "rhub/fedora-clang-devel", libarrow_binary: "TRUE"} | ||
| - {image: "rhub/ubuntu-gcc-release"} # currently ubuntu-20.04 (focal) | ||
| - {image: "rocker/r-ubuntu:22.04"} # openssl3 | ||
| - {image: "rocker/r-ver"} # whatever is latest ubuntu LTS | ||
| - {image: "rhub/ubuntu-release"} # currently ubuntu-22.04 | ||
| - {image: "rocker/r-ver:4.0.0"} # ubuntu-20.04 | ||
| - {image: "rocker/r-ver:3.6.3", libarrow_binary: "TRUE"} # debian:buster (10) | ||
| - {image: "rstudio/r-base:4.1-focal"} # ubuntu-20.04 | ||
| - {image: "rstudio/r-base:4.2-centos7", devtoolset: "8"} | ||
| - {image: "rstudio/r-base:4.3-noble"} | ||
| steps: | ||
| # Get the arrow checkout just for the docker config scripts | ||
| # Don't need submodules for this (hence false arg to macro): they fail on | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -888,12 +888,12 @@ tasks: | |
| - r-lib__libarrow__bin__darwin-arm64-openssl-3.0__arrow-{no_rc_r_version}\.zip | ||
| - r-lib__libarrow__bin__darwin-x86_64-openssl-1.1__arrow-{no_rc_r_version}\.zip | ||
| - r-lib__libarrow__bin__darwin-x86_64-openssl-3.0__arrow-{no_rc_r_version}\.zip | ||
| - r-pkg__bin__windows__contrib__4.4__arrow_{no_rc_r_version}\.zip | ||
| - r-pkg__bin__windows__contrib__4.3__arrow_{no_rc_r_version}\.zip | ||
| - r-pkg__bin__windows__contrib__4.2__arrow_{no_rc_r_version}\.zip | ||
| - r-pkg__bin__macosx__big-sur-x86_64__contrib__4.4__arrow_{no_rc_r_version}\.tgz | ||
| - r-pkg__bin__macosx__big-sur-x86_64__contrib__4.3__arrow_{no_rc_r_version}\.tgz | ||
| - r-pkg__bin__macosx__contrib__4.2__arrow_{no_rc_r_version}\.tgz | ||
| - r-pkg__bin__macosx__big-sur-arm64__contrib__4.4__arrow_{no_rc_r_version}\.tgz | ||
| - r-pkg__bin__macosx__big-sur-arm64__contrib__4.3__arrow_{no_rc_r_version}\.tgz | ||
| - r-pkg__bin__macosx__big-sur-arm64__contrib__4.2__arrow_{no_rc_r_version}\.tgz | ||
| - r-pkg__src__contrib__arrow_{no_rc_r_version}\.tar\.gz | ||
|
|
||
|
|
||
|
|
@@ -1356,7 +1356,7 @@ tasks: | |
| r_tag: latest | ||
| r_custom_ccache: true | ||
|
|
||
| {% for r_org, r_image, r_tag in [("rhub", "ubuntu-gcc-release", "latest"), | ||
| {% for r_org, r_image, r_tag in [("rhub", "ubuntu-release", "latest"), | ||
| ("rocker", "r-ver", "latest"), | ||
| ("rstudio", "r-base", "4.2-focal"), | ||
| ("rstudio", "r-base", "4.1-opensuse153")] %} | ||
|
|
@@ -1377,9 +1377,9 @@ tasks: | |
| template: r/azure.linux.yml | ||
| params: | ||
| r_org: rhub | ||
| r_image: debian-gcc-devel-lto | ||
| r_image: gcc13 | ||
| r_tag: latest | ||
| flags: '-e NOT_CRAN=false -e INSTALL_ARGS=--use-LTO' | ||
| flags: '-e INSTALL_ARGS=--use-LTO' | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The LTO image from rhub/containers doesn't have tex installed and as cran means we check the manual so we get errors there. We could install tex when we need it with something like this added to r_test.sh But I'm not sure this actually gets us much. So I've removed the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The purpose of this job IIRC is to test that we compile with LTO, we probably don't even need to run the tests, let alone do a full check with docs etc. So this is fine? |
||
|
|
||
| # This one has -flto=auto | ||
| test-r-ubuntu-22.04: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -140,7 +140,7 @@ register_bindings_type_cast <- function() { | |
| fix.empty.names = TRUE, | ||
| stringsAsFactors = FALSE) { | ||
| # we need a specific value of stringsAsFactors because the default was | ||
| # TRUE in R <= 3.6 | ||
| # TRUE in R <= 3.6 and folks might still be cargoculting to stay in the past. | ||
|
||
| if (!identical(stringsAsFactors, FALSE)) { | ||
| arrow_not_supported("stringsAsFactors = TRUE") | ||
| } | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the files touched are doing this change
ubuntu-gcc-releaseto the newubuntu-release