[jrubyscripting] Implement dependency tracking #13810

ccutrer · 2022-11-30T21:38:30Z

watchers had to be refactored similiar to jsscripting. it supports watching any directory referenced from RUBYLIB, as well as the gem home. it properly excludes lib and gem home (as well as other gem homes if you have multiple jruby versions installed) from loading as regular scripts.

this is a breaking change if you don't have RUBYLIB explicitly configured, and you are using the old default directory.

it's expected that the detection of what files and gems any given script uses will be self-identified by the script, presumably by the helper library.

JRubyScriptEngineConfiguration was largely refactored as part of this.

CONFIGURATION_PARAMETERS was renamed, and is no longer static, since it's modified every time the configuration is changed
OptionalConfigurationElement was simplified since default values are always provided now. this also simplified lots of other code that accesses the current settings.

inject ourselves into file and gem loading, and then notify OpenHAB of every file and gem that's loaded depends on openhab/openhab-addons#13810 to have any effect

ccutrer · 2022-11-30T21:39:13Z

ccutrer/openhab-jrubyscripting@3ba1054 is the commit to enable this functionality in the helper library

Reference openhab/openhab-addons#13810. Signed-off-by: Cody Cutrer <cody@cutrer.us>

watchers had to be refactored similiar to jsscripting. it supports watching any directory referenced from RUBYLIB, as well as the gem home. it properly excludes lib and gem home (as well as other gem homes if you have multiple jruby versions installed) from loading as regular scripts. this is a breaking change if you don't have RUBYLIB explicitly configured, and you are using the old default directory. it's expected that the detection of what files and gems any given script uses will be self-identified by the script, presumably by the helper library. JRubyScriptEngineConfiguration was largely refactored as part of this. * CONFIGURATION_PARAMETERS was renamed, and is no longer static, since it's modified every time the configuration is changed * OptionalConfigurationElement was simplified since default values are always provided now. this also simplified lots of other code that accesses the current settings. Signed-off-by: Cody Cutrer <cody@cutrer.us>

jimtng · 2022-12-02T00:56:04Z

This works well in my installation.

jlaur

Thanks for this adaptation/improvement! Just two questions from my side. @J-N-K, perhaps you could have a quick look as you recently did the same with #13756?

...main/java/org/openhab/automation/jrubyscripting/internal/JRubyScriptEngineConfiguration.java

jlaur · 2022-12-04T10:43:42Z

...scripting/src/main/java/org/openhab/automation/jrubyscripting/internal/watch/BidiSetBag.java

+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+
+// Copy of org.openhab.core.automation.module.script.rulesupport.internal.loader.collection.BidiSetBag


For my understanding, why does it need to be copied? Is it modified in any way, or just plain duplication?

It's in an internal package and therefore not exported from the core package.

If there is a need to reuse it in addons, would it make sense to expose it then (to avoid duplication)?

Honestly, I'd prefer if I didn't have to copy or reference it. I had to because the AbstractDependencyTracker isn't flexible enough for my needs - it assumes a single library directory that it must watch. If it were to be improved, then I could not need to reference this class at all.

J-N-K

Otherwise LGTM

J-N-K · 2022-12-04T11:09:36Z

...scripting/src/main/java/org/openhab/automation/jrubyscripting/internal/watch/BidiSetBag.java

+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+
+// Copy of org.openhab.core.automation.module.script.rulesupport.internal.loader.collection.BidiSetBag


It's in an internal package and therefore not exported from the core package.

J-N-K · 2022-12-04T11:12:55Z

...src/main/java/org/openhab/automation/jrubyscripting/internal/watch/JRubyGemWatchService.java

+ * @author Cody Cutrer - Initial contribution
+ */
+@NonNullByDefault
+public class JRubyGemWatchService extends AbstractWatchService {


I'm not so happy with these tons of new watch services, but this might improve after we have openhab/openhab-core#3004 merged which simplifies the interface.

Oooh. That'll be nice. To slightly assuage your concerns in the meantime, while this is setup to allow an arbitrary number of watch services, 99% of the time it will be the default config, and there will be just 2 - one for GEM_HOME, and a single one for RUBYLIB. And both of those will be directories nested underneath the JRubyScriptFileWatcher, so the underlying WatchQueueReader won't actually be watching any more, but will have more bookkeeping.

J-N-K · 2022-12-04T11:19:48Z

...c/main/java/org/openhab/automation/jrubyscripting/internal/watch/JRubyDependencyTracker.java

+ * @author Cody Cutrer - Initial contribution
+ */
+@NonNullByDefault
+public class JRubyDependencyTracker implements ScriptDependencyTracker {


This could be a service (component) itself which then is injected into the script engine factory (like in js). I didn't check where the individual configurations values come from, but likely they can be derived from the ConfigAdmin.

Edit another option would be that the script engine factory itself implements ScriptDependencyTracker and returns this in getDependencyTracker().

I tried really hard to get this to be an OSGi component. I actually started by just copying the JS files. But I couldn't get around the circular dependence - JRubyScriptEngineFactory needs to return the dependency tracker. But the dependency tracker needs access to the config. On the one hand, like you say, I could just use ConfigAdmin directly to get the current configuration values, but the raw configuration is wrapped by Java both to provide non-constant defaults (well, it is theoretically constant, but dependent on values from the JRuby bundle), as well as providing some post-processing to the values. I definitely didn't want to attempt to duplicate that.

I also considered just having the JRubyScriptEngineFactory implement ScriptDependencyTracker like you said, but I wanted to keep it separate in hopes of eliminating the bulk of this class by inheriting from AbstractScriptDependencyTracker as discussed in the other comment.

Another option I've thought of, but didn't really explore, is having the configuration itself be an independent component that both JRubyScriptEngineFactory and JRubyDependencyTracker depend on. If you're adamant that you really want this to be an OSGi component, I can explore that.

I would suggest to keep it like it is now and refactor it later when the core watch service is changed. As a result it should also be possible to easily adapt the AbstractScriptDependencyTracker to watch for more than one directory. Since that one needs refactoring anyway with the new watch service, I would prefer not to touch it before the release because time is a bit short.

… exist Signed-off-by: Cody Cutrer <cody@cutrer.us>

even though Ruby prefers dependency_listener over dependencyListener, it just felt weird when in a list next to the other globals injected by openHAB Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer · 2022-12-05T03:44:48Z

@jlaur: sounds like we're going to punt on @J-N-K's comments until core has an updated file watcher. I've addressed your comment on the gem home directory creation. Anything else?

jlaur · 2022-12-05T07:18:49Z

I've addressed your comment on the gem home directory creation. Anything else?

No, let's merge. :-)

jlaur

LGTM

openhab-bot · 2022-12-05T11:07:27Z

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

https://community.openhab.org/t/jruby-scripting-with-jruby-9-4/141395/1

openhab-bot · 2022-12-05T11:09:59Z

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

https://community.openhab.org/t/jruby-scripting-with-jruby-9-4/141395/4

* [jrubyscripting] implement dependency tracking watchers had to be refactored similar to jsscripting. it supports watching any directory referenced from RUBYLIB, as well as the gem home. it properly excludes lib and gem home (as well as other gem homes if you have multiple jruby versions installed) from loading as regular scripts. this is a breaking change if you don't have RUBYLIB explicitly configured, and you are using the old default directory. it's expected that the detection of what files and gems any given script uses will be self-identified by the script, presumably by the helper library. JRubyScriptEngineConfiguration was largely refactored as part of this. * CONFIGURATION_PARAMETERS was renamed, and is no longer static, since it's modified every time the configuration is changed * OptionalConfigurationElement was simplified since default values are always provided now. this also simplified lots of other code that accesses the current settings. Signed-off-by: Cody Cutrer <cody@cutrer.us> Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>

* [jrubyscripting] implement dependency tracking watchers had to be refactored similar to jsscripting. it supports watching any directory referenced from RUBYLIB, as well as the gem home. it properly excludes lib and gem home (as well as other gem homes if you have multiple jruby versions installed) from loading as regular scripts. this is a breaking change if you don't have RUBYLIB explicitly configured, and you are using the old default directory. it's expected that the detection of what files and gems any given script uses will be self-identified by the script, presumably by the helper library. JRubyScriptEngineConfiguration was largely refactored as part of this. * CONFIGURATION_PARAMETERS was renamed, and is no longer static, since it's modified every time the configuration is changed * OptionalConfigurationElement was simplified since default values are always provided now. this also simplified lots of other code that accesses the current settings. Signed-off-by: Cody Cutrer <cody@cutrer.us> Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>

* [jrubyscripting] implement dependency tracking watchers had to be refactored similar to jsscripting. it supports watching any directory referenced from RUBYLIB, as well as the gem home. it properly excludes lib and gem home (as well as other gem homes if you have multiple jruby versions installed) from loading as regular scripts. this is a breaking change if you don't have RUBYLIB explicitly configured, and you are using the old default directory. it's expected that the detection of what files and gems any given script uses will be self-identified by the script, presumably by the helper library. JRubyScriptEngineConfiguration was largely refactored as part of this. * CONFIGURATION_PARAMETERS was renamed, and is no longer static, since it's modified every time the configuration is changed * OptionalConfigurationElement was simplified since default values are always provided now. this also simplified lots of other code that accesses the current settings. Signed-off-by: Cody Cutrer <cody@cutrer.us>

* [jrubyscripting] implement dependency tracking watchers had to be refactored similar to jsscripting. it supports watching any directory referenced from RUBYLIB, as well as the gem home. it properly excludes lib and gem home (as well as other gem homes if you have multiple jruby versions installed) from loading as regular scripts. this is a breaking change if you don't have RUBYLIB explicitly configured, and you are using the old default directory. it's expected that the detection of what files and gems any given script uses will be self-identified by the script, presumably by the helper library. JRubyScriptEngineConfiguration was largely refactored as part of this. * CONFIGURATION_PARAMETERS was renamed, and is no longer static, since it's modified every time the configuration is changed * OptionalConfigurationElement was simplified since default values are always provided now. this also simplified lots of other code that accesses the current settings. Signed-off-by: Cody Cutrer <cody@cutrer.us> Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>

ccutrer requested a review from boc-tothefuture as a code owner November 30, 2022 21:38

ccutrer added a commit to ccutrer/openhab-distro that referenced this pull request Nov 30, 2022

Add breaking change warning for JRuby Scripting

859dba0

Reference openhab/openhab-addons#13810. Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer mentioned this pull request Nov 30, 2022

Add breaking change warning for JRuby Scripting openhab/openhab-distro#1425

Merged

ccutrer force-pushed the jrubyscripting-dependency-tracking branch from 78f80b1 to 716d1fc Compare November 30, 2022 21:58

jlaur added enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible labels Dec 1, 2022

jlaur requested changes Dec 4, 2022

View reviewed changes

J-N-K reviewed Dec 4, 2022

View reviewed changes

ccutrer added 2 commits December 4, 2022 08:28

[jrubyscripting] return early from configureGems if gem_home does not…

7ee2d53

… exist Signed-off-by: Cody Cutrer <cody@cutrer.us>

[jrubyscripting] change name of dependency listener global

29f53c5

even though Ruby prefers dependency_listener over dependencyListener, it just felt weird when in a list next to the other globals injected by openHAB Signed-off-by: Cody Cutrer <cody@cutrer.us>

jlaur approved these changes Dec 5, 2022

View reviewed changes

jlaur merged commit 2382fad into openhab:main Dec 5, 2022

jlaur added this to the 3.4 milestone Dec 5, 2022

jlaur changed the title ~~[jrubyscripting] implement dependency tracking~~ [jrubyscripting] Implement dependency tracking Dec 5, 2022

ccutrer deleted the jrubyscripting-dependency-tracking branch December 5, 2022 16:24

jlaur mentioned this pull request Dec 13, 2022

[jrubyscripting] remove some development logging that crept in #13947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jrubyscripting] Implement dependency tracking #13810

[jrubyscripting] Implement dependency tracking #13810

ccutrer commented Nov 30, 2022

ccutrer commented Nov 30, 2022

jimtng commented Dec 2, 2022

jlaur left a comment

jlaur Dec 4, 2022

J-N-K Dec 4, 2022

jlaur Dec 4, 2022

ccutrer Dec 4, 2022

J-N-K left a comment

J-N-K Dec 4, 2022

J-N-K Dec 4, 2022

ccutrer Dec 4, 2022

J-N-K Dec 4, 2022 •

edited

Loading

ccutrer Dec 4, 2022

J-N-K Dec 4, 2022

ccutrer commented Dec 5, 2022

jlaur commented Dec 5, 2022

jlaur left a comment

openhab-bot commented Dec 5, 2022

openhab-bot commented Dec 5, 2022


		import org.eclipse.jdt.annotation.NonNullByDefault;

		// Copy of org.openhab.core.automation.module.script.rulesupport.internal.loader.collection.BidiSetBag

[jrubyscripting] Implement dependency tracking #13810

[jrubyscripting] Implement dependency tracking #13810

Conversation

ccutrer commented Nov 30, 2022

ccutrer commented Nov 30, 2022

jimtng commented Dec 2, 2022

jlaur 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

J-N-K 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

J-N-K Dec 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccutrer commented Dec 5, 2022

jlaur commented Dec 5, 2022

jlaur left a comment

Choose a reason for hiding this comment

openhab-bot commented Dec 5, 2022

openhab-bot commented Dec 5, 2022

J-N-K Dec 4, 2022 •

edited

Loading