You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "nealrichardson (via GitHub)" <gi...@apache.org> on 2024/01/14 18:40:03 UTC
[PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
nealrichardson opened a new pull request, #39602:
URL: https://github.com/apache/arrow/pull/39602
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".
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1891161223
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/
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1892572780
> 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1892711063
It looks like we have some growing confusion in our env vars that govern the build script. [The docker-compose job has these set](https://github.com/apache/arrow/blob/main/docker-compose.yml#L1416-L1417
):
```
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_`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1892920030
See #39625 for the minor fixes
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1891036844
:warning: GitHub issue #39601 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on code in PR #39602:
URL: https://github.com/apache/arrow/pull/39602#discussion_r1451848234
##########
r/vignettes/developers/setup.Rmd:
##########
@@ -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`.
Review Comment:
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.)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1893443282
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 0d128c6d01ccf247c2922752e589c763599ded09.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/20523990591) has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1892497618
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #39602:
URL: https://github.com/apache/arrow/pull/39602#discussion_r1451790584
##########
r/vignettes/developers/setup.Rmd:
##########
@@ -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`.
Review Comment:
```suggestion
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1892351694
@github-actions crossbow submit -g r
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1892926030
@assignUser since you're addressing the offline-maximal build in #39625, is this ready to merge?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1892357215
Revision: d96fca2018e326c72aceb0e1320fb774c619ca2d
Submitted crossbow builds: [ursacomputing/crossbow @ actions-6702e93fdc](https://github.com/ursacomputing/crossbow/branches/all?query=actions-6702e93fdc)
|Task|Status|
|----|------|
|r-binary-packages|[](https://github.com/ursacomputing/crossbow/actions/runs/7530787078/job/20497961858)|
|test-fedora-r-clang-sanitizer|[](https://github.com/ursacomputing/crossbow/runs/20497940084)|
|test-r-arrow-backwards-compatibility|[](https://github.com/ursacomputing/crossbow/actions/runs/7530786616/job/20497936701)|
|test-r-depsource-bundled|[](https://github.com/ursacomputing/crossbow/runs/20497938937)|
|test-r-depsource-system|[](https://github.com/ursacomputing/crossbow/actions/runs/7530787103/job/20497938512)|
|test-r-dev-duckdb|[](https://github.com/ursacomputing/crossbow/actions/runs/7530786748/job/20497937156)|
|test-r-devdocs|[](https://github.com/ursacomputing/crossbow/actions/runs/7530787220/job/20497939415)|
|test-r-gcc-11|[](https://github.com/ursacomputing/crossbow/actions/runs/7530786984/job/20497937735)|
|test-r-gcc-12|[](https://github.com/ursacomputing/crossbow/actions/runs/7530787207/job/20497938858)|
|test-r-install-local|[](https://github.com/ursacomputing/crossbow/actions/runs/7530787264/job/20497939587)|
|test-r-install-local-minsizerel|[](https://github.com/ursacomputing/crossbow/actions/runs/7530786609/job/20497937040)|
|test-r-library-r-base-latest|[](https://github.com/ursacomputing/crossbow/runs/20497938836)|
|test-r-linux-as-cran|[](https://github.com/ursacomputing/crossbow/actions/runs/7530786939/job/20497939288)|
|test-r-linux-rchk|[](https://github.com/ursacomputing/crossbow/actions/runs/7530786917/job/20497937546)|
|test-r-linux-valgrind|[](https://github.com/ursacomputing/crossbow/runs/20497940571)|
|test-r-minimal-build|[](https://github.com/ursacomputing/crossbow/runs/20497939977)|
|test-r-offline-maximal|[](https://github.com/ursacomputing/crossbow/actions/runs/7530786629/job/20497936644)|
|test-r-offline-minimal|[](https://github.com/ursacomputing/crossbow/runs/20497938749)|
|test-r-rhub-debian-gcc-devel-lto-latest|[](https://github.com/ursacomputing/crossbow/runs/20497938145)|
|test-r-rhub-debian-gcc-release-custom-ccache|[](https://github.com/ursacomputing/crossbow/runs/20497939311)|
|test-r-rhub-ubuntu-gcc-release-latest|[](https://github.com/ursacomputing/crossbow/runs/20497941219)|
|test-r-rstudio-r-base-4.1-opensuse153|[](https://github.com/ursacomputing/crossbow/runs/20497938706)|
|test-r-rstudio-r-base-4.2-centos7-devtoolset-8|[](https://github.com/ursacomputing/crossbow/runs/20497940078)|
|test-r-rstudio-r-base-4.2-focal|[](https://github.com/ursacomputing/crossbow/runs/20497939225)|
|test-r-ubuntu-22.04|[](https://github.com/ursacomputing/crossbow/actions/runs/7530786813/job/20497937287)|
|test-r-versions|[](https://github.com/ursacomputing/crossbow/actions/runs/7530787045/job/20497939277)|
|test-ubuntu-r-sanitizer|[](https://github.com/ursacomputing/crossbow/runs/20497938899)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1891041397
Revision: d96fca2018e326c72aceb0e1320fb774c619ca2d
Submitted crossbow builds: [ursacomputing/crossbow @ actions-c2547fba9e](https://github.com/ursacomputing/crossbow/branches/all?query=actions-c2547fba9e)
|Task|Status|
|----|------|
|r-binary-packages|[](https://github.com/ursacomputing/crossbow/actions/runs/7521124185/job/20471587290)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #39602:
URL: https://github.com/apache/arrow/pull/39602#issuecomment-1891040681
@github-actions crossbow submit r-binary-packages
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #39602:
URL: https://github.com/apache/arrow/pull/39602#discussion_r1451790217
##########
r/tools/nixlibs.R:
##########
@@ -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 = "****")
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39601: [R] Don't download cmake when TEST_OFFLINE_BUILD=true [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser merged PR #39602:
URL: https://github.com/apache/arrow/pull/39602
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org