[9] RFR: JDK-8054548: JAX-WS tools need to updated to work with modular image
Hello, please review following change: JBS: https://bugs.openjdk.java.net/browse/JDK-8054548 webrev: http://cr.openjdk.java.net/~mkos/8054548/jaxws.00/ It is basically replacing obsolete code using core reflection by javax.tools API plus removing old unused code. I ran the unit tests against both standalone JAX-WS and jdk, no regressions occurred. Thanks Miran
On 11/09/2014 12:20, Miroslav Kos wrote:
Hello, please review following change: JBS: https://bugs.openjdk.java.net/browse/JDK-8054548 webrev: http://cr.openjdk.java.net/~mkos/8054548/jaxws.00/
It is basically replacing obsolete code using core reflection by javax.tools API plus removing old unused code. Thanks for doing this, it's good to get this cleaned up.
The changes looks reasonable but I wonder if they are complete. I ask because I assumed that JavaCompilerHelper.getJarFile would be removed. Also there appears to be a bit of skullduggery in both wsimport and wsgen involving -Xbootclasspath/p where it might be assuming that the JAXB or JAX-WS classes are being loaded from JAR files (I didn't did into ParallelWorldClassLoader.toJarUrl so perhaps it can deal with other URLs). In passing, I assume the comments in Invoker should use "ClassLoader" or "class loader". Also the comment in createClassLoader mentions JAXB/WS 2.1 and not clear how this relates to the method description where it talked about 2.2 In JavacompilerMessages then noJavaCompilerError() and localizedNoJavaCompilerError() might be nicer names (in case you are looking for something better). -Alan.
On 11/09/14 14:39, Alan Bateman wrote:
On 11/09/2014 12:20, Miroslav Kos wrote:
Hello, please review following change: JBS: https://bugs.openjdk.java.net/browse/JDK-8054548 webrev: http://cr.openjdk.java.net/~mkos/8054548/jaxws.00/
It is basically replacing obsolete code using core reflection by javax.tools API plus removing old unused code. Thanks for doing this, it's good to get this cleaned up.
The changes looks reasonable but I wonder if they are complete. I ask because I assumed that JavaCompilerHelper.getJarFile would be removed. Also there appears to be a bit of skullduggery in both wsimport and wsgen involving -Xbootclasspath/p where it might be assuming that the JAXB or JAX-WS classes are being loaded from JAR files (I didn't did into ParallelWorldClassLoader.toJarUrl so perhaps it can deal with other URLs). I agree that the code looks scary, but it's because we have to support range of jdk (currently jdk6 to jdk9) and we want to avoid maintaining several branches. The standalone project must work even with jdk6. The magic here is to make the latest JAX-WS working with jdk6. 1.6 contain old JAX-B/WS API versions (2.1) and this tricky code allows to load newer (2.2) javax.xml.* classes from jars on classpath (without having to use java endorsed mechanism) when running standalone JAX-WS on the top of JDK6. Hope it is not too confusing.
Anyway I found some more code to cleanup - one more class can be removed - see version 2: http://cr.openjdk.java.net/~mkos/8054548/jaxws.01/
In passing, I assume the comments in Invoker should use "ClassLoader" or "class loader". fixed.
Also the comment in createClassLoader mentions JAXB/WS 2.1 and not clear how this relates to the method description where it talked about 2.2
In JavacompilerMessages then noJavaCompilerError() and localizedNoJavaCompilerError() might be nicer names (in case you are looking for something better). Those are generated methods from resource bundle using maven plugin. I think we own the plugin, but I would need to dive into it little bit, not sure if it is worth it.
M.
-Alan.
On 12/09/2014 09:39, Miroslav Kos wrote:
I agree that the code looks scary, but it's because we have to support range of jdk (currently jdk6 to jdk9) and we want to avoid maintaining several branches. The standalone project must work even with jdk6. The magic here is to make the latest JAX-WS working with jdk6. 1.6 contain old JAX-B/WS API versions (2.1) and this tricky code allows to load newer (2.2) javax.xml.* classes from jars on classpath (without having to use java endorsed mechanism) when running standalone JAX-WS on the top of JDK6. Hope it is not too confusing. Once we are further along with modules in the JDK 9 then I expect that -Xbootclasspath/p will go away (to be replaced by an alternative facility to override the classes in modules that are linked into the runtime image). So is this wsgen and wsimport code only used with JDK 6? Maybe that is okay but at the same time I would hope these APIs could leave JDK 6 behind some day, otherwise it makes it impossible to use language features and APIs.
Anyway I found some more code to cleanup - one more class can be removed - see version 2: http://cr.openjdk.java.net/~mkos/8054548/jaxws.01/
The update looks okay to me. I guess we'll have to see if other changes are required as we move forward. Also thanks for the explanation about JavacompilerMessages.java being auto-generated, there isn't anything in the header that makes this obvious (maybe the plugin that you mentioned could be updated to generate something into the header?). -Alan.
On 12/09/14 11:51, Alan Bateman wrote:
On 12/09/2014 09:39, Miroslav Kos wrote:
I agree that the code looks scary, but it's because we have to support range of jdk (currently jdk6 to jdk9) and we want to avoid maintaining several branches. The standalone project must work even with jdk6. The magic here is to make the latest JAX-WS working with jdk6. 1.6 contain old JAX-B/WS API versions (2.1) and this tricky code allows to load newer (2.2) javax.xml.* classes from jars on classpath (without having to use java endorsed mechanism) when running standalone JAX-WS on the top of JDK6. Hope it is not too confusing. Once we are further along with modules in the JDK 9 then I expect that -Xbootclasspath/p will go away (to be replaced by an alternative facility to override the classes in modules that are linked into the runtime image). So is this wsgen and wsimport code only used with JDK 6? Maybe that is okay but at the same time I would hope these APIs could leave JDK 6 behind some day, otherwise it makes it impossible to use language features and APIs. This code is currently used for all the jdk versions, but works well with all of them (including modularized build) - it should create own modified class loader only when necessary. After dropping jdk6 support we would simplify the code. Unfortunately we can't simply use the newest language features, we may start using them with some delay (several steps behind the newest JDK) not to break the older (yet) supported jdks.
Anyway I found some more code to cleanup - one more class can be removed - see version 2: http://cr.openjdk.java.net/~mkos/8054548/jaxws.01/
The update looks okay to me. I guess we'll have to see if other changes are required as we move forward. Also thanks for the explanation about JavacompilerMessages.java being auto-generated, there isn't anything in the header that makes this obvious (maybe the plugin that you mentioned could be updated to generate something into the header?). Yes, you're right, some javadoc saying that would be helpful. I'll keep it in mind and will update the plugin. If you are ok with it, I would proceed with this change as it is for now and improved generated classes would go into jdk9 with next regular sync integration.
Thanks Miran
-Alan.
On 12/09/2014 11:51, Miroslav Kos wrote:
If you are ok with it, I would proceed with this change as it is for now and improved generated classes would go into jdk9 with next regular sync integration.
Yes, I'm okay with this. I do expect that additional changes will be needed later but we'll cross that bridge when we get to it. -Alan
participants (2)
-
Alan Bateman
-
Miroslav Kos