Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PAYARA-3911 Fix domain.xml java-home is not honored in version-specific jvm-options (#4025) #4034

Merged
merged 4 commits into from
Jul 4, 2019

Conversation

vlumi
Copy link
Contributor

@vlumi vlumi commented Jun 11, 2019

Fix GFLauncher to use the correct JDK when determining the JVM options.

Using the javaExe already set for launching the domain, run it first with the -version parameter, and use the version parsed from its output (if available and matching) when setting the JVM options instead of the JVM currently running.

In case of non-fatal errors, the functionality falls back on the current JVM version (like before). On fatal errors a GFLauncherException will be thrown.

Fixes #4025

@smillidge smillidge added PR: CLA CLA submitted on PR by the contributor community-contribution and removed PR: CLA CLA submitted on PR by the contributor community-contribution labels Jun 11, 2019
@Pandrex247 Pandrex247 changed the title Fix #4025 PAYARA-3911 Fix #4025 Jun 12, 2019
@jbee
Copy link
Contributor

jbee commented Jun 13, 2019

jenkins test please

@smillidge smillidge requested review from Pandrex247 and Cousjava June 14, 2019 18:33
Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with the launcher but had a look. It is clear that java version of a domain cannot be assumed to be the same as the one running the launcher. This seems a bit odd at first but it somewhat makes sense.

I see three ways to get the target JRE version:

  • extract from path name (in the hope it is contained, probably not very reliable)
  • extract via java -version (as done in this PR)
  • extract via a small program run with the target JRE that uses System.getProperty("java.specification.version") (as done for the JRE_VERSION constant).

I start to wonder if "java.specification.version" is the right property at all since it will not allow to distinguish specific builds.

In conclusion I think using java -version is a good choice. I am unsure about the pattern though.
E.g. it would not match the output of my current JDK:

$ java -version
openjdk version "1.8.0_171"

Maybe .*? version \"([^\"]+)\".* is flexible enough.

@vlumi
Copy link
Contributor Author

vlumi commented Jun 18, 2019

I am unsure about the pattern though.

I'll try to find some specification for the output of java -version and update the PR.

In addition to the pattern, an assumption is also made that it will be on the first line -- hopefully some specification will give these assumptions a more solid foundation.

@vlumi
Copy link
Contributor Author

vlumi commented Jun 18, 2019

extract via a small program run with the target JRE that uses System.getProperty("java.specification.version") (as done for the JRE_VERSION constant).

I could not find this pattern in the Payara repository -- did I overlook some place? The same version as output by java -version is in the property java.version, and an even more detailed one on java.runtime.version.

The only document I found describing the version output format is a pretty old one: https://www.oracle.com/technetwork/java/javase/versioning-naming-139433.html

The output appears to be stable, with the version number always on the first line, with the expected format. Also, according to the specification, the output always matches the system property, so either approach should give the same result.

Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to

String javaSpecificationVersion = System.getProperty("java.specification.version");

Saw now that also java.version is taken into consideration. Missed that before.

…om the Java version pattern, since they are not part of the version proper for JVM options.
@vlumi
Copy link
Contributor Author

vlumi commented Jun 18, 2019

Found one more edge case that was not working correctly -- pre-release (-ea) versions, e.g. "9-ea". According to JEP 223, the version string may be added by pre-release and/or build information, which starts with a hyphen.

This is handled in JDK.initialize() by simply ignoring everything followed by the first hyphen -- I have updated this PR to take the same approach, only considering the pre-hyphen part as the version proper.

@jbee
Copy link
Contributor

jbee commented Jun 18, 2019

@vlumi Thanks for looking into the JEPs and adopting the expression.

@jbee
Copy link
Contributor

jbee commented Jun 18, 2019

jenkins test please

@arjantijms
Copy link
Contributor

Note that a modified version of the same JDK class that was updated here also exists in this repo:

https://github.com/payara/ecosystem-eclipse-plugin/blob/master/plugins/org.eclipse.payara.tools/src/org/eclipse/payara/tools/sdk/server/JDK.java

See specifically line 292 and onwards.

@vlumi
Copy link
Contributor Author

vlumi commented Jun 20, 2019

Another one is in ecosystem-netbeans-plugin, too:

https://github.com/payara/ecosystem-netbeans-plugin/blob/master/payara.tooling/src/org/netbeans/modules/payara/tooling/server/JDK.java

This one is almost identical to the one in this PR.

@arjantijms arjantijms changed the title PAYARA-3911 Fix #4025 PAYARA-3911 Fix domain.xml java-home is not honored in version-specific jvm-options (#4025) Jul 4, 2019
@arjantijms arjantijms merged commit b02b03d into payara:master Jul 4, 2019
@arjantijms arjantijms added this to the 5.193 milestone Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: CLA CLA submitted on PR by the contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The domain.xml java-home is not honored in version-specific jvm-options PAYARA-3911
5 participants