nealrichardson commented on code in PR #37684:
URL: https://github.com/apache/arrow/pull/37684#discussion_r1340348176
##########
r/tools/nixlibs.R:
##########
@@ -191,33 +202,50 @@ test_for_curl_and_openssl <- "
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
#error Using OpenSSL version 3
#endif
-"
+")
compile_test_program <- function(code) {
- # Note: if we wanted to check for openssl on macOS, we'd have to set the brew
- # path as a -I directory. But since we (currently) only run this code to
- # determine whether we can download a Linux binary, it's not relevant.
+ openssl_dir <- ""
+ if(on_macos) {
+ openssl_root_dir <- get_macos_openssl_dir()
+ openssl_dir <- paste0("-I", openssl_root_dir, "/include")
+ }
runner <- paste(
R_CMD_config("CXX17"),
+ openssl_dir,
R_CMD_config("CPPFLAGS"),
R_CMD_config("CXX17FLAGS"),
R_CMD_config("CXX17STD"),
"-E",
- "-xc++"
- )
+ "-xc++" )
Review Comment:
I think this change was unintentional
##########
r/tools/nixlibs.R:
##########
@@ -191,33 +202,50 @@ test_for_curl_and_openssl <- "
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
#error Using OpenSSL version 3
#endif
-"
+")
compile_test_program <- function(code) {
- # Note: if we wanted to check for openssl on macOS, we'd have to set the brew
- # path as a -I directory. But since we (currently) only run this code to
- # determine whether we can download a Linux binary, it's not relevant.
+ openssl_dir <- ""
+ if(on_macos) {
+ openssl_root_dir <- get_macos_openssl_dir()
+ openssl_dir <- paste0("-I", openssl_root_dir, "/include")
+ }
runner <- paste(
R_CMD_config("CXX17"),
+ openssl_dir,
R_CMD_config("CPPFLAGS"),
R_CMD_config("CXX17FLAGS"),
R_CMD_config("CXX17STD"),
"-E",
- "-xc++"
- )
+ "-xc++" )
suppressWarnings(system2("echo", sprintf('"%s" | %s -', code, runner),
stdout = FALSE, stderr = TRUE))
}
+get_macos_openssl_dir <- function(){
+ openssl_root_dir <- Sys.getenv("OPENSSL_ROOT_DIR", NA)
+ if (is.na(openssl_root_dir)) {
+ # try to guess default openssl include dir based on CRAN's build script
+ # https://github.com/R-macos/recipes/blob/master/build.sh#L35
+ if(identical(Sys.info()["machine"], "arm64")){
+ openssl_root_dir <- "/opt/R/arm64"
+ } else if (file.exists("/opt/R/x86_64")) {
+ openssl_root_dir <- "/opt/R/x86_64"
+ } else {
+ openssl_root_dir <- "/usr/local"
+ }
+ }
+ return(openssl_root_dir)
Review Comment:
This feels fragile. Would it make sense to check for the existence of some
header in that directory, and fall back to check others?
##########
r/tools/nixlibs.R:
##########
@@ -231,11 +259,15 @@ determine_binary_from_stderr <- function(errs) {
return(NULL)
# Else, determine which other binary will work
} else if (any(grepl("Using OpenSSL version 1.0", errs))) {
+ if(on_macos) {
+ cat("*** OpenSSL 1.0 is not supported on macOS\n")
Review Comment:
Should the message say that you need to upgrade OpenSSL? It's not clear what
action one should take with this message.
##########
r/tools/test-nixlibs.R:
##########
@@ -21,8 +21,8 @@
# Flag so that we just load the functions and don't evaluate them like we do
# when called from configure.R
TESTING <- TRUE
-
-source("nixlibs.R", local = TRUE)
+nixlibs_env <- environment()
Review Comment:
Out of curiosity, why?
##########
r/tools/nixlibs.R:
##########
@@ -168,18 +176,21 @@ select_binary <- function(os =
tolower(Sys.info()[["sysname"]]),
} else {
# No binary available for arch
cat(sprintf("*** Building on %s %s\n", os, arch))
- NULL
+ binary <- NULL
}
+ return(binary)
}
# This tests that curl and OpenSSL are present (bc we can include their
headers)
# and it checks for other versions/features and raises errors that we grep for
-test_for_curl_and_openssl <- "
-#include <ciso646>
+test_for_curl_and_openssl <- paste0(ifelse(on_macos, "",
Review Comment:
Rather than using paste, can you solve this with an `#ifdef`, so that you
have a clean, copyable test program?
##########
r/tools/nixlibs.R:
##########
@@ -248,6 +280,11 @@ header_not_found <- function(header, errs) {
#### start distro ####
distro <- function() {
+ # This is not part of distro but needed to enable prebuilt binaries on macos
Review Comment:
You can upstream this here :) https://github.com/nealrichardson/distro 🙏
##########
r/tools/nixlibs.R:
##########
@@ -191,33 +202,50 @@ test_for_curl_and_openssl <- "
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
#error Using OpenSSL version 3
#endif
-"
+")
compile_test_program <- function(code) {
- # Note: if we wanted to check for openssl on macOS, we'd have to set the brew
- # path as a -I directory. But since we (currently) only run this code to
- # determine whether we can download a Linux binary, it's not relevant.
+ openssl_dir <- ""
+ if(on_macos) {
+ openssl_root_dir <- get_macos_openssl_dir()
+ openssl_dir <- paste0("-I", openssl_root_dir, "/include")
+ }
runner <- paste(
R_CMD_config("CXX17"),
+ openssl_dir,
R_CMD_config("CPPFLAGS"),
R_CMD_config("CXX17FLAGS"),
R_CMD_config("CXX17STD"),
"-E",
- "-xc++"
- )
+ "-xc++" )
suppressWarnings(system2("echo", sprintf('"%s" | %s -', code, runner),
stdout = FALSE, stderr = TRUE))
}
+get_macos_openssl_dir <- function(){
+ openssl_root_dir <- Sys.getenv("OPENSSL_ROOT_DIR", NA)
+ if (is.na(openssl_root_dir)) {
+ # try to guess default openssl include dir based on CRAN's build script
+ # https://github.com/R-macos/recipes/blob/master/build.sh#L35
+ if(identical(Sys.info()["machine"], "arm64")){
+ openssl_root_dir <- "/opt/R/arm64"
Review Comment:
Indentation is off
##########
r/tools/nixlibs-allowlist.txt:
##########
@@ -2,3 +2,4 @@ ubuntu
centos
redhat
rhel
+darwin
Review Comment:
> IIRC the autobrew stuff worked because CRAN agrees to it
I'm not certain that that's the case, but regardless, if the plan is to
download binaries on CRAN, I agree with Jon's concerns, and we should think
through some scenarios/options:
1. Just submit the release and don't say anything. Fine until it breaks,
then you're flagged for violating the policy. You respond "ok, sorry, we'll
stop" and fall back to building everything from source on CRAN. You'd need to
make sure that all features like S3 were enabled in the mac bundled build on
CRAN, otherwise people will find that things no longer work when they install
binaries from CRAN. Maybe the long build times on CRAN machines will get them
to concede to accept downloading binaries. Maybe the builds will fail because
of weird system configuration they have that we aren't aware of. Maybe they'll
archive the package for taking up too much of CRAN's resources on top of policy
violation--generally being a "bad citizen" (I think I got that thrown at me
once).
2. Request CRAN approval to download binaries for macOS. Explain that we
have moved to a new build system that is fully under the ASF and Apache Arrow's
governance, all releases are signed, voted on by the PMC, and checksummed, and
that the ASF guarantees their hosting in perpetuity. And that we test for
compatibility with system libraries before selecting and using a C++ binary.
Hopefully CRAN finds the ASF sufficiently reliable and says yes. If they say
yes, great. They may also ask "what about Linux and Windows"; we may be able to
just do the same for both now too. They may say no, in which case we're all in
build-from-source mode as above, and if that calls into question the Windows
build, we have bigger problems because we don't have a bundled windows build
today (which is why I might suggest only asking about downloading for macOS,
and only after they approve then ask about other platforms).
Of the options, I personally lean to (2). It definitely has risks but the
catastrophic risk is lower, I suspect. Plus, they do have in the policy that it
is possible to get approval to download binaries, and I can't see how they
could deny our request unless that's a hollow statement--ASF backing, open
governance, thorough process, and (minus Windows for now) and completely
possible, albeit slow, fallback to building from source.
Would love to hear others' thoughts.
##########
r/tools/nixlibs.R:
##########
@@ -248,6 +280,11 @@ header_not_found <- function(header, errs) {
#### start distro ####
distro <- function() {
+ # This is not part of distro but needed to enable prebuilt binaries on macos
Review Comment:
You can upstream this here :) https://github.com/nealrichardson/distro 🙏
--
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]
