Bug 58271

Are you experiencing any problem with the current approach?

(In reply to Stefan Bodewig from comment #1)
> Are you experiencing any problem with the current approach?

Hi, I am working on jdk9. The behaviour of ClassLoader.getResource is expected to change in the modular ("jigsaw") jdk9 (http://openjdk.java.net/projects/jigsaw/). Retrieving .class resource URL from a named module will not work as expected.

Given that there is cleaner alternative - namely reflection API, it is better to use java reflection to check javax.script.ScriptEngineManager exists or not in the JDK configured. Is there any particular reason why this ClassLoader.getResource is preferred? If so, please let me know.

TBH I don't remember why the code was written the way it is and haven't been involved with writing it at all - just wanted to avoid fixing code for cosmetic reasons that wasn't broken.

Looking at the code my guess is the special case for BSF that immediately before the Class.forName call is required for it to work when using BSF and the code needs a way to ensure BSF is actually there before modifying the classloader hierarchy.

The problem we should really talk about is LoaderUtils.classNameToResource which is also used inside the JUnit task.  We use it to tell people we've found multiple versions of JUnit.  Not only that, we also tell them where we've found them.  How would we achieve the same with using reflection alone?

The LoaderUtils.classExists that uses LoaderUtils.classNameToResource is only used inside the BSF classloader hack so shouldn't be a problem if we manage to fix the hack.

(In reply to Stefan Bodewig from comment #3)
> TBH I don't remember why the code was written the way it is and haven't been
> involved with writing it at all - just wanted to avoid fixing code for
> cosmetic reasons that wasn't broken.
> 
> Looking at the code my guess is the special case for BSF that immediately
> before the Class.forName call is required for it to work when using BSF and
> the code needs a way to ensure BSF is actually there before modifying the
> classloader hierarchy.
> 
> The problem we should really talk about is LoaderUtils.classNameToResource
> which is also used inside the JUnit task.  We use it to tell people we've
> found multiple versions of JUnit.  Not only that, we also tell them where
> we've found them.  How would we achieve the same with using reflection alone?
> 
> The LoaderUtils.classExists that uses LoaderUtils.classNameToResource is
> only used inside the BSF classloader hack so shouldn't be a problem if we
> manage to fix the hack.

ClassLoader.getResource behaviour change is *only* about classes in named modules (like javax.script.ScriptEngineManager and other JDK platform classes which will be in named modules). The use ClassLoader.getResource for other classes like junit or anything that is deployed via classpath or located via some custom class loader - is not be a problem. That should work fine as it does in the current JDK versions. So, if the use of ClassLoader.getResource for platform classes can be avoided, ant tasks would be fine.

IIUC we could modify ScriptRunnerCreator so that it still uses ClassLoader.getResource if the selected engine is BSF but uses Class.forName for the javax.script engine, right?

This should address the changes Java9 and allow us to fiddle with classloaders before loading BSF at the same time.

(In reply to Stefan Bodewig from comment #5)
> IIUC we could modify ScriptRunnerCreator so that it still uses
> ClassLoader.getResource if the selected engine is BSF but uses Class.forName
> for the javax.script engine, right?
> 
> This should address the changes Java9 and allow us to fiddle with
> classloaders before loading BSF at the same time.

Yes. That should work.

should be fixed with git commit 4ccca1f

Actually this was not a bug in Ant but in Java.

At least I would interpret it in that way that this error happens with Java 9 build 114, but works fine with the latest released build 156 which is the case.

So if you liked the old code better you now have the choice to change it back. Excpet of course if the Java bug is, that it works again in the latest build. :-)

Thanks Björn

back then it seemed as if the change would be required for Java9 as the way Java wrked was about to change. Now it looks the Java0 team has changed their minds again. I'm not convinced build 156 is the final call :-)

Apart from that it probably is good the old code works again as it is still used for JUnit. Aftre re-reading this thread it would have failed for JUnit as well under 9b114 if junit-jar was a named module. Well, in theory the junit devs could decide to ship junit as a Java0 module in the future.

For Ant 1.10.x I'd be willing to rewrite the whole access of javax.script to not use reflection at all as we are guaranteed to find it - Ant 1.10 requires Java8 at runtime.