Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GH-38570: [R] Ensure that test-nix-libs is warning free #38571

Merged
merged 3 commits into from
Nov 7, 2023
Merged

GH-38570: [R] Ensure that test-nix-libs is warning free #38571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e566e78
Stop on warnings
jonkeane Nov 3, 2023
b4bd90c
Prevent NULL in ifelse from warning
jonkeane Nov 3, 2023
2141d13
Pacakge versions should be characters
jonkeane Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci/scripts/r_test.sh
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
assignUser marked this conversation as resolved.
Show resolved Hide resolved
}

# (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) {
Comment on lines +835 to +838
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also wrap this whole block in if (!test_mode) { ... } since we don't want to run the actual detection yet, just check the functions. Unless I'm missing something and we actually do want this whole main chunk to run?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do want the whole main chunk to run (or at least, from spending a few minutes with the script last week it seemed like that was the original intention). Wrapping the main logic in a function and testing that function would be another way to go about it but I don't feel to strongly about it (I'm just glad there's a place to put tests for these functions).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And just to make sure I understand: you're suggesting leaving this as is rather than making the whole block skip while being tested.

We could also suppress the message more deeply (or even in the test script) but IMHO both of those are harder to figure out what's up / reason about than stopping this one bit from running during testing and producing distracting output. But happy to change it to elsewhere if you have strong feelings about it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of the distracting output is fixed in #38534 even with sourcing the script (output here: https://github.com/apache/arrow/actions/runs/6746935491/job/18341961924?pr=38534#step:5:23756 ). Is there any part of that output that could be improved?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit here is about suppressing this part, which I see in that output:

 *** No nightly binaries were found for version 8.0.0.9000: falling back to libarrow build from source

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also set LIBARROW_BINARY=false?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also just used the existing pattern so 🤷 I think it should be fine to fix it like this as further up is a block that sets the version specifically for testing and iirc we don't use the arrow repo option in any of the tests (other wise we would also see fails in ci).

not_cran <- TRUE
arrow_repo <- paste0(getOption("arrow.dev_repo", "https://nightlies.apache.org/arrow/r"), "/libarrow/")
VERSION <- find_latest_nightly(VERSION)
Expand Down
Loading