[WFCORE-6202] Don't set -server option for IBM i
#5355
Conversation
|
Hello, ThePrez. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment. |
github-actions
bot
added
the
deps-ok
|
/ok-to-test |
|
Hi @ThePrez -- I wonder if we can just drop that setting altogether. See discussion at https://wildfly.zulipchat.com/#narrow/stream/174184-wildfly-developers/topic/JVM.20-server.20option |
|
@bstansberry, given the Zulip discussion, I cleaned this up to just get rid of the |
There was a problem hiding this comment.
Thanks, @ThePrez , LGTM, added a minor comment, but that's optional in my opinion.
@jamezp Would you mind approving this as well?
We still have logic regarding "-client" option on the windows and PowerShell variants, but those are not relevant to fix the issue with the IBM I and I haven't spotted any problems with those scripts. Domain mode scripts don't use the "-client" option.
core-feature-pack/common/src/main/resources/content/bin/standalone.sh
Outdated
Show resolved
Hide resolved
|
Removed |
|
Core -> Full Integration Build 12175 outcome was UNKNOWN using a merge of 2e52474 |
|
Core -> WildFly Preview Integration Build 12104 outcome was UNKNOWN using a merge of 2e52474 |
|
Core -> Full Integration Build 11981 outcome was UNKNOWN using a merge of 2e52474 |
| # If -server not set in JAVA_OPTS, set it, if supported | ||
| SERVER_SET=`echo $JAVA_OPTS | $GREP "\-server"` | ||
| if [ "x$SERVER_SET" = "x" ]; then | ||
|
|
||
| # Check for SUN(tm) JVM w/ HotSpot support | ||
| if [ "x$HAS_HOTSPOT" = "x" ]; then | ||
| HAS_HOTSPOT=`"$JAVA" $JVM_OPTVERSION -version 2>&1 | $GREP -i HotSpot` | ||
| fi | ||
|
|
||
| # Check for OpenJDK JVM w/server support | ||
| if [ "x$HAS_OPENJDK" = "x" ]; then | ||
| HAS_OPENJDK=`"$JAVA" $JVM_OPTVERSION 2>&1 | $GREP -i OpenJDK` | ||
| fi | ||
|
|
||
| # Check for IBM JVM w/server support | ||
| if [ "x$HAS_IBM" = "x" ]; then | ||
| HAS_IBM=`"$JAVA" $JVM_OPTVERSION 2>&1 | $GREP -i "IBM J9"` | ||
| fi | ||
|
|
||
| # Enable -server if we have Hotspot or OpenJDK, unless we can't | ||
| if [ "x$HAS_HOTSPOT" != "x" -o "x$HAS_OPENJDK" != "x" -o "x$HAS_IBM" != "x" ]; then | ||
| # MacOS does not support -server flag | ||
| if [ "$darwin" != "true" ]; then | ||
| PROCESS_CONTROLLER_JAVA_OPTS="-server $PROCESS_CONTROLLER_JAVA_OPTS" | ||
| HOST_CONTROLLER_JAVA_OPTS="-server $HOST_CONTROLLER_JAVA_OPTS" | ||
| JVM_OPTVERSION="-server $JVM_OPTVERSION" | ||
| fi | ||
| fi | ||
| else | ||
| JVM_OPTVERSION="-server $JVM_OPTVERSION" | ||
| fi | ||
|
|
There was a problem hiding this comment.
I think it is harmless, the code is prepared to add the "-server" setting automatically if it is supported. The same is applicable to the standalone.bat with -client and -server.
@ThePrez I could be missing something, what's the reason for the removal?
There was a problem hiding this comment.
This code in domain.sh also breaks with system Java on IBM i (domain mode is likely very uncommon on IBM i so I didn't do proper testing with it).
Rather than cleaning it up so it works, removal seemed to make the most sense, given the comments on Zulip. The -server option has lost its relevancy over time.
There was a problem hiding this comment.
Ok. I thought it was not going to fail since the above code only will add the "-server" option for HotSpot/OpenJDK and IBM J9, but I have no experience with the JVMs in an IBM i, and I'm not sure now what it version returns.
In any case, you're right, yes, the idea was to get rid of these options. In such a case, would you mind expanding the removal starting from L101? We can also get rid of the JVM_OPTVERSION, JVM_D64_OPTION, and JVM_D32_OPTION variables.
We should also clean up the same on domain.bat variant so both are in sync regarding this.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok. I thought it was not going to fail since the above code only will add the "-server" option for HotSpot/OpenJDK and IBM J9
FWIW, System Java on this platform is IBM J9, but not all flavors of IBM J9 support -server 😖
There was a problem hiding this comment.
Ok, I thought we were going to have a different version, then removing that completely is safest for being able to use it on IBM i.
| # If -server not set in JAVA_OPTS, set it, if supported | ||
| SERVER_SET=`echo $JAVA_OPTS | $GREP "\-server"` | ||
| if [ "x$SERVER_SET" = "x" ]; then | ||
|
|
||
| # Check for SUN(tm) JVM w/ HotSpot support | ||
| if [ "x$HAS_HOTSPOT" = "x" ]; then | ||
| HAS_HOTSPOT=`"$JAVA" $JVM_OPTVERSION -version 2>&1 | $GREP -i HotSpot` | ||
| fi | ||
|
|
||
| # Check for OpenJDK JVM w/server support | ||
| if [ "x$HAS_OPENJDK" = "x" ]; then | ||
| HAS_OPENJDK=`"$JAVA" $JVM_OPTVERSION 2>&1 | $GREP -i OpenJDK` | ||
| fi | ||
|
|
||
| # Check for IBM JVM w/server support | ||
| if [ "x$HAS_IBM" = "x" ]; then | ||
| HAS_IBM=`"$JAVA" $JVM_OPTVERSION 2>&1 | $GREP -i "IBM J9"` | ||
| fi | ||
|
|
||
| # Enable -server if we have Hotspot or OpenJDK, unless we can't | ||
| if [ "x$HAS_HOTSPOT" != "x" -o "x$HAS_OPENJDK" != "x" -o "x$HAS_IBM" != "x" ]; then | ||
| # MacOS does not support -server flag | ||
| if [ "$darwin" != "true" ]; then | ||
| PROCESS_CONTROLLER_JAVA_OPTS="-server $PROCESS_CONTROLLER_JAVA_OPTS" | ||
| HOST_CONTROLLER_JAVA_OPTS="-server $HOST_CONTROLLER_JAVA_OPTS" | ||
| JVM_OPTVERSION="-server $JVM_OPTVERSION" | ||
| fi | ||
| fi | ||
| else | ||
| JVM_OPTVERSION="-server $JVM_OPTVERSION" | ||
| fi | ||
|
|
There was a problem hiding this comment.
Ok. I thought it was not going to fail since the above code only will add the "-server" option for HotSpot/OpenJDK and IBM J9, but I have no experience with the JVMs in an IBM i, and I'm not sure now what it version returns.
In any case, you're right, yes, the idea was to get rid of these options. In such a case, would you mind expanding the removal starting from L101? We can also get rid of the JVM_OPTVERSION, JVM_D64_OPTION, and JVM_D32_OPTION variables.
We should also clean up the same on domain.bat variant so both are in sync regarding this.
|
The # Check for -d32/-d64 in JAVA_OPTS
JVM_D64_OPTION=`echo $JAVA_OPTS | $GREP "\-d64"`
JVM_D32_OPTION=`echo $JAVA_OPTS | $GREP "\-d32"`
# Check If -server is specified
SERVER_SET=`echo $JAVA_OPTS | $GREP "\-server"`
if [ "x$JVM_D32_OPTION" != "x" ]; then
JVM_OPTVERSION="-d32"
elif [ "x$JVM_D64_OPTION" != "x" ]; then
JVM_OPTVERSION="-d64"
elif $darwin && [ "x$SERVER_SET" = "x" ]; then
# Use 32-bit on Mac, unless server has been specified or the user opts are incompatible
"$JAVA" -d32 $JAVA_OPTS -version > /dev/null 2>&1 && PREPEND_JAVA_OPTS="-d32" && JVM_OPTVERSION="-d32"
fiSeems unnecessary and dated, but harmless. Opinions? |
@ThePrez apologies for the delay in giving a response. The -d32/64 options look outdated and my understanding is they are only valid for systems where you can have a 32/64 JDK installed at the same time and you want to choose the bit mode. This seems only possible on Solaris. I think we can safely remove this check, even more nowadays where WildFly's minimal JDK version is JDK 11. |
|
@ThePrez I forgot to mention that we need to squash the commits, that help us to move the change across other branches. Thanks! |
I agree. Removed at latest commit |
No problem. Squashed! |
|
Core -> Full Integration Build 12203 outcome was FAILURE using a merge of 714932b Failed tests |
core-feature-pack/common/src/main/resources/content/bin/common.ps1
Outdated
Show resolved
Hide resolved
yersan
dismissed
jamezp’s stale review
There were additional changes after this review, I will ask for another one on James side.
|
@jamezp , when you have a chance, would you mind reviewing this one? |
yersan
added
the
ready-for-merge
|
@ThePrez , merged. Thank you for contributing to WildFly! |
Addresses WFCORE-6202