You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2023/01/05 09:34:02 UTC
[GitHub] [maven-wrapper] bmarwell opened a new pull request, #79: Mwrapper 88 posix
bmarwell opened a new pull request, #79:
URL: https://github.com/apache/maven-wrapper/pull/79
Fix shellcheck warnings
---
Following this checklist to help us incorporate your
contribution quickly and easily:
- [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MWRAPPER-88) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] Format the pull request title like `[MWRAPPER-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MWRAPPER-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [X] You have run the integration tests successfully (`mvn -Prun-its clean verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [X] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063556378
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -118,7 +118,7 @@ if [ -z "$JAVACMD" ] ; then
JAVACMD="$JAVA_HOME/bin/java"
fi
else
- JAVACMD="`\\unset -f command; \\command -v java`"
+ JAVACMD="$(\\unset -f command; \\command -v java)"
Review Comment:
```
JAVACMD="$("unset" -f command 2>/dev/null; "command" -v java)"
```
This above seems to work on bash and zsh:
```
➜ ~ zsh -li
➜ ~ JAVACMD="$("unset" -f command 2>/dev/null; "command" -v java)" ; echo $JAVACMD
/Users/gnodet/.jenv/shims/java
➜ ~ bash -li
gnodet-mac:~ gnodet$ JAVACMD="$("unset" -f command 2>/dev/null; "command" -v java)" ; echo $JAVACMD
/usr/bin/java
gnodet-mac:~ gnodet$
```
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063400588
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -118,7 +118,7 @@ if [ -z "$JAVACMD" ] ; then
JAVACMD="$JAVA_HOME/bin/java"
fi
else
- JAVACMD="`\\unset -f command; \\command -v java`"
+ JAVACMD="$(\\unset -f command; \\command -v java)"
Review Comment:
This is probably a bug. Double backslash will not work with `$()`:
```
user@host ~ $ JAVACMD="$(\\unset -f command; \\command -v java)"
zsh: command not found: \unset
zsh: command not found: \command
user@host ~ $ JAVACMD=$(\\unset -f command; \\command -v java)
zsh: command not found: \unset
zsh: command not found: \command
user@host ~ $ JAVACMD="$(\unset -f command; \command -v java)"
unset: no such hash table element: command
user@host ~ $ bash -li
[user@host ~]$ JAVACMD="$(\unset -f command; \command -v java)"
[user@host ~]$ JAVACMD="$(\\unset -f command; \\command -v java)"
bash: \unset: Kommando nicht gefunden.
bash: \command: Kommando nicht gefunden.
[user@host ~]$ JAVACMD=`\\unset -f command; \\command -v java`
[user@host ~]$
```
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063583507
##########
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:
We should open a Jira issue and investigate
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063547852
##########
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:
To be coherent, maybe we should rewrite it to the more simple:
```
"$JAVA_HOME/bin/java" -cp .mvn/wrapper MavenWrapperDownloader "$wrapperUrl" "$wrapperJarPath"
```
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1065065753
##########
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:
Done, thanks! I left out the concatination of user and password for now, but we can create a separate issue for this.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063397662
##########
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:
As from the `expr' readme, expr doesn't add quotes itself. However, it will never match `"no"`, but only `"\"no\""` - I am not going to fix that in this PR.
Anyway, adding surrounding quotes is correct. Consider this path:
`/home/user/java installations/current/bin/javac`
```
user@host ~ $ javaExecutable="/home/user/java installations/current/bin/javac"
user@host ~ $ expr \"$javaExecutable\" : '\([^ ]*\)'
"/home/user/java
user@host ~ $ expr "\"$javaExecutable\"" : '\([^ ]*\)'
"/home/user/java
```
Whatever it does it fails on paths with spaces.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063539117
##########
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:
What about:
```
if command -v wget > /dev/null; then
log "Found wget ... using wget"
[ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--quiet"
{ [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ] ;} \
&& USEROPT="" \
|| USEROPT="--http-user=\"$MVNW_USERNAME\" --http-password=\"$MVNW_PASSWORD\""
wget $QUIET $USEROPT "$wrapperUrl" -O "$wrapperJarPath" \
|| rm -f "$wrapperJarPath"
elif command -v curl > /dev/null; then
log "Found curl ... using curl"
[ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--silent"
{ [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ] ;} \
&& USEROPT="" \
|| USEROPT="--user \"$MVNW_USERNAME:$MVNW_PASSWORD\""
curl $QUIET $USEROPT -o "$wrapperJarPath" "$wrapperUrl" -f -L \
|| rm -f "$wrapperJarPath"
else
```
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063447254
##########
.github/workflows/shellcheck-posix.yml:
##########
@@ -27,7 +27,7 @@ jobs:
runs-on: ubuntu-latest
env:
SHELLCHECK_VERSION: '0.9.0'
- SHELLCHECK_OPTS: '-e SC2009'
+ SHELLCHECK_OPTS: '-e SC1091 -e SC2009'
Review Comment:
I would like to add description what this numbers means - what rule is exclude
##########
.github/workflows/shellcheck-posix.yml:
##########
@@ -39,7 +39,7 @@ jobs:
- name: check
run: |
echo "$GITHUB_WORKSPACE/shellcheck-v${SHELLCHECK_VERSION}" >> $GITHUB_PATH
- readarray -t SCRIPT_FILES < <(grep -ErIl '^#![[:blank:]]*/bin/(ba|da|c|k)?sh' "$GITHUB_WORKSPACE/maven-wrapper-distribution/src/resources")
+ readarray -t SCRIPT_FILES < <(grep -ErIl -e '^#![[:blank:]]*/bin/(ba|da|c|k)?sh' -e '^#!/usr/bin/env[[:blank:]]+(ba|da|c|k)?sh' "$GITHUB_WORKSPACE/maven-wrapper-distribution/src/resources")
Review Comment:
should it also match `zsh`?
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063581666
##########
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:
Yes, we would need either an OR, or a trap function.
But.
The script won't exit anyway because neither `set -e` nor `set -o pipefail` is set.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] michael-o commented on pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#issuecomment-1372039127
Put this into consideration: https://issues.apache.org/jira/browse/MWRAPPER-45
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063537820
##########
.github/workflows/shellcheck-posix.yml:
##########
@@ -39,7 +39,7 @@ jobs:
- name: check
run: |
echo "$GITHUB_WORKSPACE/shellcheck-v${SHELLCHECK_VERSION}" >> $GITHUB_PATH
- readarray -t SCRIPT_FILES < <(grep -ErIl '^#![[:blank:]]*/bin/(ba|da|c|k)?sh' "$GITHUB_WORKSPACE/maven-wrapper-distribution/src/resources")
+ readarray -t SCRIPT_FILES < <(grep -ErIl -e '^#![[:blank:]]*/bin/(ba|da|c|k)?sh' -e '^#!/usr/bin/env[[:blank:]]+(ba|da|c|k)?sh' "$GITHUB_WORKSPACE/maven-wrapper-distribution/src/resources")
Review Comment:
Can do. But then, we should only write POSIX scripts so it runs everywhere. This would only be needed for shell specific scripts. I'd consider it optional, but will add it later.
Thanks for the finding.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#issuecomment-1377694964
I don't know what you mean here. Did I change something in that regard?
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063550049
##########
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:
Are there targets that could possibly not support arrays ?
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063545643
##########
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:
The parentheses will create a sub shell, so that if the java command fails, the script won't end abruptly (unless the curl/wget above).
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063583039
##########
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:
The script won't exit anyway because neither `set -e` nor `set -o pipefail` is set.
Rewriting is not part of this commit. Let's not fix things here which are not broken. We can open an issue for this, though. I just want to make sure the script behaves the same as before.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
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.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063399570
##########
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:
As for "dirname", this is the difference:
```
user@host ~ $ dirname \"$javaExecutable\"
"/home/user/java installations/current/bin
user@host ~ $ dirname "\"$javaExecutable\""
"/home/user/java installations/current/bin
# before and after both failing:
user@host ~ $ MY_PATH="$(dirname "\"$javaExecutable\"")"; echo "$MY_PATH"
"/home/user/java installations/current/bin
user@host ~ $ MY_PATH=$(dirname \"$javaExecutable\"); echo "$MY_PATH"
"/home/user/java installations/current/bin
# this is the right way to do it (imo):
user@host ~ $ MY_PATH=$(dirname "$javaExecutable"); echo "$MY_PATH"
/home/user/java installations/current/bin
user@host ~ $ MY_PATH="$(dirname "$javaExecutable")"; echo "$MY_PATH"
/home/user/java installations/current/bin
```
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#issuecomment-1377841386
> I don't know what you mean here. Did I change something in that regard?
It is ok for me in this PR.
Simply we have two similar scripts, one in wrapper and one in Maven distribution. But it is another discussion.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063536293
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -118,7 +118,7 @@ if [ -z "$JAVACMD" ] ; then
JAVACMD="$JAVA_HOME/bin/java"
fi
else
- JAVACMD="`\\unset -f command; \\command -v java`"
+ JAVACMD="$(\\unset -f command; \\command -v java)"
Review Comment:
A backslash usually means "don't use the alias if it exists". The double backslash was probably used b/c of backticks which we got rid of.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063543811
##########
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:
Do I miss something or the shell will exit if curl/wget fail ?
If so the `rm -f "$wrapperJarPath"` is useless anyway...
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#issuecomment-1372564795
Needs a slight revert:
https://stackoverflow.com/a/53116875/1549977
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063556378
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -118,7 +118,7 @@ if [ -z "$JAVACMD" ] ; then
JAVACMD="$JAVA_HOME/bin/java"
fi
else
- JAVACMD="`\\unset -f command; \\command -v java`"
+ JAVACMD="$(\\unset -f command; \\command -v java)"
Review Comment:
```
JAVACMD="$("unset" -f command 2>/dev/null; "command" -v java)"
```
This above seems to work on bash and zsh
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063591166
##########
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:
Ah, missed that. So my first proposal should work and keep the existing behaviour.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063402134
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -118,7 +118,7 @@ if [ -z "$JAVACMD" ] ; then
JAVACMD="$JAVA_HOME/bin/java"
fi
else
- JAVACMD="`\\unset -f command; \\command -v java`"
+ JAVACMD="$(\\unset -f command; \\command -v java)"
Review Comment:
This never worked in `zsh`. Probably a bashism. Can someone verify this?
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063556378
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -118,7 +118,7 @@ if [ -z "$JAVACMD" ] ; then
JAVACMD="$JAVA_HOME/bin/java"
fi
else
- JAVACMD="`\\unset -f command; \\command -v java`"
+ JAVACMD="$(\\unset -f command; \\command -v java)"
Review Comment:
```
JAVACMD="$("unset" -f command 2>/dev/null; "command" -v java)"
```
This above seems to work on bash and zsh:
```
➜ ~ zsh -li
➜ ~ JAVACMD="$("unset" -f command 2>/dev/null; "command" -v java)" ; echo $JAVACMD
/Users/gnodet/.jenv/shims/java
➜ ~ bash -li
Restored session: Fri Jan 6 16:55:48 CET 2023
gnodet-mac:~ gnodet$ JAVACMD="$("unset" -f command 2>/dev/null; "command" -v java)" ; echo $JAVACMD
/usr/bin/java
gnodet-mac:~ gnodet$
```
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063556378
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -118,7 +118,7 @@ if [ -z "$JAVACMD" ] ; then
JAVACMD="$JAVA_HOME/bin/java"
fi
else
- JAVACMD="`\\unset -f command; \\command -v java`"
+ JAVACMD="$(\\unset -f command; \\command -v java)"
Review Comment:
```
JAVACMD="$("unset" -f command; "command" -v java)"
```
This above seems to work on bash and zsh
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063539117
##########
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:
What about:
```
if command -v wget > /dev/null; then
log "Found wget ... using wget"
[ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--quiet"
{ [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ] ;} \
&& USEROPT="" \
|| USEROPT="--http-user=\"$MVNW_USERNAME\" --http-password=\"$MVNW_PASSWORD\""
log wget $QUIET $USEROPT "$wrapperUrl" -O "$wrapperJarPath"
wget $QUIET $USEROPT "$wrapperUrl" -O "$wrapperJarPath" \
|| rm -f "$wrapperJarPath"
elif command -v curl > /dev/null; then
log "Found curl ... using curl"
[ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--silent"
{ [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ] ;} \
&& USEROPT="" \
|| USEROPT="--user \"$MVNW_USERNAME:$MVNW_PASSWORD\""
curl $QUIET $USEROPT -o "$wrapperJarPath" "$wrapperUrl" -f -L \
|| rm -f "$wrapperJarPath"
else
```
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] gnodet commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063544854
##########
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:
```
if command -v wget > /dev/null; then
log "Found wget ... using wget"
[ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--quiet"
{ [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ] ;} \
&& USEROPT="" \
|| USEROPT="--http-user=\"$MVNW_USERNAME\" --http-password=\"$MVNW_PASSWORD\""
wget $QUIET $USEROPT "$wrapperUrl" -O "$wrapperJarPath"
elif command -v curl > /dev/null; then
log "Found curl ... using curl"
[ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--silent"
{ [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ] ;} \
&& USEROPT="" \
|| USEROPT="--user \"$MVNW_USERNAME:$MVNW_PASSWORD\""
curl $QUIET $USEROPT -o "$wrapperJarPath" "$wrapperUrl" -f -L
else
```
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1065060233
##########
.github/workflows/shellcheck-posix.yml:
##########
@@ -27,7 +27,7 @@ jobs:
runs-on: ubuntu-latest
env:
SHELLCHECK_VERSION: '0.9.0'
- SHELLCHECK_OPTS: '-e SC2009'
+ SHELLCHECK_OPTS: '-e SC1091 -e SC2009'
Review Comment:
Done.
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on a diff in pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1063457960
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -118,7 +118,7 @@ if [ -z "$JAVACMD" ] ; then
JAVACMD="$JAVA_HOME/bin/java"
fi
else
- JAVACMD="`\\unset -f command; \\command -v java`"
+ JAVACMD="$(\\unset -f command; \\command -v java)"
Review Comment:
confirm on `GNU bash, version 5.2.15(1)-release (x86_64-apple-darwin21.6.0)` this is ok:
```bash
JAVACMD="$(unset -f command; command -v java)"; echo $JAVACMD
```
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell merged pull request #79: [MWRAPPER-88] fix shellcheck findings / POSIX compatiblity
Posted by GitBox <gi...@apache.org>.
bmarwell merged PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79
--
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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org