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 2022/05/17 17:27:04 UTC
[GitHub] [maven-wrapper] hazendaz opened a new pull request, #44: [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script
hazendaz opened a new pull request, #44:
URL: https://github.com/apache/maven-wrapper/pull/44
Usage of git bash will not download the maven wrapper jar along with curl (probably others) due to having windows line endings in the URL (trailing). To ensure that is not the case, make sure to strip invalid line endings out before usage.
Use case, ./mvnw in powershell will use mvnw.cmd and has no issues downloading. If user does same in git bash, it will fail with invalid URL error with curl. Using ./mvnw.cmd there will work but not natural usage. To ensure this simply just works for full support, trim out invalid line feeds.
note: This only affected the download. It worked otherwise.
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) 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)
- [ ] 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] michael-o commented on a diff in pull request #44: [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r875146529
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -198,6 +198,8 @@ else
if [ "$MVNW_VERBOSE" = true ]; then
echo "Downloading from: $wrapperUrl"
fi
+ # Remove invalid line endings from wrapperUrl
+ wrapperUrl=${wrapperUrl%$'\r'}
Review Comment:
Where does the URL come from? Do we need to fix the line ending of this resource in Git?
--
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] ascopes commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "ascopes (via GitHub)" <gi...@apache.org>.
ascopes commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1083434014
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
Review Comment:
as a side note, don't think it is necesarilly related to this issue, but https://docs.oracle.com/cd/E23095_01/Platform.93/ATGProgGuide/html/s0204propertiesfileformat01.html suggests that `:` is a valid character here too.
Along with spaces around these operators, it may be worth passing `key` and `value` through `sed 's/ \t//g'` to deal with this case at the same time.
--
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 a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1111314724
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove '\r' from value to allow usage on windows as IFS does not consider '\r' as a separator ( considers space, tab, new line ('\n'), and custom '=' )
+ safeValue=$(echo "$value" | sed 's/\r$//')
Review Comment:
> Or just use a bash variable expansion so we don't need to spawn another process. Given everyone is on bash 4 now.
NEVER expect something to be true in a target environment. That's Windows point of view.
--
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] ascopes commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "ascopes (via GitHub)" <gi...@apache.org>.
ascopes commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1083434014
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
Review Comment:
as a side note, don't think it is necesarilly related to this issue, but https://docs.oracle.com/cd/E23095_01/Platform.93/ATGProgGuide/html/s0204propertiesfileformat01.html suggests that `:` is a valid character here too.
Along with spaces around these operators, it may be worth passing `key` and `value` through `sed 's/[ \t]//g'` to deal with this case at the same time (or use TR).
e.g.
```
while IFS=':=' read -r key value; do
key="$(echo "${key}" | tr -d $' \t')"
value="$(echo "${value}" | tr -d $' \t\r')"
...
done
```
There is one other case that is potentially missed which is line continuations in the file but that is kinda out of scope 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 #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1083540094
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove '\r' from value to allow usage on windows as IFS does not consider '\r' as a separator ( considers space, tab, new line ('\n'), and custom '=' )
+ safeValue=$(echo "$value" | sed 's/\r$//')
Review Comment:
-1. See: https://www.shellcheck.net/wiki/SC2006
We use `/bin/sh` which should never point too `csh`, only to posix compliant shells (bash, dash on Ubuntu).
If you do this, shellcheck will fail.
--
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] hazendaz commented on pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "hazendaz (via GitHub)" <gi...@apache.org>.
hazendaz commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1424249993
any status 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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] ascopes commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "ascopes (via GitHub)" <gi...@apache.org>.
ascopes commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1111325458
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove '\r' from value to allow usage on windows as IFS does not consider '\r' as a separator ( considers space, tab, new line ('\n'), and custom '=' )
+ safeValue=$(echo "$value" | sed 's/\r$//')
Review Comment:
> Or just use a bash variable expansion
Didnt you say this was using /bin/sh? Surely that means there is no guarantee you have bash in the environment at all?
--
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 merged pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski merged PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44
--
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] hazendaz commented on a diff in pull request #44: [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script
Posted by GitBox <gi...@apache.org>.
hazendaz commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r913267565
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -192,7 +192,8 @@ 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
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove invalid line endings from value
+ case "$key" in (wrapperUrl) wrapperUrl="${value%$'\r'}"; break ;;
Review Comment:
@michael-o When you get a chance, can you check in on this again? Thanks.
--
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] ascopes commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "ascopes (via GitHub)" <gi...@apache.org>.
ascopes commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1083434014
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
Review Comment:
as a side note, don't think it is necesarilly related to this issue, but https://docs.oracle.com/cd/E23095_01/Platform.93/ATGProgGuide/html/s0204propertiesfileformat01.html suggests that `:` is a valid character here too.
Along with spaces around these operators, it may be worth passing `key` and `value` through `sed 's/[ \t]//g'` to deal with this case at the same time (or use TR).
e.g.
```
while IFS=':=' read -r key value; do
key="$(echo "${key}" | tr $' \t' '')"
value="$(echo "${value}" | tr $' \t\r')"
...
done
```
There is one other case that is potentially missed which is line continuations in the file but that is kinda out of scope 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] ascopes commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "ascopes (via GitHub)" <gi...@apache.org>.
ascopes commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1083434014
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
Review Comment:
as a side note, don't think it is necesarilly related to this issue, but https://docs.oracle.com/cd/E23095_01/Platform.93/ATGProgGuide/html/s0204propertiesfileformat01.html suggests that `:` is a valid character here too.
Along with spaces around these operators, it may be worth passing `key` and `value` through `sed 's/[ \t]//g'` to deal with this case at the same time (or use TR).
e.g.
```
while IFS=':=' read -r key value; do
key="$(echo "${key}" | tr $' \t' '')"
value="$(echo "${value}" | tr $' \t\r' '')"
...
done
```
There is one other case that is potentially missed which is line continuations in the file but that is kinda out of scope 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] slawekjaranowski commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1111061122
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove '\r' from value to allow usage on windows as IFS does not consider '\r' as a separator ( considers space, tab, new line ('\n'), and custom '=' )
+ safeValue=$(echo "$value" | sed 's/\r$//')
Review Comment:
Can you use `tr` instead of `sed` - it should be 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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] hazendaz commented on pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "hazendaz (via GitHub)" <gi...@apache.org>.
hazendaz commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1398906017
I think this repo would need gitattributes added to have the issue. Not sure what git does when that is missing and not telling it how to handle line endings.
basically this would need present
```
* text=auto
```
So that windows run is checked out and files end up with \r\n in 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@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] bmarwell commented on pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1398894819
So, can this be reproduced by using GitHub actions and a bash shell?
--
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] ascopes commented on pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "ascopes (via GitHub)" <gi...@apache.org>.
ascopes commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1399456163
This issue also affects another area, which I have put a PR up for at #86.
--
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] ascopes commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "ascopes (via GitHub)" <gi...@apache.org>.
ascopes commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1083434014
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
Review Comment:
as a side note, don't think it is necesarilly related to this issue, but https://docs.oracle.com/cd/E23095_01/Platform.93/ATGProgGuide/html/s0204propertiesfileformat01.html suggests that `:` is a valid character here too.
Along with spaces around these operators, it may be worth passing `key` and `value` through `sed 's/[ \t]//g'` to deal with this case at the same time (or use TR).
e.g.
while IFS=':=' read -r key value; do
key="$(echo "${key}" | tr $' \t' '')"
value="$(echo "${value}" | tr $' \t\r')"
...
done
There is one other case that is potentially missed which is line continuations in the file but that is kinda out of scope 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] michael-o commented on a diff in pull request #44: [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r875214048
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -192,7 +192,8 @@ 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
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove invalid line endings from value
+ case "$key" in (wrapperUrl) wrapperUrl="${value%$'\r'}"; break ;;
Review Comment:
...and you are sure this runs on a POSIX only shell?
--
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] hazendaz commented on pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "hazendaz (via GitHub)" <gi...@apache.org>.
hazendaz commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1436203731
Changed 'sed' to 'tr' per @slawekjaranowski request. Rebased and minor text changes on the commit message.
--
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 #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1436022941
@bmarwell If you think this one is good, please go ahead and merge.
--
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 a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1111314724
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove '\r' from value to allow usage on windows as IFS does not consider '\r' as a separator ( considers space, tab, new line ('\n'), and custom '=' )
+ safeValue=$(echo "$value" | sed 's/\r$//')
Review Comment:
> Or just use a bash variable expansion so we don't need to spawn another process. Given everyone is on bash 4 now.
NEVER expect something to be true/present in a target environment. That's Windows point of view.
--
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 a diff in pull request #44: [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r914154356
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -192,7 +192,8 @@ 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
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove invalid line endings from value
+ case "$key" in (wrapperUrl) wrapperUrl="${value%$'\r'}"; break ;;
Review Comment:
Not this month. I have basically no interest in the wrapper since I consider it more or less pointless.
--
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] norrisjeremy commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "norrisjeremy (via GitHub)" <gi...@apache.org>.
norrisjeremy commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1111314483
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove '\r' from value to allow usage on windows as IFS does not consider '\r' as a separator ( considers space, tab, new line ('\n'), and custom '=' )
+ safeValue=$(echo "$value" | sed 's/\r$//')
Review Comment:
> Or just use a bash variable expansion so we don't need to spawn another process. Given everyone is on bash 4 now.
That would be wrong assumption.
On macOS:
```
$ /bin/sh --version
GNU bash, version 3.2.57(1)-release (arm64-apple-darwin22)
Copyright (C) 2007 Free Software Foundation, Inc.
```
--
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] ascopes commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "ascopes (via GitHub)" <gi...@apache.org>.
ascopes commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1083433677
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove '\r' from value to allow usage on windows as IFS does not consider '\r' as a separator ( considers space, tab, new line ('\n'), and custom '=' )
+ safeValue=$(echo "$value" | sed 's/\r$//')
Review Comment:
using
```
`command`
```
may be favourable compared to
```
$(...)
```
...as some sources specify that $() doesn't appear to be supported by csh.
--
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] hazendaz commented on pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by GitBox <gi...@apache.org>.
hazendaz commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1383331865
Adjusted to be posix compliant. This will strip out any \r but if \r\n at end of line, it strips both but that should be fine given this is used for property value pairs and not expecting \n either but IFS is ignoring that one by design.
Sample test I did on this. In sample I have mixed in \r\n between words and at end to better show removal of \r along with fact last \n will get removed due to placement.
```
$ value=$'firstLine\r\nsecondLine\r\n'
$ set | grep "^value="
value=$'firstLine\r\nsecondLine\r\n'
$ safeValue=$(echo "$value" | sed 's/\r$//')
$ set | grep "^safeValue="
safeValue=$'firstLine\nsecondLine'
```
--
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 #44: [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1129200616
I Prefer the source to be fixed.
--
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] hazendaz commented on pull request #44: [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script
Posted by GitBox <gi...@apache.org>.
hazendaz commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1129254265
@michael-o The issue is from how the property file is read. Its reading the entire line so it gets line feed. It really should not pick up the line feed. I have changed the logic to fix at source of issue at least removing exact same as I did initially. So basically the same fix just at root of the issue. The file itself doesn't need fixed. Its irrelevent. The problem is the read from that file picking up unnecessary content. Making the file either windows or unix based is IMHO not the right fix.
--
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] hazendaz commented on pull request #44: [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script
Posted by GitBox <gi...@apache.org>.
hazendaz commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1370262620
@michael-o Ping, any possibility of this being merged before next release happens?
--
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] hazendaz commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "hazendaz (via GitHub)" <gi...@apache.org>.
hazendaz commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1111384854
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove '\r' from value to allow usage on windows as IFS does not consider '\r' as a separator ( considers space, tab, new line ('\n'), and custom '=' )
+ safeValue=$(echo "$value" | sed 's/\r$//')
Review Comment:
correct, my original code had bash expansion but that fails posix. Suggested change from 'sed' to 'tr' though makes sense. Looking at that now.
--
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] hazendaz commented on pull request #44: [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script
Posted by GitBox <gi...@apache.org>.
hazendaz commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1129200037
The script file can be either windows or Unix based as that doesn't seem to cause issue. Possibly it's from the read of wrapper properties. Tried printing the data there but all looked ok. Adding this did fix it.
Sent from my Verizon, Samsung Galaxy smartphone
Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: Michael Osipov ***@***.***>
Sent: Tuesday, May 17, 2022 2:33:27 PM
To: apache/maven-wrapper ***@***.***>
Cc: Jeremy Landis ***@***.***>; Author ***@***.***>
Subject: Re: [apache/maven-wrapper] [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script (PR #44)
@michael-o commented on this pull request.
________________________________
In maven-wrapper-distribution/src/resources/mvnw<https://github.com/apache/maven-wrapper/pull/44#discussion_r875146529>:
> @@ -198,6 +198,8 @@ else
if [ "$MVNW_VERBOSE" = true ]; then
echo "Downloading from: $wrapperUrl"
fi
+ # Remove invalid line endings from wrapperUrl
+ wrapperUrl=${wrapperUrl%$'\r'}
Where does the URL come from? Do we need to fix the line ending of this resource in Git?
—
Reply to this email directly, view it on GitHub<https://github.com/apache/maven-wrapper/pull/44#pullrequestreview-975930586>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHODI3AEZNI7SN3VKRO57DVKPQ7PANCNFSM5WFVGSHQ>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
--
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] hazendaz commented on a diff in pull request #44: [MWRAPPER-67] Remove invalid line endings from wrapperUrl when used with git bash with mvnw script
Posted by GitBox <gi...@apache.org>.
hazendaz commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r875324613
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -192,7 +192,8 @@ 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
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove invalid line endings from value
+ case "$key" in (wrapperUrl) wrapperUrl="${value%$'\r'}"; break ;;
Review Comment:
My testing was on Windows 10, Windows 2016 server, and RHEL 8. So I don't know for sure but the logic there is posix compliant per multiple sources such as [here](https://www.ibm.com/docs/en/aix/7.1?topic=shell-parameter-substitution-in-korn-posix) and [here](https://unix.stackexchange.com/questions/389209/what-kinds-of-string-interpolation-does-posix-sh-support).
--
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] hazendaz commented on pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by GitBox <gi...@apache.org>.
hazendaz commented on PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#issuecomment-1383305467
rebased, adjusted title, squashed commits.
--
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] ascopes commented on a diff in pull request #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "ascopes (via GitHub)" <gi...@apache.org>.
ascopes commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1083434014
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
Review Comment:
as a side note, don't think it is necesarilly related to this issue, but https://docs.oracle.com/cd/E23095_01/Platform.93/ATGProgGuide/html/s0204propertiesfileformat01.html suggests that `:` is a valid character here too.
--
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 #44: [MWRAPPER-67] Remove '\r' when using IFS on windows for wrapperUrl from mvnw script usage such as git bash
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on code in PR #44:
URL: https://github.com/apache/maven-wrapper/pull/44#discussion_r1111114280
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -194,7 +194,9 @@ 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 -r key value; do
- case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
+ # Remove '\r' from value to allow usage on windows as IFS does not consider '\r' as a separator ( considers space, tab, new line ('\n'), and custom '=' )
+ safeValue=$(echo "$value" | sed 's/\r$//')
Review Comment:
Or just use a bash variable expansion so we don't need to spawn another process.
Given everyone is on bash 4 now.
--
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