Experiment standalone nashorn #111

obourgain · 2021-02-22T11:32:43Z

Oups, that was meant to be a draft for discussion about #109, please treat it as the basis for discussion and not a serious PR.

What I think needs more effort:

cleanup the code
as standalone nashorn builds on JDK 11, what should we do? Some suggestions:
1- build on 11 targeting 8
2- raise the minimal version to 11 (probably not the best solution)
3- split in another module so the core builds on 8 and the module for standalone nashorn builds on 11
find a test strategy to test on at least JDK 8, 11 and 15, maybe with a bunch of modules targeting each of these versions?

mxro · 2021-02-25T21:24:18Z

src/main/java/delight/nashornsandbox/internal/JsSanitizer.java

+		try {
+			tmp_JDK_NASHORN_ScriptObjectMirror_CLASS = Class.forName("jdk.nashorn.api.scripting.ScriptObjectMirror");
+		} catch (ClassNotFoundException e) {
+			System.out.println("JDK Nashorn not found");


I assume in the final version we wouldn't want to log out here?

Hello, like I wrote in the description, this is a basis for discussion and must not be merged as is.
I think there should be somewhere a log statement indicating which version of Nashorn is used and why, but that should probably be at debug level.

mxro · 2021-02-25T21:54:25Z

src/main/java/delight/nashornsandbox/internal/NashornSandboxImpl.java

+		STANDALONE_NASHORN_NashornScriptEngineFactory_CLASS = tmp_STANDALONE_NASHORN_NashornScriptEngineFactory_CLASS;
+		STANDALONE_NASHORN_ClassFilter_CLASS = tmp_STANDALONE_NASHORN_ClassFilter_CLASS;
+	}
+


Probably better to manage this in a centralised place?

Most certainly, that should move to a class like NashornDetection, that would do the detection and debug logs about what is going on.

mxro · 2021-02-25T21:57:20Z

I think key would be to retain compatibility with 1.8. I think we can run it against different versions as part of the travis build: https://github.com/javadelight/delight-nashorn-sandbox/blob/master/.travis.yml

Also we need to be careful to try to make it as simple as possible to understand, since there is a bit of complexity involved here.

obourgain · 2021-02-25T23:05:24Z

I agree that we must not break existing users.
We cannot easily run the build against several versions of JDK as with the dependency on standalone-nashorn, JDK 8 would fail to build due to unsupported bytecode version in this jar, so we would need to build with JDK 11 or later and run tests on JDK 8.
I am not sure yet how to support this with Travis.

Will need to add back a run of the tests on JDK 8 too.

mxro · 2021-02-26T06:55:36Z

src/main/java/delight/nashornsandbox/internal/NashornDetection.java

+            STANDALONE_NASHORN_NashornScriptEngineFactory_CLASS = null;
+            STANDALONE_NASHORN_ClassFilter_CLASS = null;
+        }
+        // TODO add a report of what was detected and what will be used


Yep I think so, as you said maybe a debug level log?

yes, I had a sleepless night so I worked on this a bit, but it's not finished yet

One step at a time, and take care!

mxro · 2021-02-26T06:56:37Z

src/main/java/delight/nashornsandbox/internal/NashornDetection.java

+    }
+
+    public static SandboxClassFilter createSandboxClassFilter() {
+        // TODO allow to force one impl?


hmm interesting question - probably that would be a property on the Sandbox?

I was thinking of 2 properties:

prefer JDK or standalone nashorn, in case both are present

force the use of one of those, throw an exception if not available

Also, and maybe that starts to be overkill, we could do:

a property on the sandbox to have fine grained control, that could be useful for people that want to do it in tests

a System property, so users can set the properties on the JVM with something like -Ddelight-nashorn-sandbox.nashorn.provider=standalone
The sandbox property would have precedence, fallback to the System property, then fallback to the auto detection.

Actually I am thinking given the kind of low level nature of this, a system property maybe sufficient?

So default to standalone if present, unless there is a system property to overwrite it?

mxro · 2021-02-26T07:00:32Z

I like it, great work! So now all the tests would run against 11 and the non-JDK Nashorn version, correct? But since we still have the target 1.8 it should compile down to that.

Only thing missing would be then to run the tests against the JDK Nashorn in 1.8 as well. But not sure if we can do that. But maybe we also don't necessarily need to. I guess there is an argument to be made that we would want to run the test only against the most recent Nashorn version?

obourgain · 2021-02-26T10:03:42Z

Yes, running with JDK 11 with target 1.8 will still produce bytecode that will run on JDK 8.

I'll try to have a build testing several combinations:

jdk 8
jdk 11 with jdk nashorn
jdk 11 with standalone nashorn
jdk 15 with standalone nashorn

As Travis-ci is a bit out of favor these days, would it be a possibility to migrate to Github Actions?

mxro · 2021-02-26T18:00:23Z

I don't mind migrating to GitHub actions at all - will also have to find a new way to push this to Maven central since Bintray is shutting down 😢

souloffire · 2021-03-12T06:48:39Z

Hello. I was wondering when is it planned to merge these changes into the master branch?

Due to the use of both JDK (removed from 15) and Standalone Nashorn (compiled to 11), we must build on JDK 11 with source and target 1.8. Then we can run the tests on JDK 8 and 15 with maven verify. We must not clean or change the sources in between the build on JDK 11 and the tests on other JDKs otherwise, Maven will rebuild the module and will fail.

obourgain · 2021-03-12T08:52:08Z

Hello, I just pushed a version that seems to work on JDK 8, 11 and 15.
I didn't update the CI yet, and as it uses oraclejdk8 it will fail to compile.
You can build it and run test on both JDKs locally. You'll need to change the JAVA_HOME env var, with a command like
export JAVA_HOME=/usr/lib/jvm/zulu11 (I use Azul Zulu, but adapt this command to your own JDK)
The steps are:

build and test on JDK11 with mvn verify
test on JDK8 with mvn verify
test on JDK15 with mvn verify
You must not clean or change the Java sources in between, otherwise maven will recompile the module.
The build can only succeed on JDK 11 because it uses classes from JDK Nashorn and Standalone Nashorn.

Experiment standalone nashorn

e1ae8a2

obourgain mentioned this pull request Feb 22, 2021

Interrest in a version based on standalone Nashorn? #109

Closed

mxro reviewed Feb 25, 2021

View reviewed changes

obourgain added 2 commits February 26, 2021 01:36

Move the code to detect which Nashorn to use to a dedicated class

234ef1b

Run CI on JDK 11

5ec6dab

Will need to add back a run of the tests on JDK 8 too.

mxro reviewed Feb 26, 2021

View reviewed changes

Add detection report

4686d82

mxro merged commit 95d6d48 into javadelight:master Mar 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment standalone nashorn #111

Experiment standalone nashorn #111

obourgain commented Feb 22, 2021 •

edited

Loading

mxro Feb 25, 2021

obourgain Feb 25, 2021

mxro Feb 25, 2021

obourgain Feb 25, 2021

mxro commented Feb 25, 2021

obourgain commented Feb 25, 2021 •

edited

Loading

mxro Feb 26, 2021

obourgain Feb 26, 2021

mxro Feb 26, 2021

mxro Feb 26, 2021

obourgain Feb 26, 2021

mxro Feb 26, 2021

mxro commented Feb 26, 2021

obourgain commented Feb 26, 2021

mxro commented Feb 26, 2021

souloffire commented Mar 12, 2021

obourgain commented Mar 12, 2021

Experiment standalone nashorn #111

Experiment standalone nashorn #111

Conversation

obourgain commented Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxro commented Feb 25, 2021

obourgain commented Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxro commented Feb 26, 2021

obourgain commented Feb 26, 2021

mxro commented Feb 26, 2021

souloffire commented Mar 12, 2021

obourgain commented Mar 12, 2021

obourgain commented Feb 22, 2021 •

edited

Loading

obourgain commented Feb 25, 2021 •

edited

Loading