[
https://issues.apache.org/jira/browse/MWRAPPER-88?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17654906#comment-17654906
]
ASF GitHub Bot commented on MWRAPPER-88:
----------------------------------------
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1062317792
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -71,38 +71,38 @@ esac
if [ -z "$JAVA_HOME" ] ; then
if [ -r /etc/gentoo-release ] ; then
- JAVA_HOME=`java-config --jre-home`
+ JAVA_HOME=$(java-config --jre-home)
fi
fi
# For Cygwin, ensure paths are in UNIX format before anything is touched
if $cygwin ; then
[ -n "$JAVA_HOME" ] &&
- JAVA_HOME=`cygpath --unix "$JAVA_HOME"`
+ JAVA_HOME=$(cygpath --unix "$JAVA_HOME")
[ -n "$CLASSPATH" ] &&
- CLASSPATH=`cygpath --path --unix "$CLASSPATH"`
+ CLASSPATH=$(cygpath --path --unix "$CLASSPATH")
fi
# For Mingw, ensure paths are in UNIX format before anything is touched
if $mingw ; then
- [ -n "$JAVA_HOME" ] &&
- JAVA_HOME="`(cd "$JAVA_HOME"; pwd)`"
+ [ -n "$JAVA_HOME" ] && [ -d "$JAVA_HOME" ] &&
+ JAVA_HOME="$(cd "$JAVA_HOME" || (echo "cannot cd into $JAVA_HOME."; exit
1); pwd)"
Review Comment:
This prevents going into a non-existent directory (i.e. stay in CWD) and
replace JAVA_HOME with basically the current working dir.
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -281,20 +286,21 @@ MAVEN_OPTS="$(concat_lines
"$MAVEN_PROJECTBASEDIR/.mvn/jvm.config") $MAVEN_OPTS"
# For Cygwin, switch paths to Windows format before running java
if $cygwin; then
[ -n "$JAVA_HOME" ] &&
- JAVA_HOME=`cygpath --path --windows "$JAVA_HOME"`
+ JAVA_HOME=$(cygpath --path --windows "$JAVA_HOME")
[ -n "$CLASSPATH" ] &&
- CLASSPATH=`cygpath --path --windows "$CLASSPATH"`
+ CLASSPATH=$(cygpath --path --windows "$CLASSPATH")
[ -n "$MAVEN_PROJECTBASEDIR" ] &&
- MAVEN_PROJECTBASEDIR=`cygpath --path --windows "$MAVEN_PROJECTBASEDIR"`
+ MAVEN_PROJECTBASEDIR=$(cygpath --path --windows "$MAVEN_PROJECTBASEDIR")
fi
# Provide a "standardized" way to retrieve the CLI args that will
# work with both Windows and non-Windows executions.
-MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $@"
+MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $*"
Review Comment:
WHile `$@` works, it will in fact expand to this:
`MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $1" "$2" "$3" "$4"` (etc)
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -71,38 +71,38 @@ esac
if [ -z "$JAVA_HOME" ] ; then
if [ -r /etc/gentoo-release ] ; then
- JAVA_HOME=`java-config --jre-home`
+ JAVA_HOME=$(java-config --jre-home)
fi
fi
# For Cygwin, ensure paths are in UNIX format before anything is touched
if $cygwin ; then
[ -n "$JAVA_HOME" ] &&
- JAVA_HOME=`cygpath --unix "$JAVA_HOME"`
+ JAVA_HOME=$(cygpath --unix "$JAVA_HOME")
[ -n "$CLASSPATH" ] &&
- CLASSPATH=`cygpath --path --unix "$CLASSPATH"`
+ CLASSPATH=$(cygpath --path --unix "$CLASSPATH")
fi
# For Mingw, ensure paths are in UNIX format before anything is touched
if $mingw ; then
- [ -n "$JAVA_HOME" ] &&
- JAVA_HOME="`(cd "$JAVA_HOME"; pwd)`"
+ [ -n "$JAVA_HOME" ] && [ -d "$JAVA_HOME" ] &&
+ JAVA_HOME="$(cd "$JAVA_HOME" || (echo "cannot cd into $JAVA_HOME."; exit
1); pwd)"
fi
if [ -z "$JAVA_HOME" ]; then
- javaExecutable="`which javac`"
- if [ -n "$javaExecutable" ] && ! [ "`expr \"$javaExecutable\" : '\([^
]*\)'`" = "no" ]; then
+ javaExecutable="$(which javac)"
+ if [ -n "$javaExecutable" ] && ! [ "$(expr "\"$javaExecutable\"" : '\([^
]*\)')" = "no" ]; then
# readlink(1) is not available as standard on Solaris 10.
- readLink=`which readlink`
- if [ ! `expr "$readLink" : '\([^ ]*\)'` = "no" ]; then
+ readLink=$(which readlink)
+ if [ ! "$(expr "$readLink" : '\([^ ]*\)')" = "no" ]; then
if $darwin ; then
- javaHome="`dirname \"$javaExecutable\"`"
- javaExecutable="`cd \"$javaHome\" && pwd -P`/javac"
+ javaHome="$(dirname "\"$javaExecutable\"")"
+ javaExecutable="$(cd "\"$javaHome\"" && pwd -P)/javac"
else
- javaExecutable="`readlink -f \"$javaExecutable\"`"
+ javaExecutable="$(readlink -f "\"$javaExecutable\"")"
fi
- javaHome="`dirname \"$javaExecutable\"`"
- javaHome=`expr "$javaHome" : '\(.*\)/bin'`
+ javaHome="$(dirname "\"$javaExecutable\"")"
+ javaHome=$(expr "$javaHome" : '\(.*\)/bin')
Review Comment:
Not sure about those escaped quotes. Any idea why they exist?
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -193,42 +193,48 @@ else
else
wrapperUrl="https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/@@project.version@@/maven-wrapper-@@project.version@@.jar";
fi
- while IFS="=" read key value; do
+ while IFS="=" read -r key value; do
case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
esac
done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
log "Downloading from: $wrapperUrl"
if $cygwin; then
- wrapperJarPath=`cygpath --path --windows "$wrapperJarPath"`
+ wrapperJarPath=$(cygpath --path --windows "$wrapperJarPath")
fi
if command -v wget > /dev/null; then
log "Found wget ... using wget"
+ wgetrc=0
[ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--quiet"
if [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ]; then
wget $QUIET "$wrapperUrl" -O "$wrapperJarPath"
+ wgetrc=$?
else
wget $QUIET --http-user="$MVNW_USERNAME"
--http-password="$MVNW_PASSWORD" "$wrapperUrl" -O "$wrapperJarPath"
+ wgetrc=$?
fi
- [ $? -eq 0 ] || rm -f "$wrapperJarPath"
+ [ $wgetrc -eq 0 ] || rm -f "$wrapperJarPath"
elif command -v curl > /dev/null; then
log "Found curl ... using curl"
+ curlrc=0
[ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--silent"
if [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ]; then
curl $QUIET -o "$wrapperJarPath" "$wrapperUrl" -f -L
+ curlrc=$?
else
curl $QUIET --user "$MVNW_USERNAME:$MVNW_PASSWORD" -o
"$wrapperJarPath" "$wrapperUrl" -f -L
+ curlrc=$?
fi
- [ $? -eq 0 ] || rm -f "$wrapperJarPath"
+ [ $curlrc -eq 0 ] || rm -f "$wrapperJarPath"
Review Comment:
instead of adding a new var, we could use sth like `if wget ...` directly. I
would really remove the original line 229 though (`[ $? -eq 0]`), because it is
easy to forget about it and add yet another command in between, e.g. a
debugging print or echo statement which will make the check wrong.
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -193,42 +193,48 @@ else
else
wrapperUrl="https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/@@project.version@@/maven-wrapper-@@project.version@@.jar";
fi
- while IFS="=" read key value; do
+ while IFS="=" read -r key value; do
Review Comment:
without `-r` it will eat backslashes
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -150,17 +150,17 @@ find_maven_basedir() {
fi
# workaround for JBEAP-8937 (on Solaris 10/Sparc)
if [ -d "${wdir}" ]; then
- wdir=`cd "$wdir/.."; pwd`
+ wdir=$(cd "$wdir/.." || exit 1; pwd)
fi
# end of workaround
done
- printf '%s' "$(cd "$basedir"; pwd)"
+ printf '%s' "$(cd "$basedir" || exit 1; pwd)"
Review Comment:
We *could* print a message here using `|| ( echo ""; exit 1); pwd)`
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -237,8 +243,7 @@ else
fi
if [ -e "$javaClass" ]; then
log " - Running MavenWrapperDownloader.java ..."
- ("$JAVA_HOME/bin/java" -cp .mvn/wrapper MavenWrapperDownloader
"$wrapperUrl" "$wrapperJarPath")
- [ $? -eq 0 ] || rm -f "$wrapperJarPath"
+ ("$JAVA_HOME/bin/java" -cp .mvn/wrapper MavenWrapperDownloader
"$wrapperUrl" "$wrapperJarPath") || rm -f "$wrapperJarPath"
Review Comment:
not sure why the parenthesis are there. Kept it, but can probably be removed.
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -281,20 +286,21 @@ MAVEN_OPTS="$(concat_lines
"$MAVEN_PROJECTBASEDIR/.mvn/jvm.config") $MAVEN_OPTS"
# For Cygwin, switch paths to Windows format before running java
if $cygwin; then
[ -n "$JAVA_HOME" ] &&
- JAVA_HOME=`cygpath --path --windows "$JAVA_HOME"`
+ JAVA_HOME=$(cygpath --path --windows "$JAVA_HOME")
[ -n "$CLASSPATH" ] &&
- CLASSPATH=`cygpath --path --windows "$CLASSPATH"`
+ CLASSPATH=$(cygpath --path --windows "$CLASSPATH")
[ -n "$MAVEN_PROJECTBASEDIR" ] &&
- MAVEN_PROJECTBASEDIR=`cygpath --path --windows "$MAVEN_PROJECTBASEDIR"`
+ MAVEN_PROJECTBASEDIR=$(cygpath --path --windows "$MAVEN_PROJECTBASEDIR")
fi
# Provide a "standardized" way to retrieve the CLI args that will
# work with both Windows and non-Windows executions.
-MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $@"
+MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $*"
export MAVEN_CMD_LINE_ARGS
WRAPPER_LAUNCHER=org.apache.maven.wrapper.MavenWrapperMain
+# shellcheck disable=SC2086 # safe args
exec "$JAVACMD" \
$MAVEN_OPTS \
$MAVEN_DEBUG_OPTS \
Review Comment:
It would be better to use arrays here if possible.
> mvnw is not POSIX compatible
> ----------------------------
>
> Key: MWRAPPER-88
> URL: https://issues.apache.org/jira/browse/MWRAPPER-88
> Project: Maven Wrapper
> Issue Type: Bug
> Components: Maven Wrapper Scripts
> Affects Versions: 3.1.1
> Reporter: Benjamin Marwell
> Assignee: Benjamin Marwell
> Priority: Major
> Labels: posix
>
> * POSIX scripts should not use bash-specific features
> * the location of sh is NOT /bin/sh. Instead use /usr/bin/env sh.
> * Fix suggestions from shellcheck
> (https://github.com/apache/maven-wrapper/pull/74)
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
