8258216: Allow Nashorn to operate when not loaded as a JPMS module

[szegedi]

Allow Nashorn to operate when not loaded as a JPMS module

---

Progress

• Change must not contain extraneous whitespace
• Change must be properly reviewed

Issue

• JDK-8258216: Allow Nashorn to operate when not loaded as a JPMS module

Download

$ git fetch https://git.openjdk.java.net/nashorn pull/9/head:pull/9
$ git checkout pull/9

[bridgekeeper]

👋 Welcome back attila! A progress list of the required criteria for merging this PR into main will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[sundararajana]

Have was non-modular nashorn tested? Don't the tests assume that nashorn is run as module?

[szegedi]

Well, I did some light testing on the command line myself. I have this minimal application:

import javax.script.*;

public class X {
    public static void main(String[] args) throws ScriptException {
        ScriptEngineManager factory = new ScriptEngineManager(); 
        ScriptEngine engine = factory.getEngineByName("nashorn"); 
        engine.eval("print('Hello, World!');");
    }
}

and running it from Nashorn directory (after JAR is built) with:

java -classpath build/nashorn/dist/nashorn.jar:build/nashorn/dependencies/asm-7.3.1.jar:build/nashorn/dependencies/asm-util-7.3.1.jar X.java

Before the changes it obviously causes:

Exception in thread "main" java.lang.ExceptionInInitializerError
	at org.openjdk.nashorn.api.scripting.NashornScriptEngine$1.run(NashornScriptEngine.java:128)
	at org.openjdk.nashorn.api.scripting.NashornScriptEngine$1.run(NashornScriptEngine.java:124)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:391)
	at org.openjdk.nashorn.api.scripting.NashornScriptEngine.<init>(NashornScriptEngine.java:124)
	at org.openjdk.nashorn.api.scripting.NashornScriptEngineFactory.getScriptEngine(NashornScriptEngineFactory.java:150)
	at java.scripting/javax.script.ScriptEngineManager.getEngineByName(ScriptEngineManager.java:241)
	at X.main(X.java:6)
Caused by: java.lang.IllegalArgumentException: Null module name
	at java.base/jdk.internal.module.Checks.requireModuleName(Checks.java:46)
	...

while running it after the changes outputs

Hello, World!

as expected.

You're right that ant test target runs all the tests with Nashorn loaded as a module; that's still the preferred way. I'm doing this to foster adoption as there's a report that Nashorn doesn't work when Spring Boot based apps load it (as SB uses classpath to load app depedencies) and the referenced JBS issue was also filed by a person who seems to be on the JAX-RS expert group and is an Eclipse committer, so I suspect there might be something there too (not confirmed.) I might try to modify the test script locally for a one-time run; I'd be reluctant to add module/non-module to the test matrix.

[efge]

Hi @efge, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user efge for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

• I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

[szegedi]

Have was non-modular nashorn tested? Don't the tests assume that nashorn is run as module?

@sundararajana I created a test-demodulator branch in my own fork of the repo building on top of this branch that's running tests with Nashorn on classpath instead of on module path.
You'll notice I moved two tests into "currently failing" in that branch as they're directly testing the module system. All other tests pass. Note that that branch is very hacky and is only meant to demonstrate the correctness of this code one time, it will not be submitted as a PR. As I said, I'd rather not add non-modular execution to the test matrix in general, at least not for now.

[sundararajana]

Looks good