RFR: JDK-8182107: javax.tools.ToolProvider should detect if jdk.compiler matches the version of java.compiler

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8182107: javax.tools.ToolProvider should detect if jdk.compiler matches the version of java.compiler

Jonathan Gibbons
Please review this simple fix for javax.tools.ToolProvider, to verify
that if a candidate impl of
JavaCompiler is found, it is in a module with the sane version as that
of the java.compiler module.

The fix also changes the code to use direct API to access module
details, instead of reflection,
which is no longer necessary.

The new test performs a "control" experiment to verify that ToolProvider
continues to work as
expected in a normal system image. It then creates an updated modular
jar containing the
classes for java.compiler, but with an updated module version, and
verifies that when using this
module on the upgrade module path, the jdk.compiler in the system image
is ignored.

JBS: https://bugs.openjdk.java.net/browse/JDK-8182107
Webrev: http://cr.openjdk.java.net/~jjg/8182107/webrev.00/

-- Jon
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8182107: javax.tools.ToolProvider should detect if jdk.compiler matches the version of java.compiler

Mandy Chung


On 1/9/18 1:39 PM, Jonathan Gibbons wrote:
Please review this simple fix for javax.tools.ToolProvider, to verify that if a candidate impl of
JavaCompiler is found, it is in a module with the sane version as that of the java.compiler module.

The fix also changes the code to use direct API to access module details, instead of reflection,
which is no longer necessary.

The new test performs a "control" experiment to verify that ToolProvider continues to work as
expected in a normal system image. It then creates an updated modular jar containing the
classes for java.compiler, but with an updated module version, and verifies that when using this
module on the upgrade module path, the jdk.compiler in the system image is ignored.

JBS: https://bugs.openjdk.java.net/browse/JDK-8182107
Webrev: http://cr.openjdk.java.net/~jjg/8182107/webrev.00/


Making sure that the module version matching the version of java.compiler module is reasonable.  I think @implNote of getSystemXXXTool may need to be updated to mention the module version check.  I wondered if it should support interim release (i.e. match the feature version rather than the entire string) but it does not seem necessary since the primary use case is for IDE to upgrade a newer version of java.compiler which is a feature release.  So the check is fine.

Is useLegacy still needed?  If only supporting to run on JDK N-1, then it can be removed?

Nit: you could do ToolProvider.class.getModule().getDescriptor().version().orElse(null) so that the matches method can take Version instead of Optional<Version>

VersionTest.java

  83         String thisVersion = System.getProperty("java.specification.version");
  84         String nextVersion = String.valueOf(Integer.parseInt(thisVersion) + 1);


An alternative way is Runtime.version().feature() + 1;

Mandy
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8194901: remove interim code from javax.tools.ToolProvider

Jonathan Gibbons
In reply to this post by Jonathan Gibbons
This is a subset of an earlier review, the remainder of which has been
deferred to JDK 11,
pending design and spec clarifications.

This review just removes some interim code that was necessary in JDK 9.

JBS: https://bugs.openjdk.java.net/browse/JDK-8194901
Webrev: http://cr.openjdk.java.net/~jjg/8194901/webrev.00/

-- Jon
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8194901: remove interim code from javax.tools.ToolProvider

Mandy Chung


On 1/10/18 2:57 PM, Jonathan Gibbons wrote:
This is a subset of an earlier review, the remainder of which has been deferred to JDK 11,
pending design and spec clarifications.

This review just removes some interim code that was necessary in JDK 9.

JBS: https://bugs.openjdk.java.net/browse/JDK-8194901
Webrev: http://cr.openjdk.java.net/~jjg/8194901/webrev.00/


Looks good.

Mandy