nealrichardson commented on code in PR #48297:
URL: https://github.com/apache/arrow/pull/48297#discussion_r2626916959
##########
r/tools/nixlibs.R:
##########
@@ -340,39 +328,27 @@ get_macos_openssl_dir <- function() {
}
# (built with newer devtoolset but older glibc (2.17) for broader
compatibility, like manylinux2014)
Review Comment:
This comment seems out of place. Did we once check the glibc version in here?
##########
r/tools/nixlibs.R:
##########
@@ -340,39 +328,27 @@ get_macos_openssl_dir <- function() {
}
# (built with newer devtoolset but older glibc (2.17) for broader
compatibility, like manylinux2014)
-determine_binary_from_stderr <- function(errs) {
- if (is.null(attr(errs, "status"))) {
- # There was no error in compiling: so we found libcurl and OpenSSL >= 1.1,
- # openssl is < 3.0
- lg("Found libcurl and OpenSSL >= 1.1")
- return("openssl-1.1")
- # Else, check for dealbreakers:
- } else if (!on_macos && any(grepl("Using libc++", errs, fixed = TRUE))) {
+has_curl_and_openssl <- function(errs) {
Review Comment:
I really like the crisper function name, but it's not 100% correct because
it's also checking for libc++.
##########
r/tools/nixlibs.R:
##########
@@ -192,17 +192,12 @@ download_binary <- function(lib) {
# of action based on the current system. Other values you can set it to:
# * "FALSE" (not case-sensitive), to skip this option altogether
# * "TRUE" (not case-sensitive), to try to discover your current OS, or
-# * Some other string: a "linux-openssl-${OPENSSL_VERSION}" that corresponds to
-# a binary that is available, to override what this function may discover by
-# default.
+# * Some other string: a binary identifier that corresponds to a binary that is
Review Comment:
Since it _is_ possible for someone to pass these binary names in, do we need
to be concerned with backwards compatibility for user code that say has
`LIBARROW_BINARY=linux-x86_64-openssl-3.0`?
If we wanted to handle that, I think the logic would be:
* grep for `openssl-1`, error if found
* optionally, grep for `openssl-3` and print a deprecation warning
* then `sub("-openssl-3\\.0$", "", x)` (drop the suffix)
##########
r/tools/test-nixlibs.R:
##########
@@ -49,102 +49,85 @@ test_that("compile_test_program()", {
expect_true(header_not_found("wrong/NOTAHEADER", fail))
})
-test_that("determine_binary_from_stderr", {
+test_that("has_curl_and_openssl", {
expect_output(
- expect_identical(
- determine_binary_from_stderr(compile_test_program("int a;")),
- "openssl-1.1"
+ expect_true(
+ has_curl_and_openssl(compile_test_program("int a;"))
),
- "Found libcurl and OpenSSL >= 1.1"
+ "Found libcurl and OpenSSL >= 3.0.0"
)
nixlibs_env$on_macos <- FALSE
expect_output(
- expect_identical(
- determine_binary_from_stderr(compile_test_program("#error Using OpenSSL
version 1.0")),
- "openssl-1.0"
+ expect_false(
+ has_curl_and_openssl(compile_test_program("#error OpenSSL version must
be 3.0 or greater"))
),
- "Found libcurl and OpenSSL < 1.1"
+ "OpenSSL found but version >= 3.0.0 is required"
)
- nixlibs_env$on_macos <- TRUE
expect_output(
Review Comment:
This test seems redundant to the test on L53 now, yeah?
##########
r/tools/nixlibs.R:
##########
@@ -340,39 +328,27 @@ get_macos_openssl_dir <- function() {
}
# (built with newer devtoolset but older glibc (2.17) for broader
compatibility, like manylinux2014)
-determine_binary_from_stderr <- function(errs) {
- if (is.null(attr(errs, "status"))) {
- # There was no error in compiling: so we found libcurl and OpenSSL >= 1.1,
- # openssl is < 3.0
- lg("Found libcurl and OpenSSL >= 1.1")
- return("openssl-1.1")
- # Else, check for dealbreakers:
- } else if (!on_macos && any(grepl("Using libc++", errs, fixed = TRUE))) {
+has_curl_and_openssl <- function(errs) {
+ # Check for dealbreakers:
+ if (!on_macos && any(grepl("Using libc++", errs, fixed = TRUE))) {
# Our linux binaries are all built with GNU stdlib so they fail with libc++
lg("Linux binaries incompatible with libc++")
- return(NULL)
+ return(FALSE)
} else if (header_not_found("curl/curl", errs)) {
lg("libcurl not found")
- return(NULL)
+ return(FALSE)
} else if (header_not_found("openssl/opensslv", errs)) {
lg("OpenSSL not found")
- return(NULL)
- } else if (any(grepl("OpenSSL version too old", errs))) {
- lg("OpenSSL found but version >= 1.0.2 is required for some features")
- return(NULL)
- # Else, determine which other binary will work
- } else if (any(grepl("Using OpenSSL version 1.0", errs))) {
- if (on_macos) {
- lg("OpenSSL 1.0 is not supported on macOS")
- return(NULL)
- }
- lg("Found libcurl and OpenSSL < 1.1")
- return("openssl-1.0")
- } else if (any(grepl("Using OpenSSL version 3", errs))) {
+ return(FALSE)
+ } else if (any(grepl("OpenSSL version must be 3.0 or greater", errs))) {
+ lg("OpenSSL found but version >= 3.0.0 is required")
Review Comment:
super nit but do we really need to go all the way to the patch version and
say "3.0.0" in the message?
--
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]
