Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 7 commits into from
Oct 12, 2023
Merged

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

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7bb3433
Revert the rosetta verification
fernandomayer Sep 29, 2023
5a691a3
Merge branch 'apache:main' into main
fernandomayer Sep 30, 2023
945ca55
Merge branch 'main' of github.com:apache/arrow
fernandomayer Oct 3, 2023
b325ccc
Move on_rosetta() to install-arrow.R script
fernandomayer Oct 3, 2023
19684dd
Merge branch 'main' into rosetta
fernandomayer Oct 9, 2023
dfec06a
Re-add silencing of rosetta detection, move test
jonkeane Oct 12, 2023
793807e
Appease the linter
jonkeane Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion r/R/install-arrow.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ install_arrow <- function(nightly = FALSE,
verbose = Sys.getenv("ARROW_R_DEV", FALSE),
repos = getOption("repos"),
...) {
sysname <- tolower(Sys.info()[["sysname"]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

conda <- isTRUE(grepl("conda", R.Version()$platform))

if (conda) {
Expand All @@ -79,7 +80,8 @@ install_arrow <- function(nightly = FALSE,
# On the M1, we can't use the usual autobrew, which pulls Intel dependencies
apple_m1 <- grepl("arm-apple|aarch64.*darwin", R.Version()$platform)
# On Rosetta, we have to build without JEMALLOC, so we also can't autobrew
if (on_rosetta()) {
rosetta <- identical(sysname, "darwin") && identical(system("sysctl -n sysctl.proc_translated", intern = TRUE), "1")
if (rosetta) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@fernandomayer fernandomayer Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Sys.setenv(ARROW_JEMALLOC = "OFF")
}
if (apple_m1 || rosetta) {
Expand Down
Loading