You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "assignUser (via GitHub)" <gi...@apache.org> on 2023/10/17 01:48:32 UTC
[PR] MINOR: [R] Don't use expr to check for 10.13 to avoid stray output [arrow]
assignUser opened a new pull request, #38303:
URL: https://github.com/apache/arrow/pull/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 https://github.com/apache/arrow/pull/38236#issuecomment-1761679457 for @jonkeane's investigation.
### What changes are included in this PR?
Replace use of expr with test.
### Are these changes tested?
Crossbow
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [R] Avoid stray output from expr when checking for 10.13 [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #38303:
URL: https://github.com/apache/arrow/pull/38303#discussion_r1362304131
##########
r/configure:
##########
@@ -264,7 +264,9 @@ set_pkg_vars () {
PKG_CFLAGS="$PKG_CFLAGS $ARROW_R_CXXFLAGS"
fi
- if [ "$UNAME" = "Darwin" ] && expr $(sw_vers -productVersion) : '10\.13'; then
+ # 'expr :' always outputs the number of matched characters, to avoid noise in the log we
+ # redirect the output to /dev/null
Review Comment:
```suggestion
# We use expr because the product version returns more than just 10.13 and we want to
# match the substring. However, expr always outputs the number of matched characters
# to stdout, to avoid noise in the log we redirect the output to /dev/null
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [R] Avoid stray output from expr when checking for 10.13 [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane merged PR #38303:
URL: https://github.com/apache/arrow/pull/38303
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [R] Don't use expr to check for 10.13 to avoid stray output [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38303:
URL: https://github.com/apache/arrow/pull/38303#issuecomment-1765529713
Revision: a56ce03d8dee80df24f077384db233e010224e3c
Submitted crossbow builds: [ursacomputing/crossbow @ actions-f1096656ab](https://github.com/ursacomputing/crossbow/branches/all?query=actions-f1096656ab)
|Task|Status|
|----|------|
|r-binary-packages|[](https://github.com/ursacomputing/crossbow/actions/runs/6541502691/job/17763127003)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [R] Don't use expr to check for 10.13 to avoid stray output [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38303:
URL: https://github.com/apache/arrow/pull/38303#issuecomment-1765582546
Revision: 1780fe45cf9b7c5fa6930f6c97eae5d26c06a354
Submitted crossbow builds: [ursacomputing/crossbow @ actions-49d5b1c7dd](https://github.com/ursacomputing/crossbow/branches/all?query=actions-49d5b1c7dd)
|Task|Status|
|----|------|
|r-binary-packages|[](https://github.com/ursacomputing/crossbow/actions/runs/6541940759/job/17764215303)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [R] Don't use expr to check for 10.13 to avoid stray output [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #38303:
URL: https://github.com/apache/arrow/pull/38303#issuecomment-1765561484
The fix actually doesn't work^^
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [R] Avoid stray output from expr when checking for 10.13 [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #38303:
URL: https://github.com/apache/arrow/pull/38303#discussion_r1362100419
##########
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
Review Comment:
Could you add a comment here about why expr + why `>/dev/null 2>&1` ?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [R] Don't use expr to check for 10.13 to avoid stray output [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #38303:
URL: https://github.com/apache/arrow/pull/38303#issuecomment-1765527185
@github-actions crossbow submit r-binary-packages
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [R] Avoid stray output from expr when checking for 10.13 [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38303:
URL: https://github.com/apache/arrow/pull/38303#issuecomment-1773205646
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 40571db03cc7f819f33a05dd421ef86816fe0502.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/17909918045) has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [R] Don't use expr to check for 10.13 to avoid stray output [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #38303:
URL: https://github.com/apache/arrow/pull/38303#issuecomment-1765695449
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
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [R] Avoid stray output from expr when checking for 10.13 [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #38303:
URL: https://github.com/apache/arrow/pull/38303#discussion_r1362294806
##########
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
Review Comment:
```suggestion
# 'expr :' always outputs the number of matched characters, to avoid noise in the log we
# redirect the output to /dev/null
if [ "$UNAME" = "Darwin" ] && expr $(sw_vers -productVersion) : '10\.13' >/dev/null 2>&1; then
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org