GH-37907: [R] Setting rosetta variable is missing #37961

fernandomayer · 2023-09-30T00:19:20Z

Rationale for this change

The latest version of r/R/install-arrow.R was not working properly, since it was relying on the on_rosetta() function, which is not defined elsewhere. I just fixed the identification of rosetta in the script.

With the current code, the following gives an error

> source("https://github.com/apache/arrow/master/r/R/install-arrow.R") 
> install_arrow()
Error in on_rosetta() : could not find function "on_rosetta"

What changes are included in this PR?

It only removed the on_rosetta() function, which was not defined elsewhere, and reverted back to the rosetta object to identify if rosetta is present or not on a user's system.

Are these changes tested?

Yes. It was tested with the current code and the proposed PR. The proposed PR works as expected.

Are there any user-facing changes?

No.

Closes: [R] Setting rosetta variable is missing #37907

thisisnic · 2023-10-02T12:42:02Z

Thanks for making this PR! One small change, otherwise looks good to go :)

thisisnic · 2023-10-02T12:39:56Z

r/R/install-arrow.R

@@ -61,6 +61,7 @@ install_arrow <- function(nightly = FALSE,
                          verbose = Sys.getenv("ARROW_R_DEV", FALSE),
                          repos = getOption("repos"),
                          ...) {
+  sysname <- tolower(Sys.info()[["sysname"]])


Suggested change

sysname <- tolower(Sys.info()[["sysname"]])

thisisnic · 2023-10-02T12:41:05Z

r/R/install-arrow.R

+    rosetta <- identical(sysname, "darwin") && identical(system("sysctl -n sysctl.proc_translated", intern = TRUE), "1")
+    if (rosetta) {


Suggested change

rosetta <- identical(sysname, "darwin") && identical(system("sysctl -n sysctl.proc_translated", intern = TRUE), "1")

if (rosetta) {

rosetta <- on_rosetta()

if (rosetta) {

Another PR implemented a function on_rosetta() that already does these checks, so we can simplify this bit here, and remove the creation of the sysname variable above.

Hi @thisisnic

I believe that for the package this is fine. I saw that the on_rosetta() function was defined in r/R/arrow-package.R and that should work.

However, I'm thinking of the case where the user just uses the r/R/install-arrow.R as a standalone script, as suggested here (as it's the way I use, for example). In this way, the on_rosetta() function is not found, as the script is not self-contained anymore.

So my proposal would be to just change the on_rosetta() function from r/R/arrow-package.R to r/R/install-arrow.R, as this would not affect the former, but it would make the latter self-contained again.

I don't see the standalone use being suggested, maybe we need to clarify the docs? As I understand it install_arrow is for use when you have the arrow package installed but want to do one of the following:

You have arrow installed and want to upgrade to a different version You want to try to reinstall and fix issues with Linux C++ binaries You want to install a development build

@thisisnic please correct me if my assessment on that is wrong?

@assignUser It's here: https://arrow.apache.org/docs/r/articles/install.html?q=install_arrow#using-install_arrow

Although this function is part of the arrow package, it is also available as a standalone script, so you can access it without first installing the package:

In the case of "You want to install a development build", folks may not already have arrow installed.

On a different note, there have been other PRs in this area of the codebase merged recently, so this PR may need a rebase to see how the changes affect it, and what needs doing.

So my proposal would be to just change the on_rosetta() function from r/R/arrow-package.R to r/R/install-arrow.R, as this would not affect the former, but it would make the latter self-contained again.

This is a good catch, I'm working on a review, but overall I'm pro moving this function to install-arrow.R so that it remains self-contained. Thank you for catching it!

In this way, the on_rosetta() function is not found, as the script is not self-contained anymore.

We don't have to do this here, but we should at least make an issue that we should write a test that tests that the script remains self-contained. It would have caught this bug, for example!

assignUser · 2023-10-10T20:06:19Z

@thisisnic should we merge this before the release?

thisisnic · 2023-10-10T21:25:04Z

@jonkeane You most recently worked on the code that this PR touches and I'm not sure where is best for this function - I don't suppose you'd mind giving this a quick look over?

thisisnic · 2023-10-10T21:26:07Z

@thisisnic should we merge this before the release?

Yes, but it'll need a final change before merging

jonkeane

Thanks @fernandomayer for the catch + the fix. Like I mentioned in a comment up above, I'm supportive of moving the function definition to arrow-install.R so that it can be self contained again.

Would you mind also moving the test for this from test-arrow-package.R (and delete that file since it's the only test there) and put the test in test-install-arrow.R ?

Thanks!

jonkeane · 2023-10-10T21:56:23Z

r/R/install-arrow.R

+
+on_rosetta <- function() {
+  identical(tolower(Sys.info()[["sysname"]]), "darwin") &&
+    identical(system("sysctl -n sysctl.proc_translated", intern = TRUE), "1")


We do actually need / want to suppressWarnings() and ignore.stderr = TRUE here as well. On x86 machines this call will throw a warning + print to the console complaining that it can't find the proc_translated argument there.

Suggested change

identical(system("sysctl -n sysctl.proc_translated", intern = TRUE), "1")

identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE), "1")

jonkeane · 2023-10-10T21:57:07Z

r/R/install-arrow.R

@@ -267,3 +268,8 @@ wslify_path <- function(path) {
  end_path <- strsplit(path, drive_expr)[[1]][-1]
  file.path(wslified_drive, end_path)
 }
+
+on_rosetta <- function() {
+  identical(tolower(Sys.info()[["sysname"]]), "darwin") &&


It would be nice to also keep this comment (though edited a bit for clarity):

Suggested change

identical(tolower(Sys.info()[["sysname"]]), "darwin") &&

# make sure to suppress warnings and ignore the stderr so that this is silent on x86

identical(tolower(Sys.info()[["sysname"]]), "darwin") &&

jonkeane · 2023-10-10T22:00:18Z

r/R/install-arrow.R

+    rosetta <- identical(sysname, "darwin") && identical(system("sysctl -n sysctl.proc_translated", intern = TRUE), "1")
+    if (rosetta) {


So my proposal would be to just change the on_rosetta() function from r/R/arrow-package.R to r/R/install-arrow.R, as this would not affect the former, but it would make the latter self-contained again.

This is a good catch, I'm working on a review, but overall I'm pro moving this function to install-arrow.R so that it remains self-contained. Thank you for catching it!

In this way, the on_rosetta() function is not found, as the script is not self-contained anymore.

We don't have to do this here, but we should at least make an issue that we should write a test that tests that the script remains self-contained. It would have caught this bug, for example!

assignUser · 2023-10-11T02:29:19Z

It looks like this would also fix a nightly fail/cran warning: https://github.com/ursacomputing/crossbow/actions/runs/6463099167/job/17545762151#step:6:3794

jonkeane · 2023-10-12T20:22:00Z

I hope you don't mind that I pushed those changes requested to this branch so that we can get this in before the upcoming release.

jonkeane · 2023-10-12T20:22:04Z

@github-actions crossbow submit test-r-install-local

jonkeane · 2023-10-12T20:23:19Z

Once the crossbow build is done I'm going to merge this, unless there are objections

github-actions · 2023-10-12T20:24:54Z

Revision: dfec06a

Submitted crossbow builds: ursacomputing/crossbow @ actions-efacb091fb

Task	Status
test-r-install-local

jonkeane · 2023-10-12T20:28:55Z

I've also created #38251 to track adding a test for this self-containedness

r/R/install-arrow.R

### Rationale for this change The latest version of `r/R/install-arrow.R` was not working properly, since it was relying on the `on_rosetta()` function, which is not defined elsewhere. I just fixed the identification of rosetta in the script. With the current code, the following gives an error ````r > source("https://github.com/apache/arrow/master/r/R/install-arrow.R") > install_arrow() Error in on_rosetta() : could not find function "on_rosetta" ```` ### What changes are included in this PR? It only removed the `on_rosetta()` function, which was not defined elsewhere, and reverted back to the `rosetta` object to identify if rosetta is present or not on a user's system. ### Are these changes tested? Yes. It was tested with the current code and the proposed PR. The proposed PR works as expected. ### Are there any user-facing changes? No. * Closes: #37907 Lead-authored-by: Fernando Mayer <fernandomayer@gmail.com> Co-authored-by: Jonathan Keane <jkeane@gmail.com> Signed-off-by: Nic Crane <thisisnic@gmail.com>

fernandomayer · 2023-10-13T14:02:55Z

Thanks @jonkeane and @thisisnic

I can confirm that now the command

source("https://github.com/apache/arrow/main/r/R/install-arrow.R")
install_arrow()

works as expected again, as stated in the documentation https://arrow.apache.org/docs/r/articles/install.html?q=install_arrow#using-install_arrow

jonkeane · 2023-10-13T14:05:30Z

Thanks again @fernandomayer for the issue + PR

conbench-apache-arrow · 2023-10-17T01:44:52Z

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

There were no benchmark performance regressions. 🎉

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

### Rationale for this change The latest version of `r/R/install-arrow.R` was not working properly, since it was relying on the `on_rosetta()` function, which is not defined elsewhere. I just fixed the identification of rosetta in the script. With the current code, the following gives an error ````r > source("https://github.com/apache/arrow/master/r/R/install-arrow.R") > install_arrow() Error in on_rosetta() : could not find function "on_rosetta" ```` ### What changes are included in this PR? It only removed the `on_rosetta()` function, which was not defined elsewhere, and reverted back to the `rosetta` object to identify if rosetta is present or not on a user's system. ### Are these changes tested? Yes. It was tested with the current code and the proposed PR. The proposed PR works as expected. ### Are there any user-facing changes? No. * Closes: apache#37907 Lead-authored-by: Fernando Mayer <fernandomayer@gmail.com> Co-authored-by: Jonathan Keane <jkeane@gmail.com> Signed-off-by: Nic Crane <thisisnic@gmail.com>

fernandomayer added 2 commits September 30, 2023 00:59

Revert the rosetta verification

7bb3433

Merge branch 'apache:main' into main

5a691a3

fernandomayer requested review from paleolimbot and thisisnic as code owners September 30, 2023 00:19

github-actions bot added Component: R awaiting review Awaiting review labels Sep 30, 2023

thisisnic mentioned this pull request Oct 2, 2023

GH-37907: [R] Setting rosetta variable is missing #37973

Closed

thisisnic changed the title ~~MINOR: [R] Failing installation script~~ GH-37907: [R] Setting rosetta variable is missing Oct 2, 2023

thisisnic requested changes Oct 2, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Oct 2, 2023

Merge branch 'main' of github.com:apache/arrow

945ca55

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 3, 2023

fernandomayer added 2 commits October 3, 2023 18:10

Move on_rosetta() to install-arrow.R script

b325ccc

Merge branch 'main' into rosetta

19684dd

thisisnic requested a review from jonkeane October 10, 2023 21:25

jonkeane requested changes Oct 10, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 10, 2023

Re-add silencing of rosetta detection, move test

dfec06a

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 12, 2023

jonkeane reviewed Oct 12, 2023

View reviewed changes

r/R/install-arrow.R Outdated Show resolved Hide resolved

Appease the linter

793807e

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 12, 2023

jonkeane approved these changes Oct 12, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Oct 12, 2023

thisisnic approved these changes Oct 12, 2023

View reviewed changes

thisisnic merged commit 7b7bbdc into apache:main Oct 12, 2023
12 checks passed

thisisnic removed the awaiting merge Awaiting merge label Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37907: [R] Setting rosetta variable is missing #37961

GH-37907: [R] Setting rosetta variable is missing #37961

fernandomayer commented Sep 30, 2023 •

edited by github-actions bot

Loading

thisisnic commented Oct 2, 2023

thisisnic Oct 2, 2023

thisisnic Oct 2, 2023

fernandomayer Oct 3, 2023 •

edited

Loading

assignUser Oct 5, 2023

thisisnic Oct 9, 2023

jonkeane Oct 10, 2023

assignUser commented Oct 10, 2023

thisisnic commented Oct 10, 2023

thisisnic commented Oct 10, 2023

jonkeane left a comment

jonkeane Oct 10, 2023

jonkeane Oct 10, 2023

jonkeane Oct 10, 2023

assignUser commented Oct 11, 2023

jonkeane commented Oct 12, 2023

jonkeane commented Oct 12, 2023

jonkeane commented Oct 12, 2023

github-actions bot commented Oct 12, 2023

jonkeane commented Oct 12, 2023

fernandomayer commented Oct 13, 2023

jonkeane commented Oct 13, 2023

conbench-apache-arrow bot commented Oct 17, 2023

		rosetta <- identical(sysname, "darwin") && identical(system("sysctl -n sysctl.proc_translated", intern = TRUE), "1")
		if (rosetta) {

	identical(system("sysctl -n sysctl.proc_translated", intern = TRUE), "1")
	identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE), "1")

	identical(tolower(Sys.info()[["sysname"]]), "darwin") &&
	# make sure to suppress warnings and ignore the stderr so that this is silent on x86
	identical(tolower(Sys.info()[["sysname"]]), "darwin") &&

GH-37907: [R] Setting rosetta variable is missing #37961

GH-37907: [R] Setting rosetta variable is missing #37961

Conversation

fernandomayer commented Sep 30, 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?

thisisnic commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandomayer Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assignUser commented Oct 10, 2023

thisisnic commented Oct 10, 2023

thisisnic commented Oct 10, 2023

jonkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assignUser commented Oct 11, 2023

jonkeane commented Oct 12, 2023

jonkeane commented Oct 12, 2023

jonkeane commented Oct 12, 2023

github-actions bot commented Oct 12, 2023

jonkeane commented Oct 12, 2023

fernandomayer commented Oct 13, 2023

jonkeane commented Oct 13, 2023

conbench-apache-arrow bot commented Oct 17, 2023

fernandomayer commented Sep 30, 2023 •

edited by github-actions bot

Loading

fernandomayer Oct 3, 2023 •

edited

Loading