You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "bmarwell (via GitHub)" <gi...@apache.org> on 2024/02/01 07:23:28 UTC
[PR] [MWRAPPER-123] replace bogus printf %d with constant 104 for now [maven-wrapper]
bmarwell opened a new pull request, #117:
URL: https://github.com/apache/maven-wrapper/pull/117
Old behaviour was to always use h * 331 + 104 (because on most shells, `printf %d "'%str"` always evaluated to 104).
For now, use this constant as this does not change behavour.
It might be feasible to do another kind of string-to-int conversion in another PR to implement the intended behaviour.
---
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-123] - 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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#issuecomment-1923055548
My test:
```
hash_string() {
str="${1:-}" h=0
while [ -n "$str" ]; do
c="${str%${str#?}}"
h=$(( ( h * 31 + $(LC_CTYPE=C printf %d "'$c") ) % 4294967296 ))
str="${str#?}"
done
printf %x\\n $h
}
```
@bmarwell can you try on AIX?
--
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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#issuecomment-1923307605
```
# echo 'T="abcde"; echo "${T#?}"' | bash
bcde
# echo 'T="abcde"; echo "${T%${T#?}}"' | bash
a
```
--
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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#issuecomment-1924513620
> ```
> # echo 'T="abcde"; echo "${T#?}"' | bash
> bcde
>
> # echo 'T="abcde"; echo "${T%${T#?}}"' | bash
> a
> ```
I thought about test on AIX with ksh ...
--
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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#issuecomment-1923265987
just out of curiosity, what result will be:
```
echo 'T="abcde"; echo "${T#?}"' | bash
echo 'T="abcde"; echo "${T%${T#?}}"' | bash
```
--
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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on code in PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#discussion_r1474180787
##########
maven-wrapper-distribution/src/resources/only-mvnw:
##########
@@ -70,11 +70,19 @@ set_java_home() {
# hash string like Java String::hashCode
hash_string() {
- str="${1:-}" h=0
- while [ -n "$str" ]; do
- h=$(( ( h * 31 + $(LC_CTYPE=C printf %d "'$str") ) % 4294967296 ))
- str="${str#?}"
+ str="${1}"
+ length=${#str}
+ h=0
+ # strings start with index 1
+ idx=1
+
+ # lt: skip line break at end
+ while [ $idx -lt "$length" ]; do
+ char=$( printf "%s" "$str" | cut -c $idx )
Review Comment:
According to Ubuntu, they still use Dash as default symlink target of `/bin/sh` which does not know about variable substitution: https://wiki.ubuntu.com/DashAsBinSh
They recommend using awk (but cut is enough 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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#issuecomment-1925303195
> > ```
> > # echo 'T="abcde"; echo "${T#?}"' | bash
> > bcde
> >
> > # echo 'T="abcde"; echo "${T%${T#?}}"' | bash
> > a
> > ```
>
> I thought about test on AIX with ksh ...
```
# echo 'T="abcde"; echo "${T#?}"' | ksh
bcde
echo 'T="abcde"; echo "${T%${T#?}}"' | ksh
a
```
I think I know what you have in mind...
--
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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell merged PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117
--
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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on code in PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#discussion_r1474169068
##########
maven-wrapper-distribution/src/resources/only-mvnw:
##########
@@ -70,11 +70,19 @@ set_java_home() {
# hash string like Java String::hashCode
hash_string() {
- str="${1:-}" h=0
- while [ -n "$str" ]; do
- h=$(( ( h * 31 + $(LC_CTYPE=C printf %d "'$str") ) % 4294967296 ))
- str="${str#?}"
+ str="${1}"
+ length=${#str}
+ h=0
+ # strings start with index 1
+ idx=1
+
+ # lt: skip line break at end
+ while [ $idx -lt "$length" ]; do
+ char=$( printf "%s" "$str" | cut -c $idx )
Review Comment:
Fork here, alternative is to use string index (supported by ksh and bash and zsh). However:
```
In maven-wrapper-distribution/src/resources/only-mvnw line 80:
char=${str:${idx}:1}
^-------------^ SC3057 (warning): In POSIX sh, string indexing is undefined.
```
--
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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on code in PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#discussion_r1474170110
##########
maven-wrapper-distribution/src/resources/only-mvnw:
##########
@@ -70,11 +70,19 @@ set_java_home() {
# hash string like Java String::hashCode
hash_string() {
- str="${1:-}" h=0
- while [ -n "$str" ]; do
- h=$(( ( h * 31 + $(LC_CTYPE=C printf %d "'$str") ) % 4294967296 ))
- str="${str#?}"
+ str="${1}"
+ length=${#str}
+ h=0
+ # strings start with index 1
+ idx=1
+
+ # lt: skip line break at end
+ while [ $idx -lt "$length" ]; do
+ char=$( printf "%s" "$str" | cut -c $idx )
Review Comment:
The other advantage is that string index starts at 0 😄
--
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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#issuecomment-1925305248
I think we can replace this
```
# strings start with index 1
idx=1
# lt: skip line break at end
while [ $idx -lt "$length" ]; do
char=$( printf "%s" "$str" | cut -c $idx )
h=$(( ( h * 31 + $(LC_CTYPE=C printf %d "'$char") ) % 4294967296 ))
idx=$(( idx + 1 ))
done
```
with this:
```
while [ "$str" != "" ]; do
char="${str%${str#?}}"
h=$(( ( h * 31 + $(LC_CTYPE=C printf %d "'$char") ) % 4294967296 ))
str="${T#?}"
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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#issuecomment-1923203779
> My test:
>
> ```
> hash_string() {
> str="${1:-}" h=0
> while [ -n "$str" ]; do
> c="${str%${str#?}}"
> h=$(( ( h * 31 + $(LC_CTYPE=C printf %d "'$c") ) % 4294967296 ))
> str="${str#?}"
> done
> printf %x\\n $h
> }
> ```
>
> @bmarwell can you try on AIX?
sadly...
```
# vi test.sh
# ksh test.sh "https://reopsitory/repository/maven-central/org/apache/maven/apache-maven/3.9.6/apache-maven-3.9.6-bin.zip"
printf: 2589392689: A return value of a math subroutine is not within machine precision.
7fffffff
```
--
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
Re: [PR] [MWRAPPER-123] replace bogus printf %d with constant 104 for now [maven-wrapper]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#discussion_r1473937034
##########
maven-wrapper-distribution/src/resources/only-mvnw:
##########
@@ -72,7 +72,9 @@ set_java_home() {
hash_string() {
str="${1:-}" h=0
while [ -n "$str" ]; do
- h=$(( ( h * 31 + $(LC_CTYPE=C printf %d "'$str") ) % 4294967296 ))
+ # TODO: convert the input 'str' to an int for hash calculation
+ # printf '%d' will only convert the first character, and breaks on some ksh implementations.
+ h=$(( ( h * 31 + 104 ) % 4294967296 ))
Review Comment:
dont think we can do that due to next line which mutate str so str is not a constant in the loop, it shifts the first ascii char one by one so I guess a better fix is to use the substitution so do a substring of 1 char to let all shell handle the "string to char" (`'`) conversion working
--
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
Re: [PR] [MWRAPPER-123] hash string char-by-char for AIX's ksh implementation [maven-wrapper]
Posted by "bmarwell (via GitHub)" <gi...@apache.org>.
bmarwell commented on PR #117:
URL: https://github.com/apache/maven-wrapper/pull/117#issuecomment-1921087256
> ok for me but kind of yet another proof we don't want to have a shell script with any logic so I would move to a java/jsh or anything else script since wrapper will no more be used with older jvm version
100 %
I still don't like the subshell (`| cut `). If anyone can come up with a better solution, let us know.
--
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