jonkeane commented on code in PR #39587:
URL: https://github.com/apache/arrow/pull/39587#discussion_r1451557344
##########
r/tools/nixlibs.R:
##########
@@ -96,53 +96,76 @@ try_download <- function(from_url, to_file, hush = quietly)
{
!inherits(status, "try-error") && status == 0
}
-download_binary <- function(lib) {
- libfile <- paste0("arrow-", VERSION, ".zip")
- binary_url <- paste0(arrow_repo, "bin/", lib, "/arrow-", VERSION, ".zip")
- if (try_download(binary_url, libfile)) {
- lg("Successfully retrieved libarrow (%s)", lib)
- } else {
- lg(
- "Downloading libarrow failed for version %s (%s)\n at %s",
- VERSION, lib, binary_url
- )
- libfile <- NULL
- }
+try_checksum <- function(binary_url, libfile, hush = quietly) {
# Explicitly setting the env var to "false" will skip checksum validation
# e.g. in case the included checksums are stale.
skip_checksum <- env_is("ARROW_R_ENFORCE_CHECKSUM", "false")
enforce_checksum <- env_is("ARROW_R_ENFORCE_CHECKSUM", "true")
checksum_path <- Sys.getenv("ARROW_R_CHECKSUM_PATH", "tools/checksums")
# validate binary checksum for CRAN release only
- if (!skip_checksum && dir.exists(checksum_path) && is_release ||
- enforce_checksum) {
- checksum_file <- sub(".+/bin/(.+\\.zip)", "\\1\\.sha512", binary_url)
- checksum_file <- file.path(checksum_path, checksum_file)
- checksum_cmd <- "shasum"
- checksum_args <- c("--status", "-a", "512", "-c", checksum_file)
-
- # shasum is not available on all linux versions
- status_shasum <- try(
- suppressWarnings(
- system2("shasum", args = c("--help"), stdout = FALSE, stderr = FALSE)
- ),
- silent = TRUE
- )
+ # we do this in a try so that if it fails for any reason, we don't pollute
the log
+ # but then we will return that the checksum failed
+ status <- try(
+ {
+ if (!skip_checksum && dir.exists(checksum_path) && is_release ||
+ enforce_checksum) {
+ checksum_file <- sub(".+/bin/(.+\\.zip)", "\\1\\.sha512", binary_url)
+ checksum_file <- file.path(checksum_path, checksum_file)
+ checksum_cmd <- "shasum"
+ checksum_args <- c("--status", "-a", "512", "-c", checksum_file)
+
+ # shasum is not available on all linux versions, so check help if it
exists
+ status_shasum <- try(
+ suppressWarnings(
+ system2("shasum", args = c("--help"), stdout = FALSE, stderr =
FALSE)
+ ),
+ silent = TRUE
+ )
- if (inherits(status_shasum, "try-error") || is.integer(status_shasum) &&
status_shasum != 0) {
- checksum_cmd <- "sha512sum"
- checksum_args <- c("--status", "-c", checksum_file)
- }
+ # if shasum doens't exist, then change the command to sha512sum
+ if (inherits(status_shasum, "try-error") || is.integer(status_shasum)
&& status_shasum != 0) {
+ checksum_cmd <- "sha512sum"
+ checksum_args <- c("--status", "-c", checksum_file)
+ }
Review Comment:
Agreed that is much cleaner + more readable. Slight tangent: this current
code I was baffled by cause I didn't get at first that the call to shasum was
`--help` just to identify it (I thought we were trying once and if failing
falling back to sha512sum which is a totally valid otherway to do things).
@assignUser is there any specific reason you shell out to help rather than
what Neal's suggesting here? There aren't any comments to suggest this approach
isn't better but want to give you a chance to add some commentary here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
