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] Apply RUBYLIB configuration to $LOAD_PATH #12123

Merged
merged 2 commits into from
Jan 29, 2022

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Jan 26, 2022

The rubylib addon configuration is supposed to alter the $LOAD_PATH but currently it doesn't. As a result, personal libraries located in RUBYLIB path won't be found. Currently a workaround is applied in the helper library to parse RUBYLIB and add the paths into $LOAD_PATH.

However, GUI users who opt not to use the helper library will not be able to load personal libraries based on the rubylib search path.

This PR remedies this situation by updating $LOAD_PATH within the addon.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@lolodomo lolodomo added the enhancement An enhancement or new feature for an existing add-on label Jan 28, 2022
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng jimtng requested a review from lolodomo January 28, 2022 09:45
@jimtng
Copy link
Contributor Author

jimtng commented Jan 28, 2022

@lolodomo I've changed it to warn. There are two other existing logger calls along the same line that use error instead of warn. Should I change them too?

@lolodomo
Copy link
Contributor

Code guideline: https://www.openhab.org/docs/developer/guidelines.html#f-logging
Extract from doc;

error logging should only be used

  • to inform the user that something is tremendously wrong in the setup, the system cannot function normally anymore, and there is a need for immediate action.
  • in case some code fails irrecoverably and the user should report it as a severe bug.

So error should rarely be used.

If you see other inappropriate usages of error, yes please fix them too.

@jimtng
Copy link
Contributor Author

jimtng commented Jan 28, 2022

@lolodomo I've given this further thought, but I still can't determine with 100% certainty whether error or warn should be used here. I am leaning towards using error as in the first commit.

Essentially the code tries to add to the $LOAD_PATH global variable, and catches the exception in case of error. In such situation, the intended effect would not have been achieved, so user scripts that want to load additional libraries from the LOAD_PATH would fail.

Wouldn't this be considered that something is "tremendously wrong and cannot function normally"? Furthermore, actions are required from the user to remedy the situation (by fixing the config that caused the error).

@lolodomo
Copy link
Contributor

lolodomo commented Jan 29, 2022

As I understand, you are adding a new feature, not a mandatory feature, the system can work well without it. Not a critical error.

warn logging should only be used

  • to inform the user that something seems to be wrong in the overall setup, but the system can nonetheless function as normal,
  • in recoverable situations when a section of code that is not accessed under normal operating conditions is reached.

@jimtng
Copy link
Contributor Author

jimtng commented Jan 29, 2022

This isn't actually a new feature, more a bug fix. Originally the addon allows to set rubylib in the config. The addon then sets the corresponding RUBYLIB environment variable, which is supposed to search for user's scripts in the given paths.

require 'mylibrary`

However this isn't currently working.

So this PR adds RUBYLIB into LOAD_PATH post scriptEngine instantiation to make it work. The same solution is currently implemented within the jruby helper library, but since this helper library isn't a part of the addon, the best solution is to incorporate this patch into the addon.

@jimtng
Copy link
Contributor Author

jimtng commented Jan 29, 2022

The RUBYLIB setting will not work for those who aren't using the helper library (eg in a GUI rule).

A decision was made to implement the fix in the helper library first because it was the easiest to do at the time. This PR now includes the fix in the addon, so it would work with or without the helper library, e.g. in a GUI rule. See the discussion here boc-tothefuture/openhab-jruby#267 (comment) and here boc-tothefuture/openhab-jruby#267 (comment)

@jimtng
Copy link
Contributor Author

jimtng commented Jan 29, 2022

@lolodomo if you believe the proper level is indeed warn, then this PR can be merged now.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM

@lolodomo lolodomo merged commit 4e12200 into openhab:main Jan 29, 2022
@lolodomo lolodomo added this to the 3.3 milestone Jan 29, 2022
@jimtng jimtng deleted the jruby-rubylib branch January 29, 2022 11:21
volkmarnissen pushed a commit to volkmarnissen/openhab-addons that referenced this pull request Mar 3, 2022
…2123)

* [jrubyscripting] Apply RUBYLIB configuration to $LOAD_PATH

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Apr 27, 2022
…2123)

* [jrubyscripting] Apply RUBYLIB configuration to $LOAD_PATH

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jun 29, 2022
…2123)

* [jrubyscripting] Apply RUBYLIB configuration to $LOAD_PATH

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
…2123)

* [jrubyscripting] Apply RUBYLIB configuration to $LOAD_PATH

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
…2123)

* [jrubyscripting] Apply RUBYLIB configuration to $LOAD_PATH

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants