Copilot commented on code in PR #55461:
URL: https://github.com/apache/arrow/pull/55461#discussion_r4520060175
##########
r/tools/nixlibs.R:
##########
@@ -646,6 +646,9 @@ build_libarrow <- function(src_dir, dst_dir) {
# Add the *_SOURCE_URL env vars
env_var_list <- set_thirdparty_urls(env_var_list)
}
+ # Some thirdparty dependencies are required even for a minimal build, so they
+ # can't be turned off like the ones above.
+ env_var_list <- with_vendored_dependencies(env_var_list)
Review Comment:
The unconditional call breaks direct source installs from an unsynced
checkout: `find_local_source()` supports `../cpp` or `ARROW_SOURCE_HOME`, but
`get_component_names()` requires `tools/cpp/thirdparty/versions.txt`. Without
`make sync-cpp`, installation now stops with `Failed to run
download_dependencies_R.sh` before compilation. Skip optional vendoring when
its directory or synced metadata is absent so these installs retain their
existing dependency resolution.
##########
r/tools/test-nixlibs.R:
##########
@@ -232,3 +232,50 @@ test_that("find_latest_nightly()", {
"Failed to find latest nightly"
)
})
+
+test_that("with_vendored_dependencies()", {
+ vendored_dir <- tempfile()
+ dir.create(vendored_dir)
+ on.exit(unlink(vendored_dir, recursive = TRUE), add = TRUE)
+ original_dir <- nixlibs_env$vendored_dependency_dir
+ original_get_component_names <- nixlibs_env$get_component_names
+ on.exit(nixlibs_env$vendored_dependency_dir <- original_dir, add = TRUE)
+ on.exit(nixlibs_env$get_component_names <- original_get_component_names, add
= TRUE)
+ nixlibs_env$vendored_dependency_dir <- vendored_dir
+ nixlibs_env$get_component_names <- function() {
+ data.frame(
+ env_varname = c("ARROW_URIPARSER_URL", "ARROW_XSIMD_URL",
"ARROW_ZSTD_URL"),
+ filename = c("uriparser-1.0.2.tar.bz2", "xsimd-14.3.0.tar.gz",
"zstd-1.5.7.tar.gz")
+ )
+ }
+ withr::local_envvar(ARROW_URIPARSER_URL = NA, ARROW_XSIMD_URL = NA)
+
+ # Without the vendored tarballs, nothing is set
+ expect_output(
+ expect_identical(with_vendored_dependencies(list(A = "1")), list(A = "1")),
+ "Vendored source for ARROW_URIPARSER_URL not found"
+ )
+
+ uriparser <- file.path(vendored_dir, "uriparser-1.0.2.tar.bz2")
+ xsimd <- file.path(vendored_dir, "xsimd-14.3.0.tar.gz")
+ # Only dependencies in vendored_dependencies are used
+ zstd <- file.path(vendored_dir, "zstd-1.5.7.tar.gz")
+ file.create(uriparser, xsimd, zstd)
+ expect_identical(
+ with_vendored_dependencies(list(A = "1")),
+ list(A = "1", ARROW_URIPARSER_URL = normalizePath(uriparser),
ARROW_XSIMD_URL = normalizePath(xsimd))
+ )
+
+ # Already set (e.g. by set_thirdparty_urls()) is respected
+ expect_identical(
+ with_vendored_dependencies(list(ARROW_URIPARSER_URL = "other.tar.bz2")),
+ list(ARROW_URIPARSER_URL = "other.tar.bz2", ARROW_XSIMD_URL =
normalizePath(xsimd))
+ )
+
+ # As is a user-set env var
+ withr::local_envvar(ARROW_URIPARSER_URL =
"https://example.com/uriparser.tar.bz2";)
Review Comment:
Pass the environment variables as a named vector through `.new`, both here
and at line 251. `local_envvar()` does not interpret arbitrary named arguments
in `...` as environment-variable assignments, so these calls do not apply the
intended overrides. In a clean environment, the final assertion therefore fails
rather than exercising the user-supplied URL, blocking the tools test suite run
by `r_test.sh`.
--
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]
