Skip to content

Commit

Permalink
apacheGH-38570: [R] Ensure that test-nix-libs is warning free (apache…
Browse files Browse the repository at this point in the history
…#38571)

### Rationale for this change

Although we stop / fail the CI job for warnings elsewhere, we don't seem to be doing that for our buildsystem tests. Let's change that so warnings + other output doesn't creep in.

- [x] Prevent `*** Failed to find latest nightly for 8.0.0.9000` from showing up when the file is sourced under test
- [x] fix `Warning ('test-nixlibs.R:140:3'): select_binary() with test program 'x' is NULL so the result will be NULL`
- [x] update version strings to be characters to avoid r-devel warnings

### What changes are included in this PR?

Added `stop_on_warning` to the test call, and fixes to make that pass

### Are these changes tested?

Yes, they are all part of the test suite 

### Are there any user-facing changes?

No.
* Closes: apache#38570

Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
  • Loading branch information
jonkeane authored Nov 7, 2023
1 parent 2e01f0c commit 7622ded
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 8 deletions.
2 changes: 1 addition & 1 deletion ci/scripts/r_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pushd ${source_dir}
printenv

# Run the nixlibs.R test suite, which is not included in the installed package
${R_BIN} -e 'setwd("tools"); testthat::test_dir(".")'
${R_BIN} -e 'setwd("tools"); testthat::test_dir(".", stop_on_warning = TRUE)'

# Before release, we always copy the relevant parts of the cpp source into the
# package. In some CI checks, we will use this version of the source:
Expand Down
4 changes: 2 additions & 2 deletions r/tools/check-versions.R
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ test_mode <- exists("TESTING")
check_versions <- function(r_version, cpp_version) {
r_parsed <- package_version(r_version)
r_dev_version <- r_parsed[1, 4]
r_is_dev <- !is.na(r_dev_version) && r_dev_version > 100
r_is_patch <- !is.na(r_dev_version) && r_dev_version <= 100
r_is_dev <- !is.na(r_dev_version) && r_dev_version > "100"
r_is_patch <- !is.na(r_dev_version) && r_dev_version <= "100"
cpp_is_dev <- grepl("SNAPSHOT$", cpp_version)
cpp_parsed <- package_version(sub("-SNAPSHOT$", "", cpp_version))

Expand Down
14 changes: 9 additions & 5 deletions r/tools/nixlibs.R
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,11 @@ select_binary <- function(os = tolower(Sys.info()[["sysname"]]),
errs <- compile_test_program(test_program)
openssl_version <- determine_binary_from_stderr(errs)
arch <- ifelse(identical(os, "darwin"), paste0("-", arch, "-"), "-")
ifelse(is.null(openssl_version), NULL, paste0(os, arch, openssl_version))
if (is.null(openssl_version)) {
NULL
} else {
paste0(os, arch, openssl_version)
}
},
error = function(e) {
lg("Unable to find libcurl and openssl")
Expand All @@ -210,7 +214,7 @@ select_binary <- function(os = tolower(Sys.info()[["sysname"]]),
lg("Building on %s %s", os, arch)
binary <- NULL
}
return(binary)
binary
}

# This tests that curl and OpenSSL are present (bc we can include their headers)
Expand Down Expand Up @@ -268,7 +272,7 @@ get_macos_openssl_dir <- function() {
openssl_root_dir <- "/usr/local"
}
}
return(openssl_root_dir)
openssl_root_dir
}

# (built with newer devtoolset but older glibc (2.17) for broader compatibility,# like manylinux2014)
Expand Down Expand Up @@ -828,10 +832,10 @@ quietly <- !env_is("ARROW_R_DEV", "true")

not_cran <- env_is("NOT_CRAN", "true")

if (is_release) {
if (is_release & !test_mode) {
VERSION <- VERSION[1, 1:3]
arrow_repo <- paste0(getOption("arrow.repo", sprintf("https://apache.jfrog.io/artifactory/arrow/r/%s", VERSION)), "/libarrow/")
} else {
} else if(!test_mode) {
not_cran <- TRUE
arrow_repo <- paste0(getOption("arrow.dev_repo", "https://nightlies.apache.org/arrow/r"), "/libarrow/")
VERSION <- find_latest_nightly(VERSION)
Expand Down

0 comments on commit 7622ded

Please sign in to comment.