You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/06/10 14:55:36 UTC
[GitHub] [pulsar] lgxbslgx opened a new pull request, #16008: [fix][env] Support the java version like 'XX-internal' in env file
lgxbslgx opened a new pull request, #16008:
URL: https://github.com/apache/pulsar/pull/16008
Related to #15670, cc'ing its author @lhotari .
### Motivation
My local JDKs are almost built from the source code, such as [JDK Mainline](https://github.com/openjdk/jdk), so when I use the command `java -version`, I may get the following output message:
```
openjdk version "19-internal" 2022-09-20
OpenJDK Runtime Environment (build 19-internal-adhoc.lgx.jdk)
OpenJDK 64-Bit Server VM (build 19-internal-adhoc.lgx.jdk, mixed mode, sharing)
or
openjdk version "17-internal" 2021-09-14
OpenJDK Runtime Environment (build 17-internal+0-adhoc.lgx.jdk17u)
OpenJDK 64-Bit Server VM (build 17-internal+0-adhoc.lgx.jdk17u, mixed mode, sharing)
```
Unfortunately, the code of `pulsar_env.sh` which was integrated in #15670 yesterday didn't support it, so when I use `./bin/pulsar standalone` locally, I get the following error message:
```
[0.000s][warning][gc] -Xloggc is deprecated. Will use -Xlog:gc:/home/lgx/source/java/pulsar/logs/pulsar_gc_%p.log instead.
Unrecognized VM option 'PrintGCDateStamps'
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
```
I know I can use the JDK release version to avoid this issue. But I am an JDK developer and almost use the internal version built from the source code. By using such internal version, I can find and solve some bugs related to both OpenJDK and Pulsar as soon as possible.
### Modifications
Add the conditional statement into file `pulsar_env.sh` to support the java internal version.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
- [x] `doc-not-needed`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lgxbslgx commented on pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lgxbslgx commented on PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#issuecomment-1154223026
@Jason918 @lhotari Thanks for the review. Could I get your help to commit this patch?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#issuecomment-1154786281
> @Jason918 @lhotari Thanks for the review. Could I get your help to commit this patch?
@lgxbslgx merged. thanks for the contribution!
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lhotari merged PR #16008:
URL: https://github.com/apache/pulsar/pull/16008
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r895139457
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,10 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
I think it's better to include `-internal` as part of the pattern to make this explicit.
The version parsing logic is also duplicated in bkenv.sh. That would need to be addressed 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r895673728
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,10 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
@lgxbslgx Makes sense.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r895220305
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,10 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
The version number syntax is standardized, so why would there be a need for other syntaxes? If a new format is added, we could deal with it if that ever 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lgxbslgx commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lgxbslgx commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r895148678
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,10 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
> I think it's better to include -internal as part of the pattern to make this explicit.
I suspect whether there are other special versions, so I use this loose pattern which only identities the first digit number. I don't think it is good to restrict it as `-internal`. But I will adjust the code if @lhotari insists his opinion after second thought.
> The version parsing logic is also duplicated in bkenv.sh. That would need to be addressed too.
I will address `bkenv.sh` soon.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r895220542
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,10 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
https://openjdk.java.net/jeps/223 .
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lgxbslgx commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lgxbslgx commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r895659214
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,10 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
> https://openjdk.java.net/jeps/223 .
Thanks for pointing out the standard. I list the following important information.
```
Version numbers $VNUM
[1-9][0-9]*((\.0)*\.[1-9][0-9]*)*
Version strings $VSTR
$VNUM(-$PRE)?\+$BUILD(-$OPT)?
$VNUM-$PRE(-$OPT)?
$VNUM(+-$OPT)?
Name Syntax
------------------------------ --------------
java.version $VNUM(\-$PRE)?
java.runtime.version $VSTR
java.vm.version $VSTR
java.specification.version $VNUM
java.vm.specification.version $VNUM
$ java -version
openjdk version \"${java.version}\"
${java.runtime.name} (build ${java.runtime.version})
${java.vm.name} (build ${java.vm.version}, ${java.vm.info})
```
From the standard, we can know the `java.version` is started by a **necessary major version number**, then followed by zero or several "sub-version"(minor/security), and finally ended with a optional `-$PRE`.
So I can confirm that the newly added pattern `\"([[:digit:]]+)(.*)\"` can address all the common cases of the standard. And the pattern `\"([[:digit:]]+)\.([[:digit:]]+)\.(.*)\"` you added previously solves the vein that the `java.version` has at aleast one "sub-version".
> I guess it would make sense if the pattern handles the $PRE format described by the JEP
If we want to handle the `$PRE` format by changing the pattern `\"([[:digit:]]+)(.*)\"` . I may ask: should we need to change the `\"([[:digit:]]+)\.([[:digit:]]+)\.(.*)\"` to handle the `$PRE` too? If the answer is no, why we need to handle `$PRE` in this new common pattern.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lgxbslgx commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lgxbslgx commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r895671920
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,10 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
From the standard, I find the pattern `\"([[:digit:]]+)\.([[:digit:]]+)\.(.*)\"` can't handle the version like `1.9-XXX`. I remove the unnecessary latest dot `\.`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r894974404
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,9 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
It would be nice to add some comment about this case 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r895220813
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,10 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
I guess it would make sense if the pattern handles the $PRE format described by the JEP
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lgxbslgx commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lgxbslgx commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r894975765
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,9 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
What about such comment: `# Process the java versions without dots, such as "17-internal".`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lgxbslgx commented on a diff in pull request #16008: [fix][env] Support the java version like 'XX-internal' in env file
Posted by GitBox <gi...@apache.org>.
lgxbslgx commented on code in PR #16008:
URL: https://github.com/apache/pulsar/pull/16008#discussion_r895659214
##########
conf/pulsar_env.sh:
##########
@@ -60,6 +60,10 @@ for token in $("$JAVA_BIN" -version 2>&1 | grep 'version "'); do
JAVA_MAJOR_VERSION=${BASH_REMATCH[1]}
fi
break
+ elif [[ $token =~ \"([[:digit:]]+)(.*)\" ]]; then
Review Comment:
> https://openjdk.java.net/jeps/223 .
Thanks for pointing out the standard. I list the following important information.
```
Version numbers $VNUM
[1-9][0-9]*((\.0)*\.[1-9][0-9]*)*
Version strings $VSTR
$VNUM(-$PRE)?\+$BUILD(-$OPT)?
$VNUM-$PRE(-$OPT)?
$VNUM(+-$OPT)?
Name Syntax
------------------------------ --------------
java.version $VNUM(\-$PRE)?
java.runtime.version $VSTR
java.vm.version $VSTR
java.specification.version $VNUM
java.vm.specification.version $VNUM
$ java -version
openjdk version \"${java.version}\"
${java.runtime.name} (build ${java.runtime.version})
${java.vm.name} (build ${java.vm.version}, ${java.vm.info})
```
From the standard, we can know the `java.version` is started by a **necessary major version number**, then followed by zero or several "sub-version"(minor/security), and finally ended with a optional `-$PRE`.
So I can confirm that the newly added pattern `\"([[:digit:]]+)(.*)\"` can address all the common cases of the standard. And the pattern `\"([[:digit:]]+)\.([[:digit:]]+)\.(.*)\"` you added previously solves the vein which the `java.version` has at aleast one "sub-version".
> I guess it would make sense if the pattern handles the $PRE format described by the JEP
If we want to handle the `$PRE` format by changing the pattern `\"([[:digit:]]+)(.*)\"` . I may ask: should we need to change the `\"([[:digit:]]+)\.([[:digit:]]+)\.(.*)\"` to handle the `$PRE` too? If the answer is no, why we need to handle `$PRE` in this new common pattern.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org