nealrichardson commented on code in PR #39587:
URL: https://github.com/apache/arrow/pull/39587#discussion_r1451554041
##########
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
Review Comment:
Pollute with what? Might it be useful debug information if it fails? There
are already a bunch of trys inside here, this feels unnecessary.
##########
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:
(I know this is just moved and not new code but) to me this would be more
readable in its intent like:
```
# 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)
}
##########
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)
+ }
- checksum_ok <- system2(checksum_cmd, args = checksum_args)
+ checksum_ok <- system2(checksum_cmd, args = checksum_args)
- if (checksum_ok != 0) {
- lg("Checksum validation failed for libarrow: %s/%s", lib, libfile)
- unlink(libfile)
- libfile <- NULL
- } else {
- lg("Checksum validated successfully for libarrow: %s/%s", lib, libfile)
+ 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)
Review Comment:
Cleaner would be to drop it from the message here, the next message printed
(in `download_binary()`) will include `lib` so it's not needed.
##########
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)
+ }
- checksum_ok <- system2(checksum_cmd, args = checksum_args)
+ checksum_ok <- system2(checksum_cmd, args = checksum_args)
Review Comment:
I might make this `checksum_ok <- system2(checksum_cmd, args =
checksum_args) == 0`.
Then your message is based on `if(checksum_ok)`, and you return
`checksum_ok` from the function.
##########
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)
+ }
- checksum_ok <- system2(checksum_cmd, args = checksum_args)
+ checksum_ok <- system2(checksum_cmd, args = checksum_args)
- if (checksum_ok != 0) {
- lg("Checksum validation failed for libarrow: %s/%s", lib, libfile)
- unlink(libfile)
- libfile <- NULL
- } else {
- lg("Checksum validated successfully for libarrow: %s/%s", lib, libfile)
+ if (checksum_ok != 0) {
+ lg("Checksum validation failed for libarrow: %s/%s", lib, libfile)
+ unlink(libfile)
+ stop("The checksum failed")
Review Comment:
I don't think you need these, IIUC `download_binary()` will do the unlinking
```suggestion
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
