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

MINOR: [R] Avoid stray output from expr when checking for 10.13 #38303

Merged
merged 5 commits into from
Oct 17, 2023
Merged

MINOR: [R] Avoid stray output from expr when checking for 10.13 #38303

merged 5 commits into from
Oct 17, 2023

Conversation

assignUser
Copy link
Member

Rationale for this change

expr was printing the number of matching chars which showed up as noise in the log (which we want to avoid as much as possible to avoid any false positive checks)
See #38236 (comment) for @jonkeane's investigation.

What changes are included in this PR?

Replace use of expr with test.

Are these changes tested?

Crossbow

@assignUser assignUser added this to the 14.0.0 milestone Oct 17, 2023
@github-actions github-actions bot added the awaiting review Awaiting review label Oct 17, 2023
@assignUser
Copy link
Member Author

assignUser commented Oct 17, 2023 •
edited
Loading

@github-actions crossbow submit r-binary-packages

@assignUser assignUser mentioned this pull request Oct 17, 2023
Merged
@github-actions

This comment was marked as outdated.

Sign in to view
@assignUser
Copy link
Member Author

assignUser commented Oct 17, 2023 •
edited
Loading

The fix actually doesn't work^^ https://github.com/ursacomputing/crossbow/actions/runs/6541502691/job/17763126329#step:13:74 (the error is due to the missing flag that is set when the check is successfull)
Same code fails on 10.13
https://github.com/ursacomputing/crossbow/actions/runs/6541877833/job/17764033623

Edit:
It fails because it will return the exact version: e.g. 10.13.6 which isn't the same as 10.13. Which is of course the reason why I used expr but apparently I forgot ^^ 😅

@github-actions
Copy link

Revision: 1780fe4

Submitted crossbow builds: ursacomputing/crossbow @ actions-49d5b1c7dd

Task Status
r-binary-packages Github Actions

@assignUser
Copy link
Member Author

Works, no flag on arm build and no output https://github.com/ursacomputing/crossbow/actions/runs/6541940759/job/17764730231#step:17:61
Correct flag and no stray output on 10.13 https://github.com/ursacomputing/crossbow/actions/runs/6541940759/job/17764730131#step:17:61

@assignUser assignUser changed the title MINOR: [R] Don't use expr to check for 10.13 to avoid stray output MINOR: [R] Avoid stray output from expr when checking for 10.13 Oct 17, 2023
jonkeane
jonkeane requested changes Oct 17, 2023
View reviewed changes
Copy link
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

Would you mind adding a comment here so future-us knows why we're using expr + dumping the output?

r/configure
@@ -264,7 +264,7 @@ set_pkg_vars () {
PKG_CFLAGS="$PKG_CFLAGS $ARROW_R_CXXFLAGS"
fi

if [ "$UNAME" = "Darwin" ] && expr $(sw_vers -productVersion) : '10\.13'; then
if [ "$UNAME" = "Darwin" ] && expr $(sw_vers -productVersion) : '10\.13' >/dev/null 2>&1; then
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment here about why expr + why >/dev/null 2>&1 ?

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Oct 17, 2023
assignUser
assignUser commented Oct 17, 2023
View reviewed changes
r/configure Show resolved Hide resolved
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 17, 2023
@assignUser assignUser requested a review from jonkeane October 17, 2023 15:12
jonkeane
jonkeane reviewed Oct 17, 2023
View reviewed changes
r/configure Outdated Show resolved Hide resolved
@jonkeane
Update r/configure
053aae8 
Add both the rationale behind using expr
@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Oct 17, 2023
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 17, 2023
@jonkeane jonkeane merged commit 40571db into apache:main Oct 17, 2023
9 checks passed
@jonkeane jonkeane removed the awaiting merge Awaiting merge label Oct 17, 2023
@assignUser assignUser deleted the fix-expr-output branch October 17, 2023 15:23
raulcd pushed a commit that referenced this pull request Oct 17, 2023
@assignUser @jonkeane @raulcd
MINOR: [R] Avoid stray output from expr when checking for 10.13 (#38303)
b65c4f5 
### Rationale for this change

`expr` was printing the number of matching chars which showed up as noise in the log (which we want to avoid as much as possible to avoid any false positive checks)
See #38236 (comment) for @ jonkeane's investigation.

### What changes are included in this PR?

Replace use of expr with test.

### Are these changes tested?
Crossbow

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Jonathan Keane <[email protected]>
Signed-off-by: Jonathan Keane <[email protected]>
@conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

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

JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
@assignUser @jonkeane
MINOR: [R] Avoid stray output from expr when checking for 10.13 (apac…
afdb4a1 
…he#38303)

### Rationale for this change

`expr` was printing the number of matching chars which showed up as noise in the log (which we want to avoid as much as possible to avoid any false positive checks)
See apache#38236 (comment) for @ jonkeane's investigation.

### What changes are included in this PR?

Replace use of expr with test.

### Are these changes tested?
Crossbow

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Jonathan Keane <[email protected]>
Signed-off-by: Jonathan Keane <[email protected]>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
@assignUser @jonkeane @loicalleyne
MINOR: [R] Avoid stray output from expr when checking for 10.13 (apac…
bbbfe57 
…he#38303)

### Rationale for this change

`expr` was printing the number of matching chars which showed up as noise in the log (which we want to avoid as much as possible to avoid any false positive checks)
See apache#38236 (comment) for @ jonkeane's investigation.

### What changes are included in this PR?

Replace use of expr with test.

### Are these changes tested?
Crossbow

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Jonathan Keane <[email protected]>
Signed-off-by: Jonathan Keane <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@assignUser @jonkeane @dgreiss
MINOR: [R] Avoid stray output from expr when checking for 10.13 (apac…
70a28de 
…he#38303)

### Rationale for this change

`expr` was printing the number of matching chars which showed up as noise in the log (which we want to avoid as much as possible to avoid any false positive checks)
See apache#38236 (comment) for @ jonkeane's investigation.

### What changes are included in this PR?

Replace use of expr with test.

### Are these changes tested?
Crossbow

Lead-authored-by: Jacob Wujciak-Jens <[email protected]>
Co-authored-by: Jonathan Keane <[email protected]>
Signed-off-by: Jonathan Keane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonkeane jonkeane jonkeane approved these changes

@paleolimbot paleolimbot Awaiting requested review from paleolimbot

Assignees
No one assigned
Labels
Component: R
Projects
None yet
Milestone
14.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@assignUser @jonkeane