Enhanced wrapper for Scripting API #1011

JaroslavTulach · 2018-11-06T15:23:01Z

Introducing org.netbeans.api.scripting API to wrap access to javax.script.ScriptEngineManager and provide necessary extensions to incorporate the Truffle language registrations into the system and expose them through the javax.script interfaces. That way the NetBeans modules can use API that works on any JDK and still access functionality that is provided by GraalVM.

JaroslavTulach · 2018-11-27T14:39:29Z

Ready for review, I'd say. Please take a look. Thanks. CCing @tzezula, @thurka, @timfel, @sdedic

junichi11 · 2018-12-04T00:47:46Z

It seems that there are conflicts.

platform/api.scripting/src/org/netbeans/modules/scripting/WriterOutputStream.java

sdedic · 2018-12-04T15:56:27Z

platform/api.scripting/nbproject/project.xml

+            <code-name-base>org.netbeans.api.scripting</code-name-base>
+            <module-dependencies>
+                <dependency>
+                    <code-name-base>org.netbeans.modules.nbjunit</code-name-base>


test-dependency

sdedic · 2018-12-04T15:56:27Z

nbbuild/travis/scripting.sh

+# under the License.
+
+BASE=graalvm-ce-1.0.0-rc9
+URL=https://github.com/oracle/graal/releases/download/vm-1.0.0-rc9/$BASE-linux-amd64.tar.gz


This is rather ugly platform (amd64) dependency for the CI build.

The CI build runs on 64bit Intel machines. What other download you'd use?

sdedic · 2018-12-04T15:56:27Z

platform/api.scripting/apichanges.xml

+<apichanges>
+    <apidefs>
+        <apidef name="scripting_api">Scripting API</apidef>
+        <apidef name="scripting_spi">Scripting API</apidef>


Scripting SPI

sdedic · 2018-12-04T15:56:27Z

platform/api.scripting/external/binaries-list

+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+07B63F4902CA51B574EA892D5D5E8088B8FBF97A org.graalvm:graal-sdk:1.0.0-rc6


Wouldn't it be better to upgrade to SDK RC9, given that Travis actually downloads RC9 runtime ?

I've created libs.graalsdk, libs.truffleapi modules and used the latest RC10. I have modified debugger.jpda.truffle to use these modules, @entlicher please verify it is OK.

sdedic · 2018-12-04T15:56:27Z

platform/api.scripting/src/org/netbeans/api/scripting/Bundle.properties

+# specific language governing permissions and limitations
+# under the License.
+
+OpenIDE-Module-Name=Scripting API Wrapper


There should be probably more entries if the module is AutoUpdate-Show-In-Client: true so it's correctly listed in updates.

Probably better to hide the module: b5e58fb

sdedic · 2018-12-04T15:56:27Z

platform/api.scripting/src/org/netbeans/modules/scripting/GraalEngine.java

+
+    @Override
+    public Object invokeMethod(Object thiz, String name, Object... args) throws ScriptException, NoSuchMethodException {
+        final Value thisValue = (Value) thiz;


throw ScriptException rather than CCE if thiz is not a Value.

Good idea: 2ecb7b9

sdedic · 2018-12-04T15:56:27Z

platform/api.scripting/src/org/netbeans/modules/scripting/GraalEngine.java

+    public Object invokeMethod(Object thiz, String name, Object... args) throws ScriptException, NoSuchMethodException {
+        final Value thisValue = (Value) thiz;
+        Value fn = thisValue.getMember(name);
+        Value result = fn.execute(args);


consider throwing NoSuchMethodException if fn is !canExecute

Yes, that would be more consistent. Implemented in b49167a

sdedic · 2018-12-04T15:56:27Z

platform/api.scripting/src/org/netbeans/modules/scripting/GraalEngine.java

+    @Override
+    public <T> T getInterface(Class<T> clasz) {
+        try {
+            return getInterface(evalImpl(getContext(), "this"), clasz);


Is this a token valid for all Graal-provided languages ?

No, not at all. There is however no generic way to obtain the global object as such the current version is using getPolyglotBindings() - e.g. only exported values are visible.

sdedic · 2018-12-04T15:56:27Z

platform/api.scripting/src/org/netbeans/modules/scripting/GraalEngine.java

+            return null;
+        }
+        if (result.isNumber()) {
+            return result.as(Number.class);


Isn't further classification using fitsInByte etc needed ?

Is it? I don't know. Any example you have in mind?

sdedic · 2018-12-04T15:56:27Z

platform/api.scripting/src/org/netbeans/modules/scripting/GraalEngineFactory.java

+
+    @Override
+    public String getMethodCallSyntax(String arg0, String arg1, String... arg2) {
+        return null;


This would be a nice request/PR for Truffle :) and Polyglot API in Graal project. Who else should know about the language syntax ?

What exactly do you need here? We're in the process (oracle/graal#764) of adding API facilities for getting "completion characters" (. in many languages, sometimes ->) and "signature characters" (( in many languages) to get autocomplete and signature completion working. Maybe it would make sense to consider this use case in the PR as well, but we'd need to know what kind of "syntax" you need to create here

Something which would allow to satisfy contract of ScriptEngineFactory in a lang-independent way so it can be bridged to standard Scripting APIs here, like the Polyglot Context eval* methods are.

Also note the other get* code construction helper methods in the scripting API.

emilianbold

I'm a bit puzzled by this commit. ScriptEngineManager is supposed to be dynamic enough but apparently we still need to wrap it at Platform level for actual modules. It would be nice if the JDK would be fixed so all these classes that rely on the service provider mechanism can actually run properly in a dynamic module system.

I also don't understand why we need the Graal dependency or the Graal service factory. These should be imho in a separate graal.scripting module which I'm not even certain needs to be part of platform so soon.

emilianbold · 2018-12-05T11:39:25Z

enterprise/web.jsf/src/org/netbeans/modules/web/jsf/palette/JSFPaletteUtilities.java

@@ -242,7 +240,7 @@ protected static ScriptEngine getScriptEngine(FileObject fo) {
                    } catch (ClassNotFoundException ex) {
                        Exceptions.printStackTrace(ex);
                    }
-                    manager = new ScriptEngineManager(loader != null ? loader : Thread.currentThread().getContextClassLoader());


Why is loader ignored here?

The classloader logic is handled centrally in Scripting.createManager().

emilianbold · 2018-12-05T11:39:25Z

java/java.source.base/src/org/netbeans/api/java/source/GeneratorUtilities.java

@@ -2212,8 +2209,7 @@ private static ScriptEngine engine(FileObject fo) {
        if (obj instanceof String) {
            synchronized (GeneratorUtilities.class) {
                if (manager == null) {
-                    ClassLoader loader = Lookup.getDefault().lookup(ClassLoader.class);
-                    manager = new ScriptEngineManager(loader != null ? loader : Thread.currentThread().getContextClassLoader());
+                    manager = Scripting.createManager();


Also, why is the loader logic ignored now?

The classloader logic is handled centrally in Scripting.createManager().

emilianbold · 2018-12-05T11:39:25Z

nbbuild/external/binaries-list

@@ -17,3 +17,4 @@
 A7674A6D78B7FEA58AF76B357DAE6EA5E3FDFBE9 apitest-70.jar
 16398550402B27F81CD0D508CEF54B3E47A4A6DA org.apache.rat:apache-rat:0.12
 AB396EE119BFAD809CC9F09950CC22E2BCE2FE35 langtools-9.zip
+E32B3483FBF362C92088CB79E9F1F161F3F64A21 org.apidesign.javadoc:codesnippet-doclet:0.30


-1

According to https://search.maven.org/artifact/org.apidesign.javadoc/codesnippet-doclet/0.31/jar this is GPL 3:

Codesnippet Javadoc Doclet
Copyright (C) 2015-2018 Jaroslav Tulach - jaroslav.tulach@apidesign.org

This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, version 3.0 of the License.

It is a build time only dependency. The codesnippet is used when generating Javadoc.

emilianbold · 2018-12-05T11:39:25Z

nbbuild/external/codesnippet-doclet-0.30-license.txt

+Source: https://github.com/jtulach/codesnippet4javadoc/
+Description: Javadoc doclet to include real code snippets in the documentation
+Origin: apidesign.org
+Type: compile-time


Oh, so we are supposed to accept it since it's only compile time?

Right. It is compile time and no Apache code is linked to that library. No Apache code is derived work of that library. Also, in the readme of the codesnippet4javadoc I explicitly allow anyone to use the tool to generate Javadoc for any code of any license.

emilianbold · 2018-12-05T11:39:25Z

platform/api.scripting/external/graal-sdk-1.0.0-rc6-license.txt

+
+Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+
+The Universal Permissive License (UPL), Version 1.0


According to https://search.maven.org/artifact/org.graalvm/graal-sdk/1.0.0-rc6/jar I see a different license:

<name>GNU General Public License, version 2, with the Classpath Exception</name> <url>http://openjdk.java.net/legal/gplv2+ce.html</url>

Fixed in version RC10: https://search.maven.org/artifact/org.graalvm.sdk/graal-sdk/1.0.0-rc10/jar

emilianbold · 2018-12-05T11:39:25Z

platform/api.scripting/src/META-INF/upgrade/javax.script.ScriptEngineManager.hint

+ * under the License.
+ */
+'Use Scripting.createManager()':
+   new javax.script.ScriptEngineManager($$anything$$)


So the hint also ignores any class loader?

Right. The new code is going to use the NetBeans classloader that sees all the registrations.

emilianbold · 2018-12-05T11:39:25Z

platform/api.scripting/src/org/netbeans/api/scripting/Scripting.java

+        for (EngineProvider p : Lookup.getDefault().lookupAll(EngineProvider.class)) {
+            extra.addAll(p.factories());
+        }
+        ClassLoader l = Lookup.getDefault().lookup(ClassLoader.class);


So Scripting has a similar logic wrt ClassLoader. OK...

Right. The Scripting class is NetBeans aware and as such it can handle the classloading logic properly at a single place.

emilianbold · 2018-12-05T11:39:25Z

platform/api.scripting/src/org/netbeans/modules/scripting/GraalEnginesProvider.java

+import org.netbeans.spi.scripting.EngineProvider;
+import org.openide.util.lookup.ServiceProvider;
+
+@ServiceProvider(service = EngineProvider.class)


Shouldn't the Graal Engine Provider be in a separate module?

It used to be. It got merged in 818830a after in office discussion with Sváťa and Tomáš Zezula.

The needed org.graalvm.polyglot types are provided on bootclasspath when running on the GraalVM. Having them in the same module is similar and seemed OK as no additional runtime cost/weight of a module is paid.

I find this wrong somehow. So you manage to compile those classes importing org.graalvm.* because you have a compile-time dependency on the Graal SDK but then I assume you rely that those classes will never be loaded by a module system classloader unless on the Graal VM?

Having compiled code that cannot be loaded at runtime feels like a broken dependency to me...

Right. If we ever want to bundle Graal.js and use it on regular (e.g. non-GraalVM) JVM then this setup would be wrong. So far I was only testing on either regular JVM (the engine missing and disabled) or GraalVM (the API classes and the engine present).

I still find this an abuse of the module system. Why compile the code is this weird way, add a compile time dependency on Graal and have .class files that are un-loadable when we could have a separate module for people to load when Graal is there?

Maybe have a token or something for GraalVM (something for OpenIDE-Module-Java-Dependencies perhaps) ?

Hello @emilianbold (and also @jlahoda): per your request I moved the GraalVM specific part (including the documentation) into libs.graalsdk module. Now I expect your approvals. Thank you.

entlicher · 2019-01-16T17:47:50Z

Concerning debugger it looks fine to me.
truffle-sl is used in tests only. If it builds fine, it does not matter that it comes from rc6. It might require update in the future if truffle-api removes possible deprecations used in SL, if any.

JaroslavTulach · 2019-01-17T09:49:20Z

I think all the comments have been addressed. I have an approval (more is welcomed). I'll squash and integrate the change soon.

JaroslavTulach · 2019-01-18T11:37:28Z

platform/core.network/test/unit/src/org/netbeans/core/network/utils/FakeDns.java

@@ -172,6 +176,22 @@ public void delayAnsweringBy(long msWait ) {
        return null;
    }

+    @Override
+    Callable<InetAddress[]> createDnsTimeoutTask(String host) {


@lbruun, please take a look. Hopefully I changed no semantics, but now the test passes on JDK11. Instead of injecting something into sun.*.net.* packages, I inject different implementation of createDnsTimeoutTask in the test.

JaroslavTulach · 2019-01-18T11:39:13Z

platform/core.network/src/org/netbeans/core/network/proxy/pac/impl/NbPacScriptEvaluator.java

-        // - since Java 8 - it will be a Nashorn engine.
-        ScriptEngineManager factory = new ScriptEngineManager();
-        return factory.getEngineByName("JavaScript");
+        ScriptEngineManager manager = Scripting.createManager();


Using engine from libs.graaljs module, if it is available. Otherwise falling back to default JavaScript engine. No special treatment needed, as all Java objects are wrapped into appropriate JavaScript function closures. CCing @lbruun

I am really not sure about the removal of special-casing for Nashorn. I think I have a counter example (for Nashorn at least). Please let me know which venue is appropriate to discuss that.

matthiasblaesing · 2019-01-18T20:27:09Z

Please try to break this PR into reviewable parts. There are now 58 commits and 114 changed files Especially the the changes to the proxy selector infrastructure should be separated, as that is security critical.

jtulach · 2019-01-19T05:53:28Z

I'll squash the changes on Monday and I leave out changes to platform/core.network out of the merge per @jlahoda and @matthiasblaesing request.

…ython, Ruby, etc. script engines. Using that API to integrate Graal.js as the default JavaScript engine for the IDE.

JaroslavTulach added the work-in-progress label Nov 6, 2018

JaroslavTulach requested a review from sdedic Nov 20, 2018

JaroslavTulach force-pushed the ScriptingAPI branch from e7a5d36 to c19b673 Compare Nov 21, 2018

JaroslavTulach added enhancement API Change JavaScript and removed work-in-progress labels Nov 27, 2018

JaroslavTulach self-assigned this Nov 27, 2018

junichi11 reviewed Dec 4, 2018

View changes

platform/api.scripting/src/org/netbeans/modules/scripting/WriterOutputStream.java Outdated Show resolved Hide resolved

sdedic reviewed Dec 4, 2018

View changes

emilianbold reviewed Dec 5, 2018

View changes

JaroslavTulach force-pushed the ScriptingAPI branch from c29b76d to bbb2616 Compare Jan 14, 2019

entlicher approved these changes Jan 16, 2019

View changes

JaroslavTulach force-pushed the ScriptingAPI branch from 95f2625 to de83617 Compare Jan 17, 2019

JaroslavTulach reviewed Jan 18, 2019

View changes

JaroslavTulach force-pushed the ScriptingAPI branch from 56efa62 to ab514f7 Compare Jan 18, 2019

JaroslavTulach force-pushed the ScriptingAPI branch 2 times, most recently from c4fe20a to 77968e6 Compare Jan 21, 2019

Introducing Scripting API to simplify access to various JavaScript, P…

fc2d62b

…ython, Ruby, etc. script engines. Using that API to integrate Graal.js as the default JavaScript engine for the IDE.

JaroslavTulach force-pushed the ScriptingAPI branch from 6103311 to fc2d62b Compare Jan 21, 2019

Merge branch 'master' into ScriptingAPI

0cc960c

JaroslavTulach merged commit 483e245 into apache:master Jan 21, 2019
1 check passed

JaroslavTulach mentioned this pull request Jan 21, 2019

Using Graal.js 19.0.0 via Scripting in platform/core.network #1092

Merged


		Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.

		The Universal Permissive License (UPL), Version 1.0

Enhanced wrapper for Scripting API #1011

Enhanced wrapper for Scripting API #1011

Conversation

JaroslavTulach commented Nov 6, 2018

JaroslavTulach commented Nov 27, 2018 • edited

junichi11 commented Dec 4, 2018

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

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

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

sdedic Dec 5, 2018 • edited

Choose a reason for hiding this comment

emilianbold left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

entlicher commented Jan 16, 2019

JaroslavTulach commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasblaesing commented Jan 18, 2019

jtulach commented Jan 19, 2019

JaroslavTulach commented Nov 27, 2018 •

edited

sdedic Dec 5, 2018 •

edited