Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NETBEANS-1159] Using UPL licensed graal-js-parser. #1407

Merged
merged 7 commits into from Aug 8, 2019

Conversation

@JaroslavTulach
Copy link
Contributor

JaroslavTulach commented Aug 6, 2019

I've took the first UPL version of the graal.js parser and modified it to work with current NetBeans JavaScript editing infrastructure. Then I created Graal.js JAR using folling steps:

$ git clone https://github.com/JaroslavTulach/graaljs.git
$ cd graaljs/
graaljs$ git checkout e20fe15
graaljs$ ant
graaljs$ ls -l graal-js/mxbuild/dists/graal-js-parser.jar

Since e20fe15 there is an Ant script to perform the build. I compiled with JDK8.

The graal-js-parser has been uploaded as F500B932961B227F74352474D23F905EC52F112E-graal-js-parser-e20fe15.jar to our binaries set and this pull request is using it.

@JaroslavTulach JaroslavTulach self-assigned this Aug 6, 2019
@sdedic

This comment has been minimized.

Copy link
Member

sdedic commented Aug 6, 2019

We probably do not need customizations in webcommon/libs.nashorn/build.xml either, since JS parser lib can be part of the regular build (?)

@junichi11

This comment has been minimized.

Copy link
Member

junichi11 commented Aug 6, 2019

@JaroslavTulach Maybe, php.editor also needs the fix?

nashorn.prepend=${basedir}/../../webcommon/libs.nashorn/external/com.oracle.js.parser-ba7a8bc42268.jar
bootclasspath.prepend=${nashorn.prepend}

@geertjanw

This comment has been minimized.

Copy link
Member

geertjanw commented Aug 6, 2019

This is great, no more need for the user to install the parser themselves.

@sdedic
sdedic approved these changes Aug 6, 2019
Copy link
Member

sdedic left a comment

Great !

@matthiasblaesing

This comment has been minimized.

Copy link
Contributor

matthiasblaesing commented Aug 6, 2019

Thank you - a build worked and starting with a clean userdir, all JS features were directly accessible (nice!). I had a first look at the sources and this is what I noticed:

Build from source fails

mx still feels alien to me and I failed to build graal.js from source:

matthias@athena:~/src/graaljs/graal-js$ mx build --project GRAAL_JS_PARSER
Traceback (most recent call last):
  File "/home/matthias/src/mx/mx.py", line 19726, in <module>
    main()
  File "/home/matthias/src/mx/mx.py", line 19613, in main
    primary = _discover_suites(primarySuiteMxDir, load=should_load_suites)
  File "/home/matthias/src/mx/mx.py", line 3364, in _discover_suites
    _register_visit(primary)
  File "/home/matthias/src/mx/mx.py", line 3362, in _register_visit
    s._load()
  File "/home/matthias/src/mx/mx.py", line 1506, in _load
    self._load_extensions()
  File "/home/matthias/src/mx/mx.py", line 1931, in _load_extensions
    mod = __import__(extensionsName)
  File "/home/matthias/src/graaljs/graal-js/mx.graal-js/mx_graal_js.py", line 338, in <module>
    build_args=['--language:js']
TypeError: __init__() takes at least 6 arguments (5 given)
matthias@athena:~/src/graaljs/graal-js$ 

Am I missing something?

License information

A quick check of the licenses (aka grep -lr "GNU General " *) gave me:

GPL license header:
graal-js/mx.graal-js/mx_graal_js.py
graal-js/mx.graal-js/mx_graal_js_benchmark.py
graal-js/src/com.oracle.js.parser/src/com/oracle/js/parser/package-info.java
graal-js/src/com.oracle.js.parser/src/com/oracle/js/parser/resources/Messages.properties

GPL license file:
graal-js/mx.graal-js/LICENSE

GPL+UPL header, without indication that the file is dual licensed:
graal-js/src/com.oracle.truffle.js.runtime/src/com/oracle/truffle/js/runtime/builtins/JSURLEncoder.java
graal-js/src/com.oracle.truffle.js.runtime/src/com/oracle/truffle/js/runtime/builtins/JSURLDecoder.java

GPL+UPL+BSD license:
graal-js/src/com.oracle.truffle.js.runtime.doubleconv/src/com/oracle/truffle/js/runtime/doubleconv/DiyFp.java
graal-js/src/com.oracle.truffle.js.runtime.doubleconv/src/com/oracle/truffle/js/runtime/doubleconv/DoubleConversion.java

There are other licenses on graal-nodejs, I hope, that no code outside graal-js is used for graal-js.

@JaroslavTulach

This comment has been minimized.

Copy link
Contributor Author

JaroslavTulach commented Aug 7, 2019

Thanks for the detailed review, @matthiasblaesing. The TypeError: __init__() takes at least 6 arguments (5 given) problem has something to do with old/new version of mx. I have seen it as well before (but not when writing the above steps). The workaround was:

graal-js$ git diff
diff --git a/graal-js/mx.graal-js/mx_graal_js.py b/graal-js/mx.graal-js/mx_graal_js.py
index bb59b035d7..ae60755801 100644
--- a/graal-js/mx.graal-js/mx_graal_js.py
+++ b/graal-js/mx.graal-js/mx_graal_js.py
@@ -331,12 +331,6 @@ mx_sdk.register_graalvm_component(mx_sdk.GraalVmLanguage(
         'graal-js:ICU4J-DIST'
     ],
     launcher_configs=[
-        mx_sdk.LanguageLauncherConfig(
-            destination='bin/<exe:js>',
-            jar_distributions=['graal-js:GRAALJS_LAUNCHER'],
-            main_class='com.oracle.truffle.js.shell.JSLauncher',
-            build_args=['--language:js']
-        )
     ],
     boot_jars=['graal-js:GRAALJS_SCRIPTENGINE']
 ))

Thanks for the license check. Yes, the graal-js/src/com.oracle.js.parser/src/com/oracle/js/parser/package-info.java and graal-js/src/com.oracle.js.parser/src/com/oracle/js/parser/resources/Messages.properties are an oversight. It has just been fixed in cdbd80a95. The other files are unrelated and I'd rather leave them without any modification. They aren't used in the parser anyway. When I run:

graaljs/graal-js$ mx clean
graaljs/graal-js$ mx -v build --project GRAAL_JS_PARSER
javac -g -d mxbuild/src/com.oracle.js.parser/bin -classpath  
-parameters -proc:none -target 1.8 -source 1.8 
@mxbuild/src/com.oracle.js.parser/javafilelist.txt 
-Xlint:none -XDignore.symbol.file -encoding UTF-8 -Xmaxerrs 10000

e.g. it should be enough to run just the javac and then package the source and resource files somehow:

$ /jdk1.8.0/bin/javac -g -d mxbuild/src/com.oracle.js.parser/bin 
-classpath  -parameters -proc:none -target 1.8 -source 1.8 
@mxbuild/src/com.oracle.js.parser/javafilelist.txt 
-Xlint:none -XDignore.symbol.file -encoding UTF-8 -Xmaxerrs 10000

The javafilelist.txt contains only files under the src/com.oracle.js.parser/src directory, so licenses of other files shall not matter.

Copy link
Contributor

matthiasblaesing left a comment

Thank you for the walk through to handling mx. The change to mx_graal_js.py indeed caused the build to succeed. I build from source and ran the resulting jar and the jar included into the netbeans build through diffoscope (https://salsa.debian.org/reproducible-builds/diffoscope). The jars are identical (excluding ordering/time differenzes in the jar directory).

I agree, that package-info.java and Message.properties are the files, that really need the license update (even if we would bundle the source for the parser, we could replace mx with a trival ant script, as you already demonstrated).

I'd like to ask you to rebuild the binaries from your new tag and switch to that binary. We learned, that some folks get grumpy if they see the GPL and the parser binary caries it in the Messages.properties.

Apart from that I think this is a great improvement.

@JaroslavTulach JaroslavTulach merged commit bba551d into apache:master Aug 8, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JaroslavTulach JaroslavTulach deleted the JaroslavTulach:jtulach/UplParser branch Aug 8, 2019
@junichi11 junichi11 added this to the 11.2 milestone Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.