You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "hiroyuki-sato (via GitHub)" <gi...@apache.org> on 2026/08/04 02:29:27 UTC
[PR] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
hiroyuki-sato opened a new pull request, #50798:
URL: https://github.com/apache/arrow/pull/50798
### Rationale for this change
This is the sub issue #44748.
* SC2046: Quote this to prevent word splitting.
* SC2086: Double quote to prevent globbing and word splitting.
* SC2223: This default assignment may cause DoS due to globbing. Quote it.
```
shellcheck ci/scripts/r_valgrind.sh
In ci/scripts/r_valgrind.sh line 21:
: ${R_BIN:=RDvalgrind}
^------------------^ SC2223 (info): This default assignment may cause DoS due to globbing. Quote it.
In ci/scripts/r_valgrind.sh line 27:
pushd ${source_dir}
^-----------^ SC2086 (info): Double quote to prevent globbing and word splitting.
Did you mean:
pushd "${source_dir}"
In ci/scripts/r_valgrind.sh line 31:
${R_BIN} CMD INSTALL ${INSTALL_ARGS} arrow*.tar.gz
^-------------^ SC2086 (info): Double quote to prevent globbing and word splitting.
Did you mean:
${R_BIN} CMD INSTALL "${INSTALL_ARGS}" arrow*.tar.gz
In ci/scripts/r_valgrind.sh line 42:
if [ $(grep -c "ERROR SUMMARY: 0 errors" testthat.out) != 1 ]; then
^-- SC2046 (warning): Quote this to prevent word splitting.
For more information:
https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt...
https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...
https://www.shellcheck.net/wiki/SC2223 -- This default assignment may cause...
```
### What changes are included in this PR?
* SC2046: Quote variable to prevent word splitting.
* SC2086: Quote variable
* SC2223: Quote default variable assignments.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No.
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50798:
URL: https://github.com/apache/arrow/pull/50798#issuecomment-5173916565
:warning: GitHub issue #50796 **has been automatically assigned in GitHub** to PR creator.
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "hiroyuki-sato (via GitHub)" <gi...@apache.org>.
hiroyuki-sato commented on code in PR #50798:
URL: https://github.com/apache/arrow/pull/50798#discussion_r3717060967
##########
ci/scripts/r_valgrind.sh:
##########
@@ -18,28 +18,32 @@
set -ex
-: ${R_BIN:=RDvalgrind}
+: "${R_BIN:=RDvalgrind}"
-source_dir=${1}/r
+source_dir="${1}/r"
export CMAKE_BUILD_TYPE=RelWithDebInfo
-pushd ${source_dir}
+pushd "${source_dir}"
+
+# Convert the space-separated options into a Bash array.
+# This avoids ShellCheck SC2086 and preserves argument boundaries.
+read -r -a R_INSTALL_ARGS <<< "${INSTALL_ARGS:-}"
# build first so that any stray compiled files in r/src are ignored
-${R_BIN} CMD build --no-build-vignettes .
-${R_BIN} CMD INSTALL ${INSTALL_ARGS} arrow*.tar.gz
+"${R_BIN}" CMD build --no-build-vignettes .
+"${R_BIN}" CMD INSTALL "${R_INSTALL_ARGS[@]}" arrow*.tar.gz
pushd tests
# to generate suppression files run:
# ${R_BIN} --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --gen-suppressions=all --log-file=memcheck.log" -f testthat.R
-${R_BIN} --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --suppressions=/${1}/ci/etc/valgrind-cran.supp" -f testthat.R |& tee testthat.out
+"${R_BIN}" --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --suppressions=/${1}/ci/etc/valgrind-cran.supp" -f testthat.R |& tee testthat.out
# valgrind --error-exitcode=1 should return an erroring exit code that we can catch,
# but R eats that and returns 0, so we need to look at the output and make sure that
# we have 0 errors instead.
-if [ $(grep -c "ERROR SUMMARY: 0 errors" testthat.out) != 1 ]; then
+if [ "$(grep -c "ERROR SUMMARY: 0 errors" testthat.out)" -ne 1 ]; then
Review Comment:
Thanks for the suggestion. I'll adopt that approach.
I initially considered using `grep -q` as well, but I went with the current implementation because I wasn't sure whether the fact that there is exactly one match was significant.
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50798:
URL: https://github.com/apache/arrow/pull/50798#issuecomment-5174174047
Revision: 397dd4674e4612148d887db5b848972d513ac519
Submitted crossbow builds: [ursacomputing/crossbow @ actions-4f596c72c9](https://github.com/ursacomputing/crossbow/branches/all?query=actions-4f596c72c9)
|Task|Status|
|----|------|
|test-r-linux-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/30874057795/job/91881785985)|
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50798:
URL: https://github.com/apache/arrow/pull/50798#issuecomment-5186051530
Revision: 3c3e5779ac2a0177b4c578d787d18c48fcb9fb38
Submitted crossbow builds: [ursacomputing/crossbow @ actions-3134197238](https://github.com/ursacomputing/crossbow/branches/all?query=actions-3134197238)
|Task|Status|
|----|------|
|test-r-linux-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/30962825049/job/92170229278)|
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50798:
URL: https://github.com/apache/arrow/pull/50798#issuecomment-5198355340
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 906bc0aea74051d857bc9e676fb53522d3e09fd3.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/92469700964) has more details.
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "hiroyuki-sato (via GitHub)" <gi...@apache.org>.
hiroyuki-sato commented on PR #50798:
URL: https://github.com/apache/arrow/pull/50798#issuecomment-5174617869
@github-actions crossbow submit test-r-linux-valgrind
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "hiroyuki-sato (via GitHub)" <gi...@apache.org>.
hiroyuki-sato commented on PR #50798:
URL: https://github.com/apache/arrow/pull/50798#issuecomment-5186044731
@github-actions crossbow submit test-r-linux-valgrind
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "hiroyuki-sato (via GitHub)" <gi...@apache.org>.
hiroyuki-sato commented on code in PR #50798:
URL: https://github.com/apache/arrow/pull/50798#discussion_r3709036593
##########
ci/scripts/r_valgrind.sh:
##########
@@ -18,28 +18,28 @@
set -ex
-: ${R_BIN:=RDvalgrind}
+: "${R_BIN:=RDvalgrind}"
-source_dir=${1}/r
+source_dir="${1}/r"
export CMAKE_BUILD_TYPE=RelWithDebInfo
-pushd ${source_dir}
+pushd "${source_dir}"
# build first so that any stray compiled files in r/src are ignored
-${R_BIN} CMD build --no-build-vignettes .
-${R_BIN} CMD INSTALL ${INSTALL_ARGS} arrow*.tar.gz
+"${R_BIN}" CMD build --no-build-vignettes .
+"${R_BIN}" CMD INSTALL "${INSTALL_ARGS}" arrow*.tar.gz
pushd tests
# to generate suppression files run:
# ${R_BIN} --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --gen-suppressions=all --log-file=memcheck.log" -f testthat.R
-${R_BIN} --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --suppressions=/${1}/ci/etc/valgrind-cran.supp" -f testthat.R |& tee testthat.out
+"${R_BIN}" --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --suppressions=/${1}/ci/etc/valgrind-cran.supp" -f testthat.R |& tee testthat.out
# valgrind --error-exitcode=1 should return an erroring exit code that we can catch,
# but R eats that and returns 0, so we need to look at the output and make sure that
# we have 0 errors instead.
-if [ $(grep -c "ERROR SUMMARY: 0 errors" testthat.out) != 1 ]; then
+if [ "$(grep -c "ERROR SUMMARY: 0 errors" testthat.out)" -ne 1 ]; then
cat testthat.out
Review Comment:
`"$(grep -c "ERROR SUMMARY: 0 errors" testthat.out)"` execute `grep -c "ERROR SUMMARY: 0 errors" testthat.out` like the below. (And compare number using `-ne`)
```bash
#!
if [ "$(grep -c "HOGE HOGE" test.sh)" -ne 1 ] ; then
echo "NOT ONE"
fi
# HOGE HOGE
# HOGE HOGE
# HOGEHOGE
```
```
sh -x test.sh
++ grep -c 'HOGE HOGE' test.sh
+ '[' 3 -ne 1 ']'
+ echo 'NOT ONE'
NOT ONE
```
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "hiroyuki-sato (via GitHub)" <gi...@apache.org>.
hiroyuki-sato commented on PR #50798:
URL: https://github.com/apache/arrow/pull/50798#issuecomment-5174616172
https://github.com/ursacomputing/crossbow/actions/runs/30874057795/job/91881785985#step:6:23742
t looks like `${INSTALL_ARGS}` is not set. I’ll try converting the variable into an array.
```
R CMD INSTALL '' arrow_25.0.0.9000.tar.gz
```
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #50798:
URL: https://github.com/apache/arrow/pull/50798
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50798:
URL: https://github.com/apache/arrow/pull/50798#issuecomment-5174625445
Revision: a41f6ca65ea2ae5fe77427174b4391114be1f299
Submitted crossbow builds: [ursacomputing/crossbow @ actions-088bc54f02](https://github.com/ursacomputing/crossbow/branches/all?query=actions-088bc54f02)
|Task|Status|
|----|------|
|test-r-linux-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/30877857518/job/91892827351)|
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "hiroyuki-sato (via GitHub)" <gi...@apache.org>.
hiroyuki-sato commented on PR #50798:
URL: https://github.com/apache/arrow/pull/50798#issuecomment-5174160118
@github-actions crossbow submit test-r-linux-valgrind
--
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] GH-50796: [CI][Dev] Fix shellcheck errors in the ci/scripts/r_valgrind.sh [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #50798:
URL: https://github.com/apache/arrow/pull/50798#discussion_r3715836388
##########
ci/scripts/r_valgrind.sh:
##########
@@ -18,28 +18,32 @@
set -ex
-: ${R_BIN:=RDvalgrind}
+: "${R_BIN:=RDvalgrind}"
-source_dir=${1}/r
+source_dir="${1}/r"
export CMAKE_BUILD_TYPE=RelWithDebInfo
-pushd ${source_dir}
+pushd "${source_dir}"
+
+# Convert the space-separated options into a Bash array.
+# This avoids ShellCheck SC2086 and preserves argument boundaries.
+read -r -a R_INSTALL_ARGS <<< "${INSTALL_ARGS:-}"
# build first so that any stray compiled files in r/src are ignored
-${R_BIN} CMD build --no-build-vignettes .
-${R_BIN} CMD INSTALL ${INSTALL_ARGS} arrow*.tar.gz
+"${R_BIN}" CMD build --no-build-vignettes .
+"${R_BIN}" CMD INSTALL "${R_INSTALL_ARGS[@]}" arrow*.tar.gz
pushd tests
# to generate suppression files run:
# ${R_BIN} --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --gen-suppressions=all --log-file=memcheck.log" -f testthat.R
-${R_BIN} --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --suppressions=/${1}/ci/etc/valgrind-cran.supp" -f testthat.R |& tee testthat.out
+"${R_BIN}" --vanilla -d "valgrind --tool=memcheck --leak-check=full --track-origins=yes --suppressions=/${1}/ci/etc/valgrind-cran.supp" -f testthat.R |& tee testthat.out
# valgrind --error-exitcode=1 should return an erroring exit code that we can catch,
# but R eats that and returns 0, so we need to look at the output and make sure that
# we have 0 errors instead.
-if [ $(grep -c "ERROR SUMMARY: 0 errors" testthat.out) != 1 ]; then
+if [ "$(grep -c "ERROR SUMMARY: 0 errors" testthat.out)" -ne 1 ]; then
Review Comment:
Can we use the return code by `grep`?
```suggestion
if ! grep -q "ERROR SUMMARY: 0 errors" testthat.out; 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