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

[nealrichardson]

See #39601

Are these changes tested?

Existing CI should pass. This should also pass on macbuilder without downloading cmake, and if hardcoding download_ok <- FALSE, it should exit cleanly and informatively.

Are there any user-facing changes?

Define "user".

• Closes: [R] Don't download cmake when TEST_OFFLINE_BUILD=true #39601

[github-actions]

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

[assignUser]

@github-actions crossbow submit r-binary-packages

[github-actions]

Revision: d96fca2

Submitted crossbow builds: ursacomputing/crossbow @ actions-c2547fba9e

Task	Status
r-binary-packages

[jonkeane]

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.

[jonkeane]

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.

[nealrichardson]

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.)

[nealrichardson]

Example output added:

**** cmake 3.28.1: /usr/local/bin/cmake

https://github.com/ursacomputing/crossbow/actions/runs/7521124185/job/20471586891#step:13:223

**** cmake 3.27.6: /usr/local/bin/cmake

https://github.com/ursacomputing/crossbow/actions/runs/7521124185/job/20471587000#step:13:47

**** cmake 3.21.2: /Applications/CMake.app/Contents/bin/cmake

https://mac.r-project.org/macbuilder/results/1705263596-b370573ae984333b/

[nealrichardson]

@github-actions crossbow submit -g r

[github-actions]

Revision: d96fca2

Submitted crossbow builds: ursacomputing/crossbow @ actions-6702e93fdc

Task	Status
r-binary-packages
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
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-library-r-base-latest
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-gcc-release-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-r-sanitizer

[nealrichardson]

Here's an example of the message when cmake is downloaded: https://github.com/ursacomputing/crossbow/actions/runs/7530786939/job/20497939288#step:7:24

The offline-maximal build now fails because the image must not have cmake in it: https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=59743&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=6c939d89-0d1a-51f2-8b30-091a7a82e98c&l=348

So we'll need to install it before the R package test. I can look into that; looks like there is ci/scripts/install_cmake.sh.

Also, I was looking through the jobs for evidence of where cmake is found but too old, haven't found one yet but I have seen some unexpected things. Of note, it looks like the jobs that are based on ubuntu-r-only-r are downloading and using a nightly libarrow (here's the gcc-12 build, for example: https://github.com/ursacomputing/crossbow/actions/runs/7530787207/job/20497938858#step:8:27). That doesn't seem right, is it? It's faster, but it means that if there's a change in the C++ library that we're trying to test with those jobs, it won't be tested, nor will the C++ library be compiled with the settings/compilers in that job. Seems like LIBARROW_BINARY=false needs to be added to them? @assignUser? Not related to the changes in this PR, but seems important.

[assignUser]

That doesn't seem right, is it?

Ah good catch, we can set it in the docker-compose of that job, that way we don't have to edit all the jobs separately.

[nealrichardson]

It looks like we have some growing confusion in our env vars that govern the build script. The docker-compose job has these set:

      FORCE_BUNDLED_BUILD: 'true'
      LIBARROW_BUILD: 'true'

FORCE_BUNDLED_BUILD is only used in the configure script to skip looking for libarrow on the system. With the current state of configure, it's basically equivalent to ARROW_USE_PKG_CONFIG=false, assuming ARROW_HOME is not set. It doesn't trigger a source build.

LIBARROW_BUILD is only checked to see if it is false, to prevent doing the bundled source build. The nixlibs.R script does not check for true to mean that you must do a source build, i.e. skip binary downloading.

Side note: I agree with the comment in https://github.com/apache/arrow/pull/38534/files#r1387468097 that LIBARROW_BINARY=false should be what we recommend (and in fact already document in the script) for saying not to download a prebuilt library. LIBARROW_DOWNLOAD existed before but was removed, though reading that discussion, apparently a no-op artifact of that was not cleaned up, which caused some confusion.

This feels like creeping scope so perhaps I should make another issue, but IMO:

• FORCE_BUNDLED_BUILD should be deprecated in favor of LIBARROW_BUILD. I don't think we recommend it to people, it's more for our testing.
• LIBARROW_BUILD=true is equivalent to LIBARROW_BINARY=false. If LIBARROW_BUILD=true and LIBARROW_BINARY is something other than unset or false, raise an error.
• Set download_libarrow_ok based on LIBARROW_BINARY=false, not LIBARROW_DOWNLOAD. Remove references to LIBARROW_DOWNLOAD.
• If we want something explicit about preventing downloading, that's TEST_OFFLINE_BUILD currently; if that's a feature we want to advertise, we could rename that something that's not TEST_.

(edit: I opened #39620)

[assignUser]

See #39625 for the minor fixes

[nealrichardson]

@assignUser since you're addressing the offline-maximal build in #39625, is this ready to merge?

[assignUser]

Thanks! We could rebase after #39625 is merged but I think we can merge this as is.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 0d128c6.

There were no benchmark performance regressions. 🎉

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