You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/12/09 16:24:57 UTC
[GitHub] [solr] janhoy opened a new pull request, #1225: SOLR-9509 Fix problems reported by shellcheck
janhoy opened a new pull request, #1225:
URL: https://github.com/apache/solr/pull/1225
https://issues.apache.org/jira/browse/SOLR-9509
I used a shellcheck plugin to IntelliJ to fix these. I skipped several suggestions since I don't want to use "new" switches for various tools which may break back-compat. These were :
> SC2009 (info): Consider using pgrep instead of grepping ps output.
> SC2162 (info): read without -r will mangle backslashes.
Also you will still find complaints about this one
> SC1090 (warning): ShellCheck can't follow non-constant source. Use a directive to specify location.
A few places we accept `$FOO` as string input from user, and then redefined as array `FOO = ("bar")`, which I guess is an anti pattern. I fixed it one place, but we still have some of 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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1225:
URL: https://github.com/apache/solr/pull/1225#discussion_r1044742060
##########
solr/bin/solr:
##########
@@ -2153,16 +2160,17 @@ function start_solr() {
SOLR_JETTY_ADDL_CONFIG="$3"
# define default GC_TUNE
- if [ -z ${GC_TUNE+x} ]; then # I think this check is backwards?
- GC_TUNE=('-XX:+UseG1GC' \
+ if [ -z "${GC_TUNE}" ]; then
+ GC_TUNE_ARR=('-XX:+UseG1GC' \
Review Comment:
@madrob You added that comment about backward check. I agree, cause the `+x` thing will always pass the condition, so currently it will never be possibel to override `GC_TUNE` for the user, agree? I'm pretty sure that my fix here is enough
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1225:
URL: https://github.com/apache/solr/pull/1225#discussion_r1044737218
##########
solr/bin/solr:
##########
@@ -841,25 +846,26 @@ function stop_solr() {
DIR="$1"
SOLR_PORT="$2"
- THIS_STOP_PORT="${STOP_PORT:-$(expr $SOLR_PORT - 1000)}"
+ THIS_STOP_PORT="${STOP_PORT:-$((SOLR_PORT - 1000))}"
STOP_KEY="$3"
SOLR_PID="$4"
if [ -n "$SOLR_PID" ]; then
echo -e "Sending stop command to Solr running on port $SOLR_PORT ... waiting up to $SOLR_STOP_WAIT seconds to allow Jetty process $SOLR_PID to stop gracefully."
+ # shellcheck disable=SC2086
"$JAVA" $SOLR_SSL_OPTS $AUTHC_OPTS -jar "$DIR/start.jar" "STOP.PORT=$THIS_STOP_PORT" "STOP.KEY=$STOP_KEY" --stop || true
(loops=0
while true
do
# Check if a process is running with the specified PID.
# -o stat will output the STAT, where Z indicates a zombie
# stat='' removes the header (--no-headers isn't supported on all platforms)
- STAT=`(ps -o stat='' -p $SOLR_PID || :) | tr -d ' '`
+ STAT=$( (ps -o stat='' -p "$SOLR_PID" || :) | tr -d ' ')
Review Comment:
Note that the space after `$(` is intentional, as `$((` has a special meaning!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1225:
URL: https://github.com/apache/solr/pull/1225#discussion_r1044732306
##########
solr/bin/solr:
##########
@@ -73,25 +74,26 @@ fi
# Resolve symlinks to this script
while [ -h "$SOLR_SCRIPT" ] ; do
- ls=`ls -ld "$SOLR_SCRIPT"`
+ ls=$(ls -ld "$SOLR_SCRIPT")
# Drop everything prior to ->
- link=`expr "$ls" : '.*-> \(.*\)$'`
+ link=$(expr "$ls" : '.*-> \(.*\)$')
if expr "$link" : '/.*' > /dev/null; then
SOLR_SCRIPT="$link"
else
- SOLR_SCRIPT=`dirname "$SOLR_SCRIPT"`/"$link"
+ SOLR_SCRIPT=$(dirname "$SOLR_SCRIPT")/"$link"
fi
done
CDPATH='' # Prevent "file or directory not found" for 'cdpath' users
-SOLR_TIP=`dirname "$SOLR_SCRIPT"`/..
-SOLR_TIP=`cd "$SOLR_TIP"; pwd`
+SOLR_TIP=$(dirname "$SOLR_SCRIPT")/..
+# shellcheck disable=SC2164
+SOLR_TIP=$(cd "$SOLR_TIP"; pwd)
Review Comment:
No need to catch "cd" error when the variable is newly created and will always exist...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a diff in pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
madrob commented on code in PR #1225:
URL: https://github.com/apache/solr/pull/1225#discussion_r1063055366
##########
solr/bin/solr:
##########
@@ -2153,16 +2160,17 @@ function start_solr() {
SOLR_JETTY_ADDL_CONFIG="$3"
# define default GC_TUNE
- if [ -z ${GC_TUNE+x} ]; then # I think this check is backwards?
- GC_TUNE=('-XX:+UseG1GC' \
+ if [ -z "${GC_TUNE}" ]; then
+ GC_TUNE_ARR=('-XX:+UseG1GC' \
Review Comment:
I think this will fail if GC_TUNE is unset now? Might not fail in regular operation, but in BATS it might be more strict and complain.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1225:
URL: https://github.com/apache/solr/pull/1225#discussion_r1063389460
##########
solr/bin/solr:
##########
@@ -2153,16 +2160,17 @@ function start_solr() {
SOLR_JETTY_ADDL_CONFIG="$3"
# define default GC_TUNE
- if [ -z ${GC_TUNE+x} ]; then # I think this check is backwards?
- GC_TUNE=('-XX:+UseG1GC' \
+ if [ -z "${GC_TUNE}" ]; then
+ GC_TUNE_ARR=('-XX:+UseG1GC' \
Review Comment:
I tested this
```bash
$ unset GC_TUNE
$ if [ -z "${GC_TUNE}" ]; then 1 ↵ 10130 13:11:57
echo "DEFAULTS"
else
echo "OVERRIDE"
fi
DEFAULTS
$ GC_TUNE=foo
$ if [ -z "${GC_TUNE}" ]; then ✔ 10128 13:11:28
echo "DEFAULTS"
else
echo "OVERRIDE"
fi
OVERRIDE
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1225:
URL: https://github.com/apache/solr/pull/1225#issuecomment-1373026516
I'm ready to merge this. I ran `gradlew integrationTests` to exercise the script with BATS tests, and all 34 passed. That's a good sign.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1225:
URL: https://github.com/apache/solr/pull/1225#issuecomment-1344512480
There's a bug somewhere, trying to hunt down...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1225:
URL: https://github.com/apache/solr/pull/1225#discussion_r1044735101
##########
solr/bin/solr:
##########
@@ -729,6 +733,7 @@ function jetty_port() {
# useful for doing cross-platform work from the command-line using Java
function run_tool() {
+ # shellcheck disable=SC2086
"$JAVA" $SOLR_SSL_OPTS $AUTHC_OPTS ${SOLR_ZK_CREDS_AND_ACLS:-} -Dsolr.install.dir="$SOLR_TIP" \
Review Comment:
I tried to quote `$AUTHC_OPTS` but when that var is undefined/empty, it leaves two quotes `''` which trips the startup. So it is safest to just leave these here. Besides, these vars may intentionally contain multiple arguments that would be wrong to pass as one quoted argument.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #1225:
URL: https://github.com/apache/solr/pull/1225#discussion_r1044738129
##########
solr/bin/solr:
##########
@@ -841,25 +846,26 @@ function stop_solr() {
DIR="$1"
SOLR_PORT="$2"
- THIS_STOP_PORT="${STOP_PORT:-$(expr $SOLR_PORT - 1000)}"
+ THIS_STOP_PORT="${STOP_PORT:-$((SOLR_PORT - 1000))}"
STOP_KEY="$3"
SOLR_PID="$4"
if [ -n "$SOLR_PID" ]; then
echo -e "Sending stop command to Solr running on port $SOLR_PORT ... waiting up to $SOLR_STOP_WAIT seconds to allow Jetty process $SOLR_PID to stop gracefully."
+ # shellcheck disable=SC2086
"$JAVA" $SOLR_SSL_OPTS $AUTHC_OPTS -jar "$DIR/start.jar" "STOP.PORT=$THIS_STOP_PORT" "STOP.KEY=$STOP_KEY" --stop || true
(loops=0
while true
do
# Check if a process is running with the specified PID.
# -o stat will output the STAT, where Z indicates a zombie
# stat='' removes the header (--no-headers isn't supported on all platforms)
- STAT=`(ps -o stat='' -p $SOLR_PID || :) | tr -d ' '`
+ STAT=$( (ps -o stat='' -p "$SOLR_PID" || :) | tr -d ' ')
Review Comment:
I would definitely include this in a comment so someone doesn't "fix" it.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1225:
URL: https://github.com/apache/solr/pull/1225#discussion_r1044739591
##########
solr/bin/solr:
##########
@@ -1821,7 +1825,7 @@ fi
# otherwise let this script proceed to process the user request
#
if [ -n "${EXAMPLE:-}" ] && [ "$SCRIPT_CMD" == "start" ]; then
- run_tool run_example -e $EXAMPLE -d "$SOLR_SERVER_DIR" -urlScheme $SOLR_URL_SCHEME $PASS_TO_RUN_EXAMPLE
+ run_tool run_example -e "$EXAMPLE" -d "$SOLR_SERVER_DIR" -urlScheme "$SOLR_URL_SCHEME" "$PASS_TO_RUN_EXAMPLE"
Review Comment:
Hmm, I have not tested the examples - is it safe to quote `"$PASS_TO_RUN_EXAMPLE"` here?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy merged pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
janhoy merged PR #1225:
URL: https://github.com/apache/solr/pull/1225
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1225: SOLR-9509 Fix problems reported by shellcheck
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1225:
URL: https://github.com/apache/solr/pull/1225#discussion_r1063389460
##########
solr/bin/solr:
##########
@@ -2153,16 +2160,17 @@ function start_solr() {
SOLR_JETTY_ADDL_CONFIG="$3"
# define default GC_TUNE
- if [ -z ${GC_TUNE+x} ]; then # I think this check is backwards?
- GC_TUNE=('-XX:+UseG1GC' \
+ if [ -z "${GC_TUNE}" ]; then
+ GC_TUNE_ARR=('-XX:+UseG1GC' \
Review Comment:
I tested this
```bash
$ unset GC_TUNE
$ if [ -z "${GC_TUNE}" ]; then echo "DEFAULTS"; else echo "OVERRIDE"; fi
DEFAULTS
$ GC_TUNE=foo
$ if [ -z "${GC_TUNE}" ]; then echo "DEFAULTS"; else echo "OVERRIDE"; fi
OVERRIDE
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org