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

[jrubyscripting] Support using Gemfile with Bundler #18404

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Mar 17, 2025

This PR is based on #18374 and will be rebased once that's been merged.

- For UI users, the content of the Gemfile can be entered in the addon configuration. This will be saved in USERDATA/tmp/jrubyscripting/Gemfile and used. Gemfile.lock will be created in the same directory.

  • By default, if bundle_gemfile (default: CONF/automation/ruby/Gemfile) exists, it takes priority over the gems setting.
  • On startup bundle install / bundle update is executed (depending on check_update setting)
  • When a Gemfile exists, Bundler.setup + require will be executed on user script startups.

The configuration system has been refactored to support the use of multiline / List configuration, to support entering UI Gemfile but that idea is dropped for now. The refactoring should stay nevertheless.

Note: update USAGE.md and README after this is merged

@jimtng jimtng added enhancement An enhancement or new feature for an existing add-on awaiting other PR Depends on another PR labels Mar 17, 2025
@jimtng jimtng changed the title Jruby bundler @jimtng [jrubyscripting] Support using Gemfile with Bundler Mar 17, 2025
@jimtng jimtng changed the title @jimtng [jrubyscripting] Support using Gemfile with Bundler [jrubyscripting] Support using Gemfile with Bundler Mar 17, 2025
@jimtng jimtng force-pushed the jruby-bundler branch 2 times, most recently from 2faabd9 to d6f4826 Compare March 17, 2025 12:57
@jimtng jimtng marked this pull request as ready for review March 18, 2025 01:47
@jimtng jimtng requested a review from ccutrer as a code owner March 18, 2025 01:47
Copy link
Contributor

@ccutrer ccutrer left a comment

Choose a reason for hiding this comment

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

I'm debating the usefulness of being able to write a full Gemfile in the UI. on the one hand, it's definitely helpful for UI users that want to use gems, without having to use bundler/inline in every rule/script they write to get those gems. on the other, having a true lockfile almost necessarily necessitates being able to run bundler to manage it. Which is now easily accomplished via the console, but that's already a command line tool only, not UI. Perhaps if we can add buttons to the add-on configuration page - "Update Bundle" and "Clear Gems" (with the latter clearing both the entire GEM_HOME, as well as removing the Gemfile.lock).

Eventually we could remove the old "Ruby Gems" config option, and default the UI-configured Gemfile to have the following contents:

source "https://rubygems.org"

gem "openhab-scripting", "~> 5.0"

Thus scripts would always run in the context of a bundle. I just tested, and bundler/inline still works even when run in the context of an existing bundle (though if they have requirements in the inline gemfile that conflict with the full Gemfile, it would obviously cause errors). But it would cleanup our weird parsing of the Gems config option, and we also wouldn't need a default value for the require setting at all (assuming we add a file called openhab-scripting.rb to the helper library, so that bundler can auto-require it properly).

}
}

public void bundlerInstall(ScriptEngine engine) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this method is unnecessary with the auto_install change above. though it could still make sense to change it to the equivalent of bundle update, and control it with the same config setting as is used to check for updates at all

<advanced>true</advanced>
</parameter>

<parameter name="bundle_install" type="boolean" required="true" groupName="bundler">
Copy link
Contributor

Choose a reason for hiding this comment

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

this configuration parameter is unnecessary with the auto_install Bundler setting above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the removal of this, and using auto_install: true how could we provide an option to not auto install (and just do Bundler.setup only) to speed up startup? It would skip the need to check against source (rubygems.org).

I haven't tested what would happen with a system without an internet connection when bundler install runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've decided to keep it and run bundler install / update on startup. That would speed up the execution of the first script because the installation is already done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But instead of bundle_install parameter, I kept the old check_update and use it for bundler too.

try {
logger.debug("Running Bundler on Gemfile {}", gemfilePath);
logger.trace("Bundler code:\n{}", code);
engine.eval(code);
Copy link
Contributor

Choose a reason for hiding this comment

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

if bundler logs anything to stdout/stderr, does that end up in openhab.log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the errors end up in openhab log. Example:

2025-03-20 22:54:09.009 [ERROR] [ernal.JRubyScriptEngineConfiguration] - Error running Bundler setup: (DSLError) 
[!] There was an error parsing `Gemfile`: Undefined local variable or method `asdfsdfs' for Gemfile. Bundler cannot continue.

 #  from /openhab/conf/automation/ruby/Gemfile:6
 #  -------------------------------------------
 #  # gem "rails"
 >  asdfsdfs
 #  gem "faraday", require: false
 #  -------------------------------------------

logger.trace("Bundler code:\n{}", code);
engine.eval(code);
} catch (ScriptException e) {
logger.warn("Error running Bundler setup: {}", e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

how does bundler react when it can't resolve the bundle (i.e. if you have conflicting gem requirements or something)? I know "normally" it would log to stderr, and exit with a non-zero exit code. what happens if something in Ruby calls Kernel.exit? is an exception thrown here? What I'm getting at is that if the bundle can't be setup, should we prevent the rest of the script from even attempting to load?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I'm getting at is that if the bundle can't be setup, should we prevent the rest of the script from even attempting to load?

Yes, perhaps we should do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently it just continues to try loading the script, but failed at loading the openhab/dsl require. Full log:

2025-03-20 22:54:09.009 [ERROR] [ernal.JRubyScriptEngineConfiguration] - Error running Bundler setup: (DSLError) 
[!] There was an error parsing `Gemfile`: Undefined local variable or method `asdfsdfs' for Gemfile. Bundler cannot continue.

 #  from /openhab/conf/automation/ruby/Gemfile:6
 #  -------------------------------------------
 #  # gem "rails"
 >  asdfsdfs
 #  gem "faraday", require: false
 #  -------------------------------------------

2025-03-20 22:54:09.231 [ERROR] [ipt.internal.ScriptEngineManagerImpl] - Error during evaluation of script '/openhab/conf/automation/ruby/test.rb': Error during evaluation of Ruby in /openhab/conf/automation/ruby/test.rb at line 21: (NameError) undefined local variable or method `logger' for main:Object
2025-03-20 22:54:09.231 [WARN ] [ort.loader.AbstractScriptFileWatcher] - Script loading error, ignoring file '/openhab/conf/automation/ruby/test.rb'

@jimtng
Copy link
Contributor Author

jimtng commented Mar 18, 2025

Perhaps if we can add buttons to the add-on configuration page - "Update Bundle" and "Clear Gems" (with the latter clearing both the entire GEM_HOME, as well as removing the Gemfile.lock).

Unfortunately, AFAIK, this is mechanism is not currently available through the UI. We would need to invent a new mechanism to support this, and that would involve making changes to core, which usually takes a long time to get approved.

Eventually we could remove the old "Ruby Gems" config option, and default the UI-configured Gemfile to have the following contents:

source "https://rubygems.org"

gem "openhab-scripting", "~> 5.0"

This is really a great idea. Some thoughts:

  • It would be kind of a breaking change. People with custom gems setting would have to make manual changes / entry to their config
  • openHAB 5.0 is a good reason to make these changes

@jimtng jimtng marked this pull request as draft March 20, 2025 01:41
@jimtng
Copy link
Contributor Author

jimtng commented Mar 20, 2025

Since there's no easy way to have a UI button to perform actions, I've decided to revert the UI Gemfile option. So now it's back to the original gems setting, if no Gemfile exists. So you're either using gems settings + the mandatory require, or you're using Gemfile (but the require still applies in case you want to require your personal libraries).

This way it doesn't introduce any changes to existing setup - unless they happen to have a Gemfile lying around in their automation/ruby folder.

jimtng added 2 commits March 20, 2025 23:37
Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Jimmy Tanagra <[email protected]>
@jimtng jimtng requested a review from ccutrer March 21, 2025 09:42
@jimtng
Copy link
Contributor Author

jimtng commented Mar 24, 2025

Perhaps if we can add buttons to the add-on configuration page - "Update Bundle" and "Clear Gems" (with the latter clearing both the entire GEM_HOME, as well as removing the Gemfile.lock).

A way we can provide something like this, is by creating the corresponding script, generated by the add-on. The users will see them in the "Scripts" section of the UI and they can run it to perform the task. WDYT?

If we want to go this direction, I think we do it in a separate PR too. I am not too convinced that this extra complexity is a good thing for UI users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting other PR Depends on another PR enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants