GH-37208: [R] Use currrently running R binary to compile test program (nix install) #37225

meztez · 2023-08-17T17:59:54Z

Rationale for this change

See #37208.

What changes are included in this PR?

Use the currrently running R binary to compile test program (nix install).

Are these changes tested?

No. Installation.

Are there any user-facing changes?

No

Closes: [R] Arrow package install - R binary version mismatch #37208

github-actions · 2023-08-17T18:00:25Z

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

kou · 2023-08-18T00:30:38Z

r/tools/nixlibs.R

@@ -197,7 +197,9 @@ compile_test_program <- function(code) {
  # Note: if we wanted to check for openssl on macOS, we'd have to set the brew
  # path as a -I directory. But since we (currently) only run this code to
  # determine whether we can download a Linux binary, it's not relevant.
-  runner <- "`R CMD config CXX17` `R CMD config CPPFLAGS` `R CMD config CXX17FLAGS` `R CMD config CXX17STD` -E -xc++"
+  r_exec <- paste(R.home(component = "bin"), "R CMD config", sep="/")


Can we use R_CMD_config() here?

arrow/r/tools/nixlibs.R

Lines 380 to 382 in 43c05c5

R_CMD_config <- function(var) {

tools::Rcmd(paste("config", var), stdout = TRUE)

}

The original author did not use it.
Maybe because all 4 commands need to run at the same time?
I would not know.

@nealrichardson Can we use R_CMD_config() here?

I don't see why not, and I dug through the PR from last year when this was added and don't see any reason why it has to be this way.

kou · 2023-08-18T00:31:57Z

Could you use our PR template instead of removing it entirely?
(You must see it when you open this PR.)

assignUser · 2023-08-18T06:37:01Z

@github-actions crossbow submit -g r

github-actions · 2023-08-18T06:41:58Z

Revision: bd296ce

Submitted crossbow builds: ursacomputing/crossbow @ actions-b2e2361c9a

Task	Status
conda-linux-aarch64-cpu-r41
conda-linux-aarch64-cpu-r42
conda-linux-x64-cpu-r41
conda-linux-x64-cpu-r42
conda-osx-arm64-cpu-r41
conda-osx-arm64-cpu-r42
conda-osx-x64-cpu-r41
conda-osx-x64-cpu-r42
conda-win-x64-cpu-r41
homebrew-r-autobrew
homebrew-r-brew
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

…#37236) ### Rationale for this change To simplify the implementation of `Time32Type`, `Time64Type`, `Date32Type`, and `Date64Type`, it would be helpful to create an abstract `arrow.type.TemporalType` class that the date and time types can inherit from. As a first step, we can modify the implementation of `TimestampType` to inherit from `TemporalType`. This mimics the class inheritance hierarchy from the C++ libraries. ### What changes are included in this PR? 1. Added a new MATLAB class called `arrow.type.TemporalType`. It inherits from `arrow.type.FixedWidthType` and defines one read-only property: `TimeUnit`. 2. Modified the MATLAB class `arrow.type.TimestampType` to inherit from `arrow.type.TemporalType` instead of `arrow.type.FixedWidthType`. 3. Renamed the C++ `proxy::TimestampType` methods `timeUnit()` and `timeZone()` to `getTimeUnit()` and `getTimeZone()`. ### Are these changes tested? Yes. The existing tests in `tTimetampType.m` cover these changes. ### Are there any user-facing changes? No. ### Future Directions 1. Add `arrow.type.Time32Type` (#37231) 2. Add `arrow.type.Time64Type` (#37225) 3. Add `arrow.type.Date32Type` (#37229), 4. Add `arrow.type.Time64Type` (#37230). 5. Add `arrow.array.Time32Array` 6. Add `arrow.array.Time64Array` 7. Add `arrow.array.Date32Array` 8. Add `arrow.array.Date64Array` * Closes: #37234 Lead-authored-by: Sarah Gilmore <sgilmore@mathworks.com> Co-authored-by: Kevin Gurney <kgurney@mathworks.com> Signed-off-by: Kevin Gurney <kgurney@mathworks.com>

r/tools/nixlibs.R

Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>

kou · 2023-08-24T03:02:25Z

@github-actions crossbow submit -g r

github-actions · 2023-08-24T03:07:18Z

Revision: ced8973

Submitted crossbow builds: ursacomputing/crossbow @ actions-bf7a320d75

Task	Status
conda-linux-aarch64-cpu-r41
conda-linux-aarch64-cpu-r42
conda-linux-x64-cpu-r41
conda-linux-x64-cpu-r42
conda-osx-arm64-cpu-r41
conda-osx-arm64-cpu-r42
conda-osx-x64-cpu-r41
conda-osx-x64-cpu-r42
conda-win-x64-cpu-r41
homebrew-r-autobrew
homebrew-r-brew
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

kou

+1

conbench-apache-arrow · 2023-08-28T09:27:46Z

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

There were no benchmark performance regressions. 🎉

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

… class (apache#37236) ### Rationale for this change To simplify the implementation of `Time32Type`, `Time64Type`, `Date32Type`, and `Date64Type`, it would be helpful to create an abstract `arrow.type.TemporalType` class that the date and time types can inherit from. As a first step, we can modify the implementation of `TimestampType` to inherit from `TemporalType`. This mimics the class inheritance hierarchy from the C++ libraries. ### What changes are included in this PR? 1. Added a new MATLAB class called `arrow.type.TemporalType`. It inherits from `arrow.type.FixedWidthType` and defines one read-only property: `TimeUnit`. 2. Modified the MATLAB class `arrow.type.TimestampType` to inherit from `arrow.type.TemporalType` instead of `arrow.type.FixedWidthType`. 3. Renamed the C++ `proxy::TimestampType` methods `timeUnit()` and `timeZone()` to `getTimeUnit()` and `getTimeZone()`. ### Are these changes tested? Yes. The existing tests in `tTimetampType.m` cover these changes. ### Are there any user-facing changes? No. ### Future Directions 1. Add `arrow.type.Time32Type` (apache#37231) 2. Add `arrow.type.Time64Type` (apache#37225) 3. Add `arrow.type.Date32Type` (apache#37229), 4. Add `arrow.type.Time64Type` (apache#37230). 5. Add `arrow.array.Time32Array` 6. Add `arrow.array.Time64Array` 7. Add `arrow.array.Date32Array` 8. Add `arrow.array.Date64Array` * Closes: apache#37234 Lead-authored-by: Sarah Gilmore <sgilmore@mathworks.com> Co-authored-by: Kevin Gurney <kgurney@mathworks.com> Signed-off-by: Kevin Gurney <kgurney@mathworks.com>

…rogram (nix install) (apache#37225) ### Rationale for this change See apache#37208. ### What changes are included in this PR? Use the currrently running R binary to compile test program (nix install). ### Are these changes tested? No. Installation. ### Are there any user-facing changes? No * Closes: apache#37208 Lead-authored-by: Bruno Tremblay <bruno@boostao.ca> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Use currrently running R binary to compile test program (nix install)

247eb24

meztez requested a review from assignUser as a code owner August 17, 2023 17:59

github-actions bot added Component: R awaiting review Awaiting review labels Aug 17, 2023

thisisnic requested a review from paleolimbot August 17, 2023 19:47

sgilmore10 mentioned this pull request Aug 17, 2023

GH-37234: [MATLAB] Create an abstract arrow.type.TemporalType class #37236

Merged

kou reviewed Aug 18, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Aug 18, 2023

use file path

bd296ce

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 18, 2023

nealrichardson reviewed Aug 19, 2023

View reviewed changes

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

kevingurney assigned kevingurney and unassigned kevingurney Aug 21, 2023

Use R_CMD_config()

ced8973

Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 24, 2023

kou approved these changes Aug 28, 2023

View reviewed changes

kou merged commit f27dc02 into apache:main Aug 28, 2023
11 checks passed

kou removed the awaiting change review Awaiting change review label Aug 28, 2023

github-actions bot added the awaiting merge Awaiting merge label Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37208: [R] Use currrently running R binary to compile test program (nix install) #37225

GH-37208: [R] Use currrently running R binary to compile test program (nix install) #37225

meztez commented Aug 17, 2023 •

edited

github-actions bot commented Aug 17, 2023

kou Aug 18, 2023

meztez Aug 18, 2023

kou Aug 18, 2023

nealrichardson Aug 19, 2023

kou commented Aug 18, 2023

assignUser commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

kou commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

kou left a comment

conbench-apache-arrow bot commented Aug 28, 2023

	R_CMD_config <- function(var) {
	tools::Rcmd(paste("config", var), stdout = TRUE)
	}

GH-37208: [R] Use currrently running R binary to compile test program (nix install) #37225

GH-37208: [R] Use currrently running R binary to compile test program (nix install) #37225

Conversation

meztez commented Aug 17, 2023 • edited

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 Aug 17, 2023

kou Aug 18, 2023

Choose a reason for hiding this comment

meztez Aug 18, 2023

Choose a reason for hiding this comment

kou Aug 18, 2023

Choose a reason for hiding this comment

nealrichardson Aug 19, 2023

Choose a reason for hiding this comment

kou commented Aug 18, 2023

assignUser commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

kou commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

kou left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Aug 28, 2023

meztez commented Aug 17, 2023 •

edited