GH-38049: [R] Prevent `on_rosetta()` from warning #38052

jonkeane · 2023-10-05T19:09:19Z

Rationale for this change

Stop extraneous warnings.

What changes are included in this PR?

Prevent warning when detecting translation, but on x86

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, there will be fewer extraneous warnings.

Closes: [R] Rosetta detector has output when running on x86 #38049

jonkeane · 2023-10-05T19:15:18Z

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

github-actions · 2023-10-05T19:17:52Z

Revision: 58ff39b

Submitted crossbow builds: ursacomputing/crossbow @ actions-1c137994ff

Task	Status
test-r-install-local

jonkeane · 2023-10-05T20:12:57Z

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

github-actions · 2023-10-05T20:15:27Z

Revision: 673a604

Submitted crossbow builds: ursacomputing/crossbow @ actions-7c4a2d95fb

Task	Status
test-r-install-local

jonkeane · 2023-10-06T00:54:13Z

r/tests/testthat/test-arrow-pacakge.R

+  # There is no warning
+  expect_warning(on_rosetta(), NA)


I wasn't actually able to find a way to test that the console output from stderr doesn't show here. I tried both expect_output(..., NA) and expect_silent(...) as well as capturing the output with capture_output(...) and capture.output(...) but none of those seemed to actually capture the console warning I'm seeing. This is probably fine. I've tested locally and the output is supressed on my system.

paleolimbot

Thank you!

paleolimbot · 2023-10-06T12:47:27Z

(It floated in last night that the system("...", ignore.stdout = TRUE) == 1 might be sufficient if the return code is what's causing the warning. Either way!)

paleolimbot · 2023-10-06T14:20:49Z

I also checked this on 10.13 where it correctly silenced a warning that occurred as a result of a non-zero exit status/unknown old. Thanks again!

jonkeane · 2023-10-06T14:31:32Z

Thanks for merging.

For posterity only:

(It floated in last night that the system("...", ignore.stdout = TRUE) == 1 might be sufficient if the return code is what's causing the warning. Either way!)

We actually do need to inter to get the output. system("...", ignore.stdout = TRUE) == 1 will be 1 if you're on x86 (because the command failed to find sysctl.proc_translated), but 0 on an M* processor regardless if you're under emulation or not since the command succeeded at finding the property and printing 0 or 1 to the console depending on if you're in emulation or not. There might be a cleaner system2 incantation, but I didn't want to drift too far from what was there.

jonkeane · 2023-10-06T16:31:36Z

And, actually writting that out now made me realize I had introduced a bug resolved in #38083 I tested this code on x86 but not M*

### Rationale for this change A follow on to #38052 which introduced a small bug ### What changes are included in this PR? Don't ignore the stdout which is the mechanism to tell if under emulation. ### Are these changes tested? I tested these locally: On an M2 with emulation: ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) [1] "1" > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] TRUE ``` On an M2 without emulation: ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) [1] "0" > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] FALSE ``` On an x86 (note the warning message suppression is what #38052 resolved): ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) attr(,"status") [1] 1 Warning message: In system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, : running command 'sysctl -n sysctl.proc_translated >/dev/null 2>/dev/null' had status 1 > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) character(0) attr(,"status") [1] 1 Warning message: In system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) : running command 'sysctl -n sysctl.proc_translated 2>/dev/null' had status 1 > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] FALSE ``` ### Are there any user-facing changes? No / the functionality is restored Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

conbench-apache-arrow · 2023-10-08T01:11:43Z

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

There were no benchmark performance regressions. 🎉

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

### Rationale for this change Stop extraneous warnings. ### What changes are included in this PR? Prevent warning when detecting translation, but on x86 ### Are these changes tested? Yes ### Are there any user-facing changes? Yes, there will be fewer extraneous warnings. * Closes: apache#38049 Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

…#38083) ### Rationale for this change A follow on to apache#38052 which introduced a small bug ### What changes are included in this PR? Don't ignore the stdout which is the mechanism to tell if under emulation. ### Are these changes tested? I tested these locally: On an M2 with emulation: ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) [1] "1" > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] TRUE ``` On an M2 without emulation: ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) [1] "0" > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] FALSE ``` On an x86 (note the warning message suppression is what apache#38052 resolved): ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) attr(,"status") [1] 1 Warning message: In system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, : running command 'sysctl -n sysctl.proc_translated >/dev/null 2>/dev/null' had status 1 > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) character(0) attr(,"status") [1] 1 Warning message: In system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) : running command 'sysctl -n sysctl.proc_translated 2>/dev/null' had status 1 > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] FALSE ``` ### Are there any user-facing changes? No / the functionality is restored Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

### Rationale for this change Stop extraneous warnings. ### What changes are included in this PR? Prevent warning when detecting translation, but on x86 ### Are these changes tested? Yes ### Are there any user-facing changes? Yes, there will be fewer extraneous warnings. * Closes: apache#38049 Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

…#38083) ### Rationale for this change A follow on to apache#38052 which introduced a small bug ### What changes are included in this PR? Don't ignore the stdout which is the mechanism to tell if under emulation. ### Are these changes tested? I tested these locally: On an M2 with emulation: ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) [1] "1" > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] TRUE ``` On an M2 without emulation: ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) [1] "0" > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] FALSE ``` On an x86 (note the warning message suppression is what apache#38052 resolved): ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) attr(,"status") [1] 1 Warning message: In system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, : running command 'sysctl -n sysctl.proc_translated >/dev/null 2>/dev/null' had status 1 > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) character(0) attr(,"status") [1] 1 Warning message: In system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) : running command 'sysctl -n sysctl.proc_translated 2>/dev/null' had status 1 > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] FALSE ``` ### Are there any user-facing changes? No / the functionality is restored Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

### Rationale for this change Stop extraneous warnings. ### What changes are included in this PR? Prevent warning when detecting translation, but on x86 ### Are these changes tested? Yes ### Are there any user-facing changes? Yes, there will be fewer extraneous warnings. * Closes: apache#38049 Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

…#38083) ### Rationale for this change A follow on to apache#38052 which introduced a small bug ### What changes are included in this PR? Don't ignore the stdout which is the mechanism to tell if under emulation. ### Are these changes tested? I tested these locally: On an M2 with emulation: ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) [1] "1" > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] TRUE ``` On an M2 without emulation: ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) [1] "0" > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] FALSE ``` On an x86 (note the warning message suppression is what apache#38052 resolved): ``` > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE)), "1") [1] FALSE > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, ignore.stdout = TRUE) character(0) attr(,"status") [1] 1 Warning message: In system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE, : running command 'sysctl -n sysctl.proc_translated >/dev/null 2>/dev/null' had status 1 > system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) character(0) attr(,"status") [1] 1 Warning message: In system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE) : running command 'sysctl -n sysctl.proc_translated 2>/dev/null' had status 1 > identical(suppressWarnings(system("sysctl -n sysctl.proc_translated", intern = TRUE, ignore.stderr = TRUE)), "1") [1] FALSE ``` ### Are there any user-facing changes? No / the functionality is restored Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

A failing test

58ff39b

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

Suppress warnings, ignore out

673a604

jonkeane changed the title ~~GH-38049: [R] Prevent on_rosetta() from warning [WIP]~~ GH-38049: [R] Prevent on_rosetta() from warning Oct 6, 2023

jonkeane marked this pull request as ready for review October 6, 2023 00:52

jonkeane requested review from paleolimbot and thisisnic as code owners October 6, 2023 00:52

jonkeane commented Oct 6, 2023

View reviewed changes

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

paleolimbot approved these changes Oct 6, 2023

View reviewed changes

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

paleolimbot merged commit fdecb6a into apache:main Oct 6, 2023

paleolimbot removed the awaiting merge Awaiting merge label Oct 6, 2023

jonkeane mentioned this pull request Oct 6, 2023

MINOR: [R] Fix the error of ignoring stdout in on_rosetta() #38083

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38049: [R] Prevent `on_rosetta()` from warning #38052

GH-38049: [R] Prevent `on_rosetta()` from warning #38052

jonkeane commented Oct 5, 2023 •

edited by github-actions bot

Loading

jonkeane commented Oct 5, 2023

github-actions bot commented Oct 5, 2023

jonkeane commented Oct 5, 2023

github-actions bot commented Oct 5, 2023

jonkeane Oct 6, 2023

paleolimbot left a comment

paleolimbot commented Oct 6, 2023

paleolimbot commented Oct 6, 2023

jonkeane commented Oct 6, 2023

jonkeane commented Oct 6, 2023

conbench-apache-arrow bot commented Oct 8, 2023

GH-38049: [R] Prevent on_rosetta() from warning #38052

GH-38049: [R] Prevent on_rosetta() from warning #38052

Conversation

jonkeane commented Oct 5, 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?

jonkeane commented Oct 5, 2023

github-actions bot commented Oct 5, 2023

jonkeane commented Oct 5, 2023

github-actions bot commented Oct 5, 2023

jonkeane Oct 6, 2023

Choose a reason for hiding this comment

paleolimbot left a comment

Choose a reason for hiding this comment

paleolimbot commented Oct 6, 2023

paleolimbot commented Oct 6, 2023

jonkeane commented Oct 6, 2023

jonkeane commented Oct 6, 2023

conbench-apache-arrow bot commented Oct 8, 2023

GH-38049: [R] Prevent `on_rosetta()` from warning #38052

GH-38049: [R] Prevent `on_rosetta()` from warning #38052

jonkeane commented Oct 5, 2023 •

edited by github-actions bot

Loading