PR comments

r/inst/build_arrow_static.sh

@@ -74,8 +74,8 @@ ${CMAKE} -DARROW_BOOST_USE_SHARED=OFF \
     -DARROW_DATASET=${ARROW_DATASET:-ON} \
     -DARROW_DEPENDENCY_SOURCE=${ARROW_DEPENDENCY_SOURCE:-AUTO} \
     -DAWSSDK_SOURCE=${AWSSDK_SOURCE:-} \
-    -DBoost_SOURCE=${BOOST_SOURCE:-} \
-    -Dlz4_SOURCE=${LZ4_SOURCE:-} \
+    -DBoost_SOURCE=${Boost_SOURCE:-} \
+    -Dlz4_SOURCE=${lz4_SOURCE:-} \
     -DARROW_FILESYSTEM=ON \
     -DARROW_GCS=${ARROW_GCS:-$ARROW_DEFAULT_PARAM} \
     -DARROW_JEMALLOC=${ARROW_JEMALLOC:-$ARROW_DEFAULT_PARAM} \

r/tools/nixlibs.R

@@ -96,60 +96,49 @@ try_download <- function(from_url, to_file, hush = quietly) {
   !inherits(status, "try-error") && status == 0
 }
 
-try_checksum <- function(binary_url, libfile, hush = quietly) {
+validate_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
-  # 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 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)
-        }
+  if (!skip_checksum && dir.exists(checksum_path) && is_release ||
+    enforce_checksum) {
+    # Munge the path to the correct sha file which we include during the
+    # release process
+    checksum_file <- sub(".+/bin/(.+\\.zip)", "\\1\\.sha512", binary_url)
+    checksum_file <- file.path(checksum_path, checksum_file)
+
+    # Check for `shasum`, and try `sha512sum` if not found
+    if (nzchar(Sys.which("shasum"))) {
+      checksum_cmd <- "shasum"
+      checksum_args <- c("--status", "-a", "512", "-c", checksum_file)
+    } else {
+      checksum_cmd <- "sha512sum"
+      checksum_args <- c("--status", "-c", checksum_file)
+    }
 
-        checksum_ok <- system2(checksum_cmd, args = checksum_args)
+    checksum_ok <- system2(checksum_cmd, args = checksum_args) == 0
 
-        if (checksum_ok != 0) {
-          lg("Checksum validation failed for libarrow: %s/%s", lib, libfile)
-          unlink(libfile)
-          stop("The checksum failed")
-        } else {
-          lg("Checksum validated successfully for libarrow: %s/%s", lib, libfile)
-        }
-      }
-    },
-    silent = hush
-  )
+    if (checksum_ok) {
+      lg("Checksum validated successfully for libarrow")
+    } else {
+      lg("Checksum validation failed for libarrow")
+      unlink(libfile)
+    }
+  } else {
+    checksum_ok <- TRUE
+  }
 
   # Return whether the checksum was successful
-  !inherits(status, "try-error")
+  checksum_ok
 }
 
 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) && try_checksum(binary_url, libfile)) {
+  if (try_download(binary_url, libfile) && validate_checksum(binary_url, libfile)) {
     lg("Successfully retrieved libarrow (%s)", lib)
   } else {
     # If the download or checksum fail, we will set libfile to NULL this will
@@ -487,7 +476,7 @@ env_vars_as_string <- function(env_var_list) {
   stopifnot(
     length(env_var_list) == length(names(env_var_list)),
     all(grepl("^[^0-9]", names(env_var_list))),
-    all(grepl("^[A-Z0-9_]+$", names(env_var_list))),
+    all(grepl("^[a-zA-Z0-9_]+$", names(env_var_list))),
     !any(grepl("'", env_var_list, fixed = TRUE))
   )
   env_var_string <- paste0(names(env_var_list), "='", env_var_list, "'", collapse = " ")
@@ -564,12 +553,12 @@ build_libarrow <- function(src_dir, dst_dir) {
 
   # On macOS, if not otherwise set, let's override Boost_SOURCE to be bundled
   if (on_macos) {
+    # Using lowercase (e.g. Boost_SOURCE) to match the cmake args we use already.
     deps_to_bundle <- c("Boost", "lz4")
     for (dep_to_bundle in deps_to_bundle) {
       env_var <- paste0(dep_to_bundle, "_SOURCE")
       if (Sys.getenv(env_var) == "") {
-        # TODO: env_var_list gets checked for caps, so we need to do that, but maybe it shouldn't?
-        env_var_list <- c(env_var_list, setNames("BUNDLED", toupper(env_var)))
+        env_var_list <- c(env_var_list, setNames("BUNDLED", env_var))
       }
     }
   }