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

Add Jython and Groovy ScriptEngineFactories #519

Closed
wants to merge 3 commits into from

Conversation

5iver
Copy link

@5iver 5iver commented Feb 2, 2019

... and add support for Jython and Groovy scripted Actions and Conditions in Paper UI.

Fixes #521

Signed-off-by: Scott Rushworth openhab@5iver.com (github: openhab-5iver)

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

I have one general concern with this change:
The reason that only Javascript is listed is that it is always present.
For Groovy and Jython, additional libraries need to be in place. For installations, where those are not installed, the UI should also not display these options.
I'd thus think that the registration of the factories + config description should be constructed dynamically, wdyt?

@@ -0,0 +1,64 @@
/**
* Copyright (c) 2019 Contributors to the Eclipse Foundation
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if I should use this...

Copyright (c) 2010-${year} Contributors to the openHAB project

Or...

Copyright (c) 2010-2019 Contributors to the openHAB project

Copy link
Member

Choose a reason for hiding this comment

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

Clearly the version with the years, not the placeholders!

Copy link
Author

Choose a reason for hiding this comment

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

I thought so... but clearly not so clear to me! Thank you! 😄

import org.slf4j.LoggerFactory;

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

Please add some Javadoc

import org.slf4j.LoggerFactory;

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

Please add some Javadoc

@5iver
Copy link
Author

5iver commented Feb 3, 2019

I'd thus think that the registration of the factories + config description should be constructed dynamically, wdyt?

Absolutely, but I did not see how to dynamically construct it. Could you please point me in the right direction on how to do that?

@kaikreuzer
Copy link
Member

Well, the first thing is to figure out how to determine whether support of these languages is there or not.
Then you could write some code that registers a marker service and have a mandatory dependency of the factory to that service.

For the config options, this should be removed from the static json file and be implemented as an ConfigOptionProvider instead.

@5iver
Copy link
Author

5iver commented Feb 3, 2019

Well, the first thing is to figure out how to determine whether support of these languages is there or not.

  1. We could include the libraries by default, but that would take up about 42Mb to the distribution, so that is likely not an option.

  2. We could add Jython and Groovy bundles that install the necessary libraries in the classpath, and possibly in the future include some helper libraries and scripts. With this option, we could check for the existence of the bundle(s) in the ConfigOptionProvider.

  3. In the ConfigOptionProvider, we could check to see if the libraries are in the classpath.

Option 3 looks best to me, in case the libraries are manually installed. Adding some bundles for JSR223 libraries is on my list though! What type of bundles would these be?

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/configuration-of-openhab/64682/203

@kaikreuzer
Copy link
Member

I think 2+3 should be combined.

Adding some bundles for JSR223 libraries is on my list though! What type of bundles would these be?

What do you mean by type? Wrt the id, it could be org.openhab.core.automation.module.script.jython and org.openhab.core.automation.module.script.groovy.

@5iver
Copy link
Author

5iver commented Feb 5, 2019

I think 2+3 should be combined.

If 3 is implemented, I don't see any need for 2. The bundle could be installed but misconfigured. Checking the classpath will tell us for certain if the libraries are available for use.

But I'm rethinking this. Instead of checking the classpath, I can get a list of ScriptEngines from the ScriptEngineManager. I also don't think these factories will be needed, since they are really just providing the MimeType for the ScriptEngine, which is used in the ScriptTypes.json. But I'm thinking the name might be able to be used (need to test), which can also be provided by the SEM. This will also allow for something like a Kotlin library to be added into the the classpath, a ScriptEngine would be created by the GenericScriptEngineFactory, and then it would show up as an option in Paper UI.

What do you mean by type?

I was meaning binding, io, etc., but don't know the right terminology for it. I'm confused by org.openhab.core.automation.module.script.jython, as I was thinking these would be in openhab2-addons.

I have a number of questions, so I opened a separate issue for Jython... https://github.com/openhab/openhab2-addons/issues/4801, and I'll open another for Groovy.

@5iver
Copy link
Author

5iver commented Mar 7, 2019

Replaced with #635.

@5iver 5iver closed this Mar 7, 2019
@5iver 5iver deleted the add-se-factories branch March 7, 2019 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants