jonkeane commented on code in PR #37961:
URL: https://github.com/apache/arrow/pull/37961#discussion_r1353475618
##########
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") &&
+ identical(system("sysctl -n sysctl.proc_translated", intern = TRUE), "1")
Review Comment:
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.
```suggestion
identical(suppressWarnings(system("sysctl -n sysctl.proc_translated",
intern = TRUE, ignore.stderr = TRUE), "1")
```
##########
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") &&
Review Comment:
It would be nice to also keep this comment (though edited a bit for
clarity):
```suggestion
# make sure to suppress warnings and ignore the stderr so that this is
silent on x86
identical(tolower(Sys.info()[["sysname"]]), "darwin") &&
```
##########
r/R/install-arrow.R:
##########
@@ -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) {
Review Comment:
> 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!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
