GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor #38534

paleolimbot · 2023-10-31T19:03:09Z

Rationale for this change

A few rough edges exist after #38236 including:

When zero or 1 nightly with the matching major version exist, detection of the latest nightly might fail
At least one CI job is pulling nightlies instead of using the version from the current commit

What changes are included in this PR?

Clean up find_latest_nightly() + add test
Ensure all CI jobs are not using find_latest_nightly()

Are these changes tested?

Yes (test added)

Are there any user-facing changes?

No

Closes: [R] Add test for new nixlibs.R #38430

github-actions · 2023-10-31T19:03:41Z

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

paleolimbot · 2023-11-02T13:26:17Z

@github-actions crossbow submit r-binary-packages

github-actions · 2023-11-02T13:29:12Z

Revision: 1c25889

Submitted crossbow builds: ursacomputing/crossbow @ actions-84648ca1ab

Task	Status
r-binary-packages

paleolimbot · 2023-11-03T16:07:24Z

@github-actions crossbow submit r-binary-packages

github-actions · 2023-11-03T16:09:40Z

Revision: 1e8d8f4

Submitted crossbow builds: ursacomputing/crossbow @ actions-c6ce43e763

Task	Status
r-binary-packages

paleolimbot · 2023-11-08T20:51:25Z

@assignUser Can you take a look at this?

I'd like to add in a comment regarding whether we do or do not intend to run detection when source()ing the nixlibs in the test-nixlibs.R. It's not a particularly good test if that is the intention, although I think it's fine if sourcing the file in test mode happens to run all the way though without warning.

assignUser

Some nice additions, looks good!

I think it makes sense to not look for latest nightly when sourceing the script. As that only happens for tests and there it makes more sense to test the function directly and not produce spurious output in the test file.

assignUser · 2023-11-09T04:21:55Z

r/tools/nixlibs.R

@@ -35,7 +35,9 @@ exit <- function(..., .status = 1) {


 # checks the nightly repo for the latest nightly version X.Y.Z.100<dev>
-find_latest_nightly <- function(description_version) {
+find_latest_nightly <- function(description_version,
+                                list_uri = "https://nightlies.apache.org/arrow/r/src/contrib/PACKAGES",


oh yeah that makes more sense ^^

assignUser · 2023-11-09T04:25:22Z

r/tools/nixlibs.R

@@ -867,6 +887,8 @@ build_ok <- !env_is("LIBARROW_BUILD", "false")
 #  https://arrow.apache.org/docs/developers/cpp/building.html#offline-builds)
 download_ok <- !test_mode && !env_is("TEST_OFFLINE_BUILD", "true")

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


Why this addition? Would not LIBARROW_BINARY=false do the same?

I forget what exactly it caused to fail, but I think the idea is that LIBARROW_BINARY controls whether or not static libs are attempted, sort of, and LIBARROW_DOWNLOAD strictly controls whether or not an http request is issued to get that binary. Mostly I just noticed that it was already set in the CI job ( https://github.com/apache/arrow/actions/runs/6801463295/job/18492381028?pr=38534#step:5:1094 ) and it was somewhat complicated to figure out how to wire up LIBARROW_BINARY because of the levels of abstraction on top of the R CI jobs.

Ah ok, fine with me.

Would you mind adding this to the other env-var documentation we have at

arrow/r/vignettes/install.Rmd

Lines 277 to 336 in 1f71014

#### R package configuration

There are a number of other variables that affect the `configure` script and

the bundled build script. All boolean variables are case-insensitive.

| Name | Description | Default |

| --- | --- | :-: |

| `LIBARROW_BUILD` | Allow building from source | `true` |

| `LIBARROW_BINARY` | Try to install `libarrow` binary instead of building from source | (unset) |

| `LIBARROW_MINIMAL` | Build with minimal features enabled | (unset) |

| `NOT_CRAN` | Set `LIBARROW_BINARY=true` and `LIBARROW_MINIMAL=false` | `false` |

| `ARROW_R_DEV` | More verbose messaging and regenerates some code | `false` |

| `ARROW_USE_PKG_CONFIG` | Use `pkg-config` to search for `libarrow` install | `true` |

| `LIBARROW_DEBUG_DIR` | Directory to save source build logs | (unset) |

| `CMAKE` | Alternative CMake path | (unset) |

See below for more in-depth explanations of these environment variables.

* `LIBARROW_BINARY` : By default on many distributions, or if explicitly set to

`true`, the script will determine whether there is a prebuilt libarrow that

will work with your system. You can set it to `false` to skip this option

altogether, or you can specify a string "distro-version" that corresponds to

a binary that is available, to override what this function may discover by

default. Possible values are: "linux-openssl-1.0", "linux-openssl-1.1",

"linux-openssl-3.0".

* `LIBARROW_BUILD` : If set to `false`, the build script

will not attempt to build the C++ from source. This means you will only get

a working arrow R package if a prebuilt binary is found.

Use this if you want to avoid compiling the C++ library, which may be slow

and resource-intensive, and ensure that you only use a prebuilt binary.

* `LIBARROW_MINIMAL` : If set to `false`, the build script

will enable some optional features, including S3

support and additional alternative memory allocators. This will increase the

source build time but results in a more fully functional library. If set to

`true` turns off Parquet, Datasets, compression libraries, and other optional

features. This is not commonly used but may be helpful if needing to compile

on a platform that does not support these features, e.g. Solaris.

* `NOT_CRAN` : If this variable is set to `true`, as the `devtools` package does,

the build script will set `LIBARROW_BINARY=true` and `LIBARROW_MINIMAL=false`

unless those environment variables are already set. This provides for a more

complete and fast installation experience for users who already have

`NOT_CRAN=true` as part of their workflow, without requiring additional

environment variables to be set.

* `ARROW_R_DEV` : If set to `true`, more verbose messaging will be printed

in the build script. `arrow::install_arrow(verbose = TRUE)` sets this.

This variable also is needed if you're modifying C++

code in the package: see the developer guide article.

* `ARROW_USE_PKG_CONFIG`: If set to `false`, the configure script won't look for

Arrow libraries on your system and instead will look to download/build them.

Use this if you have a version mismatch between installed system libraries and

the version of the R package you're installing.

* `LIBARROW_DEBUG_DIR` : If the C++ library building from source fails (`cmake`),

there may be messages telling you to check some log file in the build directory.

However, when the library is built during R package installation,

that location is in a temp directory that is already deleted.

To capture those logs, set this variable to an absolute (not relative) path

and the log files will be copied there.

The directory will be created if it does not exist.

* `CMAKE` : When building the C++ library from source, you can specify a

`/path/to/cmake` to use a different version than whatever is found on the `$PATH`.

?

Doing so might also clarify better for all of us the differences between these / how they work together

r/tools/nixlibs.R

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

paleolimbot · 2023-11-09T19:41:16Z

@github-actions crossbow submit r-binary-packages

github-actions · 2023-11-09T19:43:59Z

Revision: 371e003

Submitted crossbow builds: ursacomputing/crossbow @ actions-2269729e3b

Task	Status
r-binary-packages

paleolimbot · 2023-11-09T20:01:33Z

Hmm...I'm also getting

   *** Trying Arrow C++ in ARROW_HOME: /Users/deweydunnington/.r-arrow-dev-build/dist
   **** Not using: C++ library version (14.0.0-SNAPSHOT) does not match R package (14.0.0.9000)

locally, which is problematic!

paleolimbot · 2023-11-09T20:11:18Z

Oh just kidding...the C++ version bumped since the last time I rebuilt.

jonkeane · 2023-11-10T00:20:43Z

If the CI is good, I'm ok to merge this, though if you're looking for some other people to test this locally let me know and I'll pull it and try it (I will admit I have not yet!)

paleolimbot · 2023-11-10T02:00:18Z

Thank you for taking a look! I checked this locally and I'm comfortable merging 🙂

conbench-apache-arrow · 2023-11-10T08:14:17Z

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

There were no benchmark performance regressions. 🎉

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

jonkeane · 2023-11-10T14:48:14Z

Thanks for this!

…tor (apache#38534) ### Rationale for this change A few rough edges exist after apache#38236 including: - When zero or 1 nightly with the matching major version exist, detection of the latest nightly might fail - At least one CI job is pulling nightlies instead of using the version from the current commit ### What changes are included in this PR? - Clean up `find_latest_nightly()` + add test - Ensure all CI jobs are not using `find_latest_nightly()` ### Are these changes tested? Yes (test added) ### Are there any user-facing changes? No * Closes: apache#38430 Lead-authored-by: Dewey Dunnington <dewey@fishandwhistle.net> Co-authored-by: Jonathan Keane <jkeane@gmail.com> Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>

…38534) ### Rationale for this change A few rough edges exist after #38236 including: - When zero or 1 nightly with the matching major version exist, detection of the latest nightly might fail - At least one CI job is pulling nightlies instead of using the version from the current commit ### What changes are included in this PR? - Clean up `find_latest_nightly()` + add test - Ensure all CI jobs are not using `find_latest_nightly()` ### Are these changes tested? Yes (test added) ### Are there any user-facing changes? No * Closes: #38430 Lead-authored-by: Dewey Dunnington <dewey@fishandwhistle.net> Co-authored-by: Jonathan Keane <jkeane@gmail.com> Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>

…tor (apache#38534) ### Rationale for this change A few rough edges exist after apache#38236 including: - When zero or 1 nightly with the matching major version exist, detection of the latest nightly might fail - At least one CI job is pulling nightlies instead of using the version from the current commit ### What changes are included in this PR? - Clean up `find_latest_nightly()` + add test - Ensure all CI jobs are not using `find_latest_nightly()` ### Are these changes tested? Yes (test added) ### Are there any user-facing changes? No * Closes: apache#38430 Lead-authored-by: Dewey Dunnington <dewey@fishandwhistle.net> Co-authored-by: Jonathan Keane <jkeane@gmail.com> Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>

github-actions bot added Component: R awaiting committer review Awaiting committer review labels Oct 31, 2023

paleolimbot force-pushed the r-nixlibs-improve branch from 66e738a to 7d57b6a Compare November 1, 2023 01:08

paleolimbot marked this pull request as ready for review November 3, 2023 16:07

paleolimbot requested a review from assignUser as a code owner November 3, 2023 16:07

paleolimbot mentioned this pull request Nov 5, 2023

GH-38570: [R] Ensure that test-nix-libs is warning free #38571

Merged

3 tasks

paleolimbot force-pushed the r-nixlibs-improve branch from 1e8d8f4 to cb5f46b Compare November 7, 2023 18:01

paleolimbot requested a review from thisisnic as a code owner November 8, 2023 14:27

paleolimbot added 6 commits November 8, 2023 12:53

maybe clean up find_latest_nightly()

3c4dfb0

slightly better nightly testing

8218475

maybe default LIBARROW_BINARY to false

d045043

remove a debug

0709f30

maybe fix logic

ff398ed

ensure quiet nixlibs.R tests

0f2783f

paleolimbot force-pushed the r-nixlibs-improve branch from 488c0cd to 0f2783f Compare November 8, 2023 16:55

assignUser approved these changes Nov 9, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Nov 9, 2023

paleolimbot requested a review from jonkeane November 9, 2023 13:20

jonkeane reviewed Nov 9, 2023

View reviewed changes

r/tools/nixlibs.R Outdated Show resolved Hide resolved

jonkeane reviewed Nov 9, 2023

View reviewed changes

r/tools/nixlibs.R Outdated Show resolved Hide resolved

Apply suggestions from code review

1361154

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 9, 2023

document LIBARROW_DOWNLOAD

371e003

paleolimbot merged commit c260a24 into apache:main Nov 10, 2023
10 checks passed

paleolimbot removed the awaiting change review Awaiting change review label Nov 10, 2023

paleolimbot deleted the r-nixlibs-improve branch November 15, 2023 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor #38534

GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor #38534

paleolimbot commented Oct 31, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Oct 31, 2023

paleolimbot commented Nov 2, 2023

github-actions bot commented Nov 2, 2023

paleolimbot commented Nov 3, 2023

github-actions bot commented Nov 3, 2023

paleolimbot commented Nov 8, 2023

assignUser left a comment

assignUser Nov 9, 2023

assignUser Nov 9, 2023

paleolimbot Nov 9, 2023

assignUser Nov 9, 2023

jonkeane Nov 9, 2023

paleolimbot commented Nov 9, 2023

github-actions bot commented Nov 9, 2023

paleolimbot commented Nov 9, 2023

paleolimbot commented Nov 9, 2023

jonkeane commented Nov 10, 2023

paleolimbot commented Nov 10, 2023

conbench-apache-arrow bot commented Nov 10, 2023

jonkeane commented Nov 10, 2023

	#### R package configuration

	There are a number of other variables that affect the `configure` script and
	the bundled build script. All boolean variables are case-insensitive.

	\| Name \| Description \| Default \|
	\| --- \| --- \| :-: \|
	\| `LIBARROW_BUILD` \| Allow building from source \| `true` \|
	\| `LIBARROW_BINARY` \| Try to install `libarrow` binary instead of building from source \| (unset) \|
	\| `LIBARROW_MINIMAL` \| Build with minimal features enabled \| (unset) \|
	\| `NOT_CRAN` \| Set `LIBARROW_BINARY=true` and `LIBARROW_MINIMAL=false` \| `false` \|
	\| `ARROW_R_DEV` \| More verbose messaging and regenerates some code \| `false` \|
	\| `ARROW_USE_PKG_CONFIG` \| Use `pkg-config` to search for `libarrow` install \| `true` \|
	\| `LIBARROW_DEBUG_DIR` \| Directory to save source build logs \| (unset) \|
	\| `CMAKE` \| Alternative CMake path \| (unset) \|

	See below for more in-depth explanations of these environment variables.

	* `LIBARROW_BINARY` : By default on many distributions, or if explicitly set to
	`true`, the script will determine whether there is a prebuilt libarrow that
	will work with your system. You can set it to `false` to skip this option
	altogether, or you can specify a string "distro-version" that corresponds to
	a binary that is available, to override what this function may discover by
	default. Possible values are: "linux-openssl-1.0", "linux-openssl-1.1",
	"linux-openssl-3.0".
	* `LIBARROW_BUILD` : If set to `false`, the build script
	will not attempt to build the C++ from source. This means you will only get
	a working arrow R package if a prebuilt binary is found.
	Use this if you want to avoid compiling the C++ library, which may be slow
	and resource-intensive, and ensure that you only use a prebuilt binary.
	* `LIBARROW_MINIMAL` : If set to `false`, the build script
	will enable some optional features, including S3
	support and additional alternative memory allocators. This will increase the
	source build time but results in a more fully functional library. If set to
	`true` turns off Parquet, Datasets, compression libraries, and other optional
	features. This is not commonly used but may be helpful if needing to compile
	on a platform that does not support these features, e.g. Solaris.
	* `NOT_CRAN` : If this variable is set to `true`, as the `devtools` package does,
	the build script will set `LIBARROW_BINARY=true` and `LIBARROW_MINIMAL=false`
	unless those environment variables are already set. This provides for a more
	complete and fast installation experience for users who already have
	`NOT_CRAN=true` as part of their workflow, without requiring additional
	environment variables to be set.
	* `ARROW_R_DEV` : If set to `true`, more verbose messaging will be printed
	in the build script. `arrow::install_arrow(verbose = TRUE)` sets this.
	This variable also is needed if you're modifying C++
	code in the package: see the developer guide article.
	* `ARROW_USE_PKG_CONFIG`: If set to `false`, the configure script won't look for
	Arrow libraries on your system and instead will look to download/build them.
	Use this if you have a version mismatch between installed system libraries and
	the version of the R package you're installing.
	* `LIBARROW_DEBUG_DIR` : If the C++ library building from source fails (`cmake`),
	there may be messages telling you to check some log file in the build directory.
	However, when the library is built during R package installation,
	that location is in a temp directory that is already deleted.
	To capture those logs, set this variable to an absolute (not relative) path
	and the log files will be copied there.
	The directory will be created if it does not exist.
	* `CMAKE` : When building the C++ library from source, you can specify a
	`/path/to/cmake` to use a different version than whatever is found on the `$PATH`.

GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor #38534

GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor #38534

Conversation

paleolimbot commented Oct 31, 2023 • edited by github-actions bot 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 Oct 31, 2023

paleolimbot commented Nov 2, 2023

github-actions bot commented Nov 2, 2023

paleolimbot commented Nov 3, 2023

github-actions bot commented Nov 3, 2023

paleolimbot commented Nov 8, 2023

assignUser left a comment

Choose a reason for hiding this comment

assignUser Nov 9, 2023

Choose a reason for hiding this comment

assignUser Nov 9, 2023

Choose a reason for hiding this comment

paleolimbot Nov 9, 2023

Choose a reason for hiding this comment

assignUser Nov 9, 2023

Choose a reason for hiding this comment

jonkeane Nov 9, 2023

Choose a reason for hiding this comment

paleolimbot commented Nov 9, 2023

github-actions bot commented Nov 9, 2023

paleolimbot commented Nov 9, 2023

paleolimbot commented Nov 9, 2023

jonkeane commented Nov 10, 2023

paleolimbot commented Nov 10, 2023

conbench-apache-arrow bot commented Nov 10, 2023

jonkeane commented Nov 10, 2023

paleolimbot commented Oct 31, 2023 •

edited by github-actions bot

Loading