GH-41952: [R] Turn S3 and ZSTD on by default for macOS #42210

jonkeane · 2024-06-19T20:06:01Z

Changeup nixlibs.R so that we enable S3 and ZSTD by default on CRAN. I've checked this against the CRAN macbuilder to confirm it does work (that's not a guarantee it'll work on other CRAN maintained macOS builders, but a good indication).

It also removes gcs from the list of features we expect to be on and warn folks about if it is not.

Rationale for this change

So that our builds are more fully featured.

What changes are included in this PR?

Enable ARROW_S3 by default when building form source on macOS.

Are these changes tested?

Existing CI should not fail.

Are there any user-facing changes?

Getting Arrow from the canonical repository would be more fully featured.

GitHub Issue: [R] macOS binaries on CRAN are not feature complete #41952

github-actions · 2024-06-19T20:06:25Z

⚠️ GitHub issue #41952 has been automatically assigned in GitHub to PR creator.

jonkeane · 2024-06-19T20:07:08Z

r/tools/nixlibs.R

@@ -536,7 +536,7 @@ build_libarrow <- function(src_dir, dst_dir) {
  }
  cleanup(build_dir)

-  env_var_list <- c(
+  env_var_list <- list(


This isn't technically necessary, but it was the easiest way to do what I wanted to in is_feature_requested(). Further, it's a bit strange that this thing called X_X_list is not a list.

jonkeane · 2024-06-19T20:08:46Z

r/tools/nixlibs.R

+is_feature_requested <- function(env_varname, env_var_list, default = env_is("LIBARROW_MINIMAL", "false")) {
+  # look in our env_var_list first, if it's not found there go to
+  # the actual environment
+  env_value <- tolower(env_var_list[[env_varname]])
+  if (is.null(env_value)) {
+    env_value <- tolower(Sys.getenv(env_varname))
+  }


This is a change in behavior that warrants some thought. Though I will say this is how I thought this worked already — or rather I thought is_feature_requested() was looking at env_var_list and not only looking at the literal environment. It's possible this was a mistake / it should have never looked at the environment but instead be looking at the env_var_list. But I kept it for now in case this is actually intended.

The original intent of the function was to check whether the user requested a feature. env_var_list doesn't contain any user-requested features, at least not in the active sense. But now that you're putting some default selections in there, it makes sense to check it here. The function is currently only used to check for S3/GCS, to know whether we need to have openssl and curl.

AAAAH I was missing the user- part there, that makes it make more sense now.

See, this is why passive voice is bad.

jonkeane · 2024-06-19T20:42:53Z

I also noticed two other optional libraries that were being flagged as not a full build. I added those, and ran mac builder: https://mac.r-project.org/macbuilder/results/1718828010-3fa0fa8b00b3b11c/

nealrichardson

This seems like the least bad option for turning these features on. The other places where we handle defaults, like by the value of LIBARROW_MINIMAL, aren't good for S3/GCS because we have to check up front for openssl/curl.

r/tools/nixlibs.R

nealrichardson · 2024-06-20T11:42:21Z

r/tools/nixlibs.R

+is_feature_requested <- function(env_varname, env_var_list, default = env_is("LIBARROW_MINIMAL", "false")) {
+  # look in our env_var_list first, if it's not found there go to
+  # the actual environment
+  env_value <- tolower(env_var_list[[env_varname]])
+  if (is.null(env_value)) {
+    env_value <- tolower(Sys.getenv(env_varname))
+  }


The original intent of the function was to check whether the user requested a feature. env_var_list doesn't contain any user-requested features, at least not in the active sense. But now that you're putting some default selections in there, it makes sense to check it here. The function is currently only used to check for S3/GCS, to know whether we need to have openssl and curl.

Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>

jonkeane · 2024-06-20T12:41:56Z

I also wonder whether GCS is worth including.

I agree + though similar when I checked if S3 alone was sufficient for quieting the message. I'm happy to pull GCS out of this, though IMO we should also add GCS to the block list so that it's not being checked if we do that too.

I'm happy to do this in this PR if we want.

nealrichardson · 2024-06-20T12:46:04Z

I also wonder whether GCS is worth including.

I agree + though similar when I checked if S3 alone was sufficient for quieting the message. I'm happy to pull GCS out of this, though IMO we should also add GCS to the block list so that it's not being checked if we do that too.

I'm happy to do this in this PR if we want.

Right, we would want to change that message.

On the one hand, users won't generally experience this because they'll get a binary from CRAN. On the other, GCS increases build time there and increases the BDR attack surface. I don't know how big that risk is and if demand for GCS outweighs it--I haven't noticed demand for GCS, but maybe I'm not looking in the right places.

I guess we could leave it on, and if we get dinged on CRAN, we know how to turn it back off?

jonkeane · 2024-06-20T15:26:22Z

MM, I think you've convinced me that we should pull GCS back for now (and see if someone requests it before we open up our attack surface more). At the very least, it would be nice to send something with a more minimal expansion at first in case the CRAN macOS builders are different in key ways, we'll know that it's just an issue with S3 or zstd now. And maybe add GCS in 18 (or only when someone asks for it in a CRAN release). Like you said, it's easy enough to see how to change these now.

jonkeane · 2024-06-20T22:45:28Z

@github-actions crossbow submit -g r

github-actions · 2024-06-20T22:48:02Z

Revision: 548b6b2

Submitted crossbow builds: ursacomputing/crossbow @ actions-e5cdc79a11

Task	Status
r-binary-packages
test-r-arrow-backwards-compatibility
test-r-clang-sanitizer
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse155
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-r-sanitizer

assignUser · 2024-06-20T22:53:16Z

I also wonder whether GCS is worth including.

IMO the CRAN builds should be as feature complete as possible as they are (well after the issue with the last two released binaries maybe used to be...) the way to get the package for mac users and there shouldn't be a reason for users to user alternative sources like r-universe or source build but:

On the other, GCS increases build time there and increases the BDR attack surface.

While I would have disagreed previously reality has beaten me down and eh... ok.

jonkeane · 2024-06-21T01:03:56Z

@github-actions crossbow submit -g r

github-actions · 2024-06-21T01:06:38Z

Revision: b23878a

Submitted crossbow builds: ursacomputing/crossbow @ actions-f75e6668f4

Task	Status
r-binary-packages
test-r-arrow-backwards-compatibility
test-r-clang-sanitizer
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse155
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-r-sanitizer

jonkeane · 2024-06-21T12:42:54Z

This is probably paranoid, but I'm going to run one last mc builder run before I merge.

nealrichardson · 2024-06-21T13:31:30Z

r/tools/nixlibs.R

+  env_var_list_value <- env_var_list[[env_varname]]
+  if (is.null(env_var_list_value)) {
+    env_var_list_value <- ""
+  }
+  env_value <- tolower(Sys.getenv(env_varname, env_var_list_value))


Is this equivalent?

Suggested change

env_var_list_value <- env_var_list[[env_varname]]

if (is.null(env_var_list_value)) {

env_var_list_value <- ""

}

env_value <- tolower(Sys.getenv(env_varname, env_var_list_value))

env_value <- tolower(Sys.getenv(env_varname, env_var_list[[env_varname]]))

Well I feel slightly better having thought the exact same thing. But turns out: no! env_var_list[[env_varname]] is NULL if it doesn't exist (which is what we want) but:

Sys.getenv("env_varname", NULL) Error in Sys.getenv("env_varname", NULL) : wrong type for argument

Though I only found that out when I pushed (+ got a little too eager and triggered all of crossbow r) and then saw lots of red.

This would be a perfect place for %||% or the native equivalent, but I didn't want to require rlang here (I mean we require it for the package, so maybe that's ok?)

jonkeane · 2024-06-21T14:49:37Z

I'm glad I submitted to mac builder again, cause I noticed there that we're doing something wrong with linking snappy: ... -lparquet -larrow_acero -larrow -larrow_bundled_dependencies /opt/homebrew/lib/libsnappy.1.1.9.dylib ...

Which then means when you try to load without having snappy from homebrew it fails. I will look into why this is and push it to this branch.

jonkeane · 2024-06-21T21:30:22Z

r/inst/build_arrow_static.sh

@@ -65,6 +65,7 @@ esac
 mkdir -p "${BUILD_DIR}"
 pushd "${BUILD_DIR}"
 ${CMAKE} -DARROW_BOOST_USE_SHARED=OFF \
+    -DARROW_SNAPPY_USE_SHARED=OFF \


This prevents shared (homebrew) snappy from being used on the builder. Are there other dependencies we should add here?

ARROW_DEPENDENCY_USE_SHARED should be the default for all the more specific versions, so just setting that should work?

conbench-apache-arrow · 2024-06-24T04:22:36Z

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 67bbf84.

There were 4 benchmark results indicating a performance regression:

Commit Run on ec2-m5-4xlarge-us-east-2 at 2024-06-23 23:12:34Z
- dataframe-to-table (R) with dataset=type_strings, language=R
- partitioned-dataset-filter (R) with dataset=dataset-taxi-parquet, language=R, query=vignette
and 2 more (see the report linked below)

The full Conbench report has more details. It also includes information about 88 possible false positives for unstable benchmarks that are known to sometimes produce them.

Turn S3 on by default for macos

3c62f88

jonkeane requested a review from assignUser as a code owner June 19, 2024 20:06

github-actions bot added Component: R awaiting committer review Awaiting committer review labels Jun 19, 2024

jonkeane commented Jun 19, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jun 19, 2024

jonkeane commented Jun 19, 2024

View reviewed changes

jonkeane requested review from nealrichardson and thisisnic June 19, 2024 20:09

Also GCS and ZSTD so we avoid the message about lacking

d32e1fa

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 19, 2024

jonkeane changed the title ~~GH-41952: [R] Turn S3 on by default for macOS~~ GH-41952: [R] Turn S3, GCS, and ZSTD on by default for macOS Jun 19, 2024

nealrichardson requested changes Jun 20, 2024

View reviewed changes

Update r/tools/nixlibs.R

46e33db

Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 20, 2024

Remove GCS to lessen attack surface

548b6b2

jonkeane requested a review from paleolimbot as a code owner June 20, 2024 22:42

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 20, 2024

jonkeane changed the title ~~GH-41952: [R] Turn S3, GCS, and ZSTD on by default for macOS~~ GH-41952: [R] Turn S3 and ZSTD on by default for macOS Jun 20, 2024

Oops, can't use NULL in unset

b23878a

nealrichardson approved these changes Jun 21, 2024

View reviewed changes

nealrichardson reviewed Jun 21, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 21, 2024

turn off shared snappy

82b64d2

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 21, 2024

jonkeane commented Jun 21, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 21, 2024

jonkeane merged commit 67bbf84 into apache:main Jun 23, 2024
12 checks passed

jonkeane removed the awaiting changes Awaiting changes label Jun 23, 2024

jonkeane mentioned this pull request Jun 23, 2024

[R] macOS binaries on CRAN are not feature complete #41952

Closed

jonkeane deleted the macos_default_s3 branch June 23, 2024 21:03

ludvigla mentioned this pull request Jun 24, 2024

Updates for arrow 17 compatibility PixelgenTechnologies/pixelatorR#31

Merged

9 tasks

grantmcdermott mentioned this pull request Jun 24, 2024

[R] S3 support not enabled for r-universe builds on MacOS #43030

Closed

jonkeane mentioned this pull request Aug 17, 2024

[R] AWS SDK fails to build on one of CRAN's M1 builders #43735

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41952: [R] Turn S3 and ZSTD on by default for macOS #42210

GH-41952: [R] Turn S3 and ZSTD on by default for macOS #42210

jonkeane commented Jun 19, 2024 •

edited

Loading

github-actions bot commented Jun 19, 2024

jonkeane Jun 19, 2024

jonkeane Jun 19, 2024

nealrichardson Jun 20, 2024

jonkeane Jun 20, 2024

nealrichardson Jun 20, 2024

jonkeane commented Jun 19, 2024

nealrichardson left a comment

nealrichardson Jun 20, 2024

jonkeane commented Jun 20, 2024 •

edited

Loading

nealrichardson commented Jun 20, 2024

jonkeane commented Jun 20, 2024

jonkeane commented Jun 20, 2024

github-actions bot commented Jun 20, 2024

assignUser commented Jun 20, 2024

jonkeane commented Jun 21, 2024

github-actions bot commented Jun 21, 2024

jonkeane commented Jun 21, 2024

nealrichardson Jun 21, 2024

jonkeane Jun 21, 2024

jonkeane commented Jun 21, 2024

jonkeane Jun 21, 2024

assignUser Jun 21, 2024

conbench-apache-arrow bot commented Jun 24, 2024

GH-41952: [R] Turn S3 and ZSTD on by default for macOS #42210

GH-41952: [R] Turn S3 and ZSTD on by default for macOS #42210

Conversation

jonkeane commented Jun 19, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeane commented Jun 19, 2024

nealrichardson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeane commented Jun 20, 2024 • edited Loading

nealrichardson commented Jun 20, 2024

jonkeane commented Jun 20, 2024

jonkeane commented Jun 20, 2024

github-actions bot commented Jun 20, 2024

assignUser commented Jun 20, 2024

jonkeane commented Jun 21, 2024

github-actions bot commented Jun 21, 2024

jonkeane commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeane commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Jun 24, 2024

jonkeane commented Jun 19, 2024 •

edited

Loading

jonkeane commented Jun 20, 2024 •

edited

Loading