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

make rhino-engine an osgi bundle #1278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stbischof
Copy link

@stbischof stbischof commented Nov 9, 2022

solves:#1277

@p-bakker
Copy link
Collaborator

LGTM, but no osgi expert by far

It seems to bring engineJar more in line with jar and runtimeJar, but I wonder why runtimeJar would not also have "Automatic-Module-Name": "org.mozilla.rhino-runtime", similar to engineJar and jar

@carlosame any thoughts, since you're the one to last make changes in this area (in #873)?

"Bundle-SymbolicName": "org.mozilla.rhino.engine",
"Bundle-Version": project.version.replaceAll("-.*", ""),
"Export-Package": "org.mozilla.javascript.engine",
"Import-Package": "javax.script,org.mozilla.javascript"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not declare an Import-Package, and instead leave the default (automatic) behaviour.

Copy link
Author

@stbischof stbischof Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an OSGi import.(has nothing to do with the Automatic-Module-Name) If you do not declare it the bundle will not work/ throw an ClassNotFoundExcaption because bundle does not declare that is needs it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird. I do not use OSGi myself, however from https://bnd.bndtools.org/heads/import_package.html:

The Import-Package header lists the packages that are required by the contained packages. The default for this header is *, resulting in importing all referred packages. This header therefore rarely has to be specified.

Copy link
Author

@stbischof stbischof Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. bnd is a tool that calculates die imports. if you say to bnd do * then it will habe as result in the manifest:

Import-Package javax.script,org.mozilla.javascript

but since yopu do not use a tool loke bnd you have to write it by yourown

@juergen-albert (OSGi Working Group Steering Committee)

could you please review this from the OSGi point of view?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What @stbischof wrote is correct. bnd is a build tool, that can calculate the Imports for you with proper Version ranges. The same goes for the Export Packages. OSGi requires all the packages to be named separately and can not interpret the *.

bnd even has a dedicated gradle plugin that can automatically calculate a Manifest for your, not only putting in proper OSGi Headers, but can also automatically handle things in respect to Java Module System.

I'm not that firm with gradle, but you can find an example of the gradle plugin usage here: https://github.com/osgi/osgi-test/tree/main/examples/osgi-test-example-gradle

@carlosame
Copy link
Contributor

It seems to bring engineJar more in line with jar and runtimeJar, but I wonder why runtimeJar would not also have "Automatic-Module-Name": "org.mozilla.rhino-runtime", similar to engineJar and jar

@carlosame any thoughts, since you're the one to last make changes in this area (in #873)?

I commented on this in a few issues, like #1092. Essentially, rhino-runtime is modular-incompatible with the other jars, so it would be pointless to put a module name and thus invite people to use it in modular projects.

Quoting from one of my comments in the aforementioned issue (#1092 (comment)):

"Split packages" problem with rhino.jar and rhino-runtime.jar. That's the reason why I did not add a module name to rhino-runtime.jar. The problem hasn't been observed out there because almost nobody uses rhino-runtime.jar, but if this project keeps telling people that "you should use rhino-runtime.jar" they will be hit (impact: unable to start the JVM). It depends on the current issue to be fixed.

@stbischof
Copy link
Author

stbischof commented Nov 15, 2022

Hi we also should add the Capability.

Provide-Capability: osgi.service;objectClass:List<String>="javax.script.ScriptEngineFactory";effective:=active,osgi.serviceloader;osgi.serviceloader="javax.script.ScriptEngineFactory";register:="org.mozilla.javascript.engine.RhinoScriptEngineFactory"
Require-Capability: osgi.extender;filter:="(&(osgi.extender=osgi.serviceloader.registrar)(version>=1.0.0)(!(version>=2.0.0)))"

also a missing import in core

java.lang.NoClassDefFoundError: javax/lang/model/SourceVersion
	at org.mozilla.javascript.JavaMembers.<clinit>(JavaMembers.java:37)
	at org.mozilla.javascript.NativeJavaClass.initMembers(NativeJavaClass.java:50)
	at org.mozilla.javascript.NativeJavaObject.<init>(NativeJavaObject.java:53)
	at org.mozilla.javascript.NativeJavaClass.<init>(NativeJavaClass.java:44)
	at org.mozilla.javascript.NativeJavaClass.<init>(NativeJavaClass.java:40)
	at org.mozilla.javascript.WrapFactory.wrapJavaClass(WrapFactory.java:136)
	at org.mozilla.javascript.NativeJavaPackage.getPkgProperty(NativeJavaPackage.java:134)
	at org.mozilla.javascript.NativeJavaPackage.get(NativeJavaPackage.java:84)
	at org.mozilla.javascript.ScriptableObject.getProperty(ScriptableObject.java:2023)
	at org.mozilla.javascript.ScriptRuntime.getObjectProp(ScriptRuntime.java:1672)
	at org.mozilla.javascript.ScriptRuntime.getObjectProp(ScriptRuntime.java:1667)
	at org.mozilla.javascript.gen.eval_1._c_getParameterTypes_8(eval:23)
	at org.mozilla.javascript.gen.eval_1.call(eval)
	at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:380)
	at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:3868)
	at org.mozilla.javascript.gen.eval_1.call(eval)
	at org.mozilla.javascript.engine.RhinoScriptEngine.invokeMethodRaw(RhinoScriptEngine.java:218)
	at org.mozilla.javascript.engine.RhinoInvocationHandler.invoke(RhinoInvocationHandler.java:23)

next Steps?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants