Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true #39602

Merged
merged 3 commits into from
Jan 16, 2024
Merged

GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true #39602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06c1e79
GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true
nealrichardson Jan 14, 2024
d96fca2
Handle possibility that cmake version isn't determined correctly
nealrichardson Jan 14, 2024
2d6b8b3
add missing format token
nealrichardson Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion r/DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ URL: https://github.com/apache/arrow/, https://arrow.apache.org/docs/r/
BugReports: https://github.com/apache/arrow/issues
Encoding: UTF-8
Language: en-US
SystemRequirements: C++17; for AWS S3 support on Linux, libcurl and openssl (optional)
SystemRequirements: C++17; for AWS S3 support on Linux, libcurl and openssl (optional);
cmake >= 3.16 (build-time only, and only for full source build)
Biarch: true
Imports:
assertthat,
Expand Down
38 changes: 26 additions & 12 deletions r/tools/nixlibs.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ find_latest_nightly <- function(description_version,
}

try_download <- function(from_url, to_file, hush = quietly) {
if (!download_ok) {
# Don't even try
return(FALSE)
}
# We download some fairly large files, so ensure the timeout is set appropriately.
# This assumes a static library size of 100 MB (generous) and a download speed
# of .3 MB/s (slow). This is to anticipate slower user connections or load on
Expand Down Expand Up @@ -496,7 +500,7 @@ build_libarrow <- function(src_dir, dst_dir) {
Sys.setenv(MAKEFLAGS = makeflags)
}
if (!quietly) {
lg("Building with MAKEFLAGS=", makeflags)
lg("Building with MAKEFLAGS=%s", makeflags)
}
# Check for libarrow build dependencies:
# * cmake
Expand Down Expand Up @@ -595,7 +599,6 @@ ensure_cmake <- function(cmake_minimum_required = "3.16") {

if (is.null(cmake)) {
# If not found, download it
lg("cmake", .indent = "****")
CMAKE_VERSION <- Sys.getenv("CMAKE_VERSION", "3.26.4")
if (on_macos) {
postfix <- "-macos-universal.tar.gz"
Expand Down Expand Up @@ -642,28 +645,42 @@ ensure_cmake <- function(cmake_minimum_required = "3.16") {
bin_dir,
"/cmake"
)
} else {
# Show which one we found
# Full source builds will always show "cmake" in the logs
lg("cmake: %s", cmake, .indent = "****")
lg("cmake %s", CMAKE_VERSION, .indent = "****")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding version here – in my other testing on macbuilder I wanted this / added it in hackily so glad to see it for real here.

}
cmake
}

find_cmake <- function(paths = c(
Sys.getenv("CMAKE"),
Sys.which("cmake"),
# CRAN has it here, not on PATH
if (on_macos) "/Applications/CMake.app/Contents/bin/cmake",
Sys.which("cmake3")
),
version_required = "3.16") {
# Given a list of possible cmake paths, return the first one that exists and is new enough
# version_required should be a string or packageVersion; numeric version
# can be misleading (e.g. 3.10 is actually 3.1)
for (path in paths) {
if (nzchar(path) && cmake_version(path) >= version_required) {
if (nzchar(path) && file.exists(path)) {
# Sys.which() returns a named vector, but that plays badly with c() later
names(path) <- NULL
return(path)
found_version <- cmake_version(path)
if (found_version >= version_required) {
# Show which one we found
lg("cmake %s: %s", found_version, path, .indent = "****")
# Stop searching here
return(path)
} else {
# Keep trying
lg("Not using cmake found at %s", path, .indent = "****")
if (found_version > 0) {
lg("Version >= %s required; found %s", version_required, found_version, .indent = "*****")
} else {
# If cmake_version() couldn't determine version, it returns 0
lg("Could not determine version; >= %s required", version_required, .indent = "*****")
}
}
}
}
# If none found, return NULL
Expand Down Expand Up @@ -890,11 +907,8 @@ if (not_cran || on_macos) {
# and don't fall back to a full source build
build_ok <- !env_is("LIBARROW_BUILD", "false")

# Check if we're authorized to download (not asked an offline build).
# (Note that cmake will still be downloaded if necessary
# https://arrow.apache.org/docs/developers/cpp/building.html#offline-builds)
# Check if we're authorized to download
download_ok <- !test_mode && !env_is("TEST_OFFLINE_BUILD", "true")

download_libarrow_ok <- download_ok && !env_is("LIBARROW_DOWNLOAD", "false")

# This "tools/thirdparty_dependencies" path, within the tar file, might exist if
Expand Down
3 changes: 1 addition & 2 deletions r/vignettes/developers/setup.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,11 +281,10 @@ withr::with_makevars(list(CPPFLAGS = "", LDFLAGS = ""), remotes::install_github(
environment variables that determine how the build works and what features
get built.
* `TEST_OFFLINE_BUILD`: When set to `true`, the build script will not download
prebuilt the C++ library binary.
prebuilt the C++ library binary or, if needed, `cmake`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
prebuilt the C++ library binary or, if needed, `cmake`.
prebuilt the C++ library binary or, if needed, `cmake` of an appropriate version.

We could also say "cmake >= 3.16" here instead, but then have to make sure we bump both. But it might be nice to call out the version requirement here too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly; don't think it matters much one way or another. Most likely if you're reading this file, you've already tried to install and hit the error message that tells you what version you need 🤷

IMO your exact suggestion isn't worth adding, I'd think that appropriate version was implied. If you wanted to list the version here, I'd leave a comment in nixlibs.R next to cmake_minimum_version saying to update the vignette when you bump it. (Of course, that only gets bumped when we bump https://github.com/apache/arrow/blob/main/cpp/CMakeLists.txt#L18, and really only if anyone remembers that we have it listed here too.)

It will turn off any features that require a download, unless they're available
in `ARROW_THIRDPARTY_DEPENDENCY_DIR` or the `tools/thirdparty_download/` subfolder.
`create_package_with_all_dependencies()` creates that subfolder.
Regardless of this flag's value, `cmake` will be downloaded if it's unavailable.

# Troubleshooting

Expand Down