Bug 26163 - Add plugin directories to PERL5LIB
Summary: Add plugin directories to PERL5LIB
Status: RESOLVED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 25778
Blocks: 25698
  Show dependency treegraph
 
Reported: 2020-08-06 15:01 UTC by Kyle M Hall
Modified: 2020-08-27 11:30 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 26163: Add plugin directories to PERL5LIB (2.32 KB, patch)
2020-08-06 15:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26163: Add plugin directories to PERL5LIB (2.28 KB, patch)
2020-08-18 14:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26163: Add plugin directories to PERL5LIB (2.23 KB, patch)
2020-08-27 02:14 UTC, Galen Charlton
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-08-06 15:01:46 UTC
It would be nice if we could create Koha objects and schema files to be used in plugins. The problem is that by the time the 
Plugin directories are added to @INC only at the point that Koha::Plugins is evaluated. This is too late for loading additional Schema files that may be part of a plugin.

In addition, it makes sense to allow plugins to have a 'lib' directory so that any additional object and schema files can be stored inside the plugin assets directory and not pollute the root plugins directory. Without such a 'lib' directory, we would need to place them in the plugins directory such as '/var/lib/koha/instancename/plugins/Koha/MyObject.pm'. This file would not be removed when the plugin is uninstalled.
Comment 1 Kyle M Hall 2020-08-06 15:56:54 UTC
Created attachment 107899 [details] [review]
Bug 26163: Add plugin directories to PERL5LIB

It would be nice if we could create Koha objects and schema files to be used in plugins. The problem is that by the time the
Plugin directories are added to @INC only at the point that Koha::Plugins is evaluated. This is too late for loading additional Schema files that may be part of a plugin.

In addition, it makes sense to allow plugins to have a 'lib' directory so that any additional object and schema files can be stored inside the plugin assets directory and not pollute the root plugins directory. Without such a 'lib' directory, we would need to place them in the plugins directory such as '/var/lib/koha/instancename/plugins/Koha/MyObject.pm'. This file would not be removed when the plugin is uninstalled.

Test Plan:
1) Apply this patch
2) Start koha-testing-docker or Update your plack.psgi file with the changes from the patch ( e.g. /etc/koha/sites/kohadevbox/plack.psgi )
3) Install the Kitchen Sink plugin, version 2.1.42
   https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.42/koha-plugin-kitchen-sink-v2.1.42.kpz
4) Restart all the things!
5) Note the message "Kitchen Sink test module was loaded!" in the plack error log when you visit the plugin's pages ( confi, tool, report )
Comment 2 David Cook 2020-08-07 00:00:40 UTC
Could you elaborate more on the rationale for this change? I don't understand it at all.

Also, the change to koha-plack has the same problem as Bug 25778. The PERL5LIB will grow and grow each time you restart that starman instance.
Comment 3 Kyle M Hall 2020-08-11 11:08:40 UTC
(In reply to David Cook from comment #2)
> Could you elaborate more on the rationale for this change? I don't
> understand it at all.
> 
> Also, the change to koha-plack has the same problem as Bug 25778. The
> PERL5LIB will grow and grow each time you restart that starman instance.

Basically, it's so that plugins can have DBIC schema files.

We should be able to test PERL5LIB to see if the path already exists in and to not add it if it does.
Comment 4 David Cook 2020-08-11 23:30:45 UTC
(In reply to Kyle M Hall from comment #3)
> Basically, it's so that plugins can have DBIC schema files.
> 

I was hoping for more of a technical explanation, but I think I understand after re-reading the earlier comments. The plugin developer would provide DBIC schema files with the plugin in the lib directory, and if the plugin is loaded early enough, then Koha::Database will detect them?

I am all for using DBIC schema files. It would make plugins a lot easier to write.

However, wouldn't that mean that you'd have to restart Starman before being able to use the plugin? Shouldn't that requirement should be surfaced to users (even if only as a warning)? 

> We should be able to test PERL5LIB to see if the path already exists in and
> to not add it if it does.

Since koha-plack uses bash, we could try something like the following (although I admit it uses bash magic I haven't fully understood yet):

[[ ":$PATH:" != *":/path/to/add:"* ]] && PATH="${PATH}:/path/to/add"

(https://unix.stackexchange.com/questions/217622/add-path-to-path-if-not-already-in-path)
Comment 5 Tomás Cohen Arazi 2020-08-14 16:10:13 UTC
This is such a great idea Kyle. We need to document it once pushed. I hope we can even backport this.
Comment 6 Tomás Cohen Arazi 2020-08-14 16:10:46 UTC
I made it dependent on bug 25778 because that bugfix should be pushed ASAP as well, to the stable branches.
Comment 7 David Cook 2020-08-16 23:43:41 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> I made it dependent on bug 25778 because that bugfix should be pushed ASAP
> as well, to the stable branches.

Thanks, Tomas. That would remove my concern!
Comment 8 Tomás Cohen Arazi 2020-08-18 14:58:42 UTC
Created attachment 108524 [details] [review]
Bug 26163: Add plugin directories to PERL5LIB

It would be nice if we could create Koha objects and schema files to be used in plugins. The problem is that by the time the
Plugin directories are added to @INC only at the point that Koha::Plugins is evaluated. This is too late for loading additional Schema files that may be part of a plugin.

In addition, it makes sense to allow plugins to have a 'lib' directory so that any additional object and schema files can be stored inside the plugin assets directory and not pollute the root plugins directory. Without such a 'lib' directory, we would need to place them in the plugins directory such as '/var/lib/koha/instancename/plugins/Koha/MyObject.pm'. This file would not be removed when the plugin is uninstalled.

Test Plan:
1) Apply this patch
2) Start koha-testing-docker or Update your plack.psgi file with the changes from the patch ( e.g. /etc/koha/sites/kohadevbox/plack.psgi )
3) Install the Kitchen Sink plugin, version 2.1.42
   https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.42/koha-plugin-kitchen-sink-v2.1.42.kpz
4) Restart all the things!
5) Note the message "Kitchen Sink test module was loaded!" in the plack error log when you visit the plugin's pages ( confi, tool, report )

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 David Cook 2020-08-18 23:12:48 UTC
I came to apologize for not signing this one off last night, but looks like Tomas has already signed off. Hurray! 

I'm actually quite looking forward to this change going in.
Comment 10 Katrin Fischer 2020-08-18 23:44:02 UTC
If you signed it off still, we could count Tomas for towards QA if he agrees.
Comment 11 David Cook 2020-08-19 00:13:31 UTC
(In reply to Katrin Fischer from comment #10)
> If you signed it off still, we could count Tomas for towards QA if he agrees.

Ok I could do that

I was actually wondering about that same methodology for Bug 22417...
Comment 12 Tomás Cohen Arazi 2020-08-19 00:16:32 UTC
I was going to write the same but got a mid air collision. Never hesitate to add your signature, even if I wasn't a QA team member. It means something for the QA members and the RM.
Comment 13 David Cook 2020-08-19 06:12:33 UTC
Failing QA.

The plugin doesn't show at http://localhost:8081/cgi-bin/koha/plugins/plugins-home.pl

I also get the following in /var/log/koha/kohadev/plack-error.log:

Can't locate TestModule.pm in @INC (you may need to install the TestModule module) (@INC contains: /kohadevbox/koha /kohadevbox/koha/installer /kohadevbox/koha/lib/installer /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/loca
l/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/kohadev/plugins) at /var/lib/ko
ha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink.pm line 27.
Compilation failed in require at /usr/share/perl/5.24/Module/Load.pm line 77.
Can't locate Koha/Plugin/Com/ByWaterSolutions/KitchenSink in @INC (@INC contains: /kohadevbox/koha /kohadevbox/koha/installer /kohadevbox/koha/lib/installer /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24
.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/kohadev/plugins) at /usr/share/perl/5.24/Module/
Load.pm line 77.
Kitchen Sink test module was loaded! at /var/lib/koha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink/lib/TestModule.pm line 4.
Comment 14 Kyle M Hall 2020-08-19 15:19:48 UTC
This is actually due to the plugin design being bad. Can you try again using the curbside pickup plugin (https://github.com/bywatersolutions/koha-plugin-curbside-pickup)? Basically, to make this work, the plugin needs to 'require' libraries in the lib directory using of 'use'ing them, as the libs won't be available until the plugin is installed and plack is restarted.

(In reply to David Cook from comment #13)
> Failing QA.
> 
> The plugin doesn't show at
> http://localhost:8081/cgi-bin/koha/plugins/plugins-home.pl
> 
> I also get the following in /var/log/koha/kohadev/plack-error.log:
> 
> Can't locate TestModule.pm in @INC (you may need to install the TestModule
> module) (@INC contains: /kohadevbox/koha /kohadevbox/koha/installer
> /kohadevbox/koha/lib/installer /etc/perl
> /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/loca
> l/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5
> /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24
> /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base
> /var/lib/koha/kohadev/plugins) at /var/lib/ko
> ha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink.pm line 27.
> Compilation failed in require at /usr/share/perl/5.24/Module/Load.pm line 77.
> Can't locate Koha/Plugin/Com/ByWaterSolutions/KitchenSink in @INC (@INC
> contains: /kohadevbox/koha /kohadevbox/koha/installer
> /kohadevbox/koha/lib/installer /etc/perl
> /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24
> .1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5
> /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24
> /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base
> /var/lib/koha/kohadev/plugins) at /usr/share/perl/5.24/Module/
> Load.pm line 77.
> Kitchen Sink test module was loaded! at
> /var/lib/koha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink/
> lib/TestModule.pm line 4.
Comment 15 Kyle M Hall 2020-08-19 15:20:29 UTC
(In reply to Kyle M Hall from comment #14)
> This is actually due to the plugin design being bad.

I just wanted to clarify that I'm mean I designed the kitchen sink plugin addition to test this badly.
Comment 16 David Cook 2020-08-20 02:27:02 UTC
(In reply to Kyle M Hall from comment #15)
> I just wanted to clarify that I'm mean I designed the kitchen sink plugin
> addition to test this badly.

No worries. I've just looked at the plugin and I see what you mean.

I'll have to take a look at it later, but I'll make a note to do it with the curbside plugin.
Comment 17 Galen Charlton 2020-08-27 02:14:46 UTC
Created attachment 109216 [details] [review]
Bug 26163: Add plugin directories to PERL5LIB

It would be nice if we could create Koha objects and schema files to be
used in plugins. The problem is that by the time the Plugin directories
are added to @INC only at the point that Koha::Plugins is evaluated.
This is too late for loading additional Schema files that may be part of
a plugin.

In addition, it makes sense to allow plugins to have a 'lib' directory
so that any additional object and schema files can be stored inside the
plugin assets directory and not pollute the root plugins directory.
Without such a 'lib' directory, we would need to place them in the
plugins directory such as '/var/lib/koha/instancename/plugins/Koha/MyObject.pm'.
This file would not be removed when the plugin is uninstalled.

Test Plan:
1) Apply this patch
2) Start koha-testing-docker or Update your plack.psgi file with the changes from the patch ( e.g. /etc/koha/sites/kohadevbox/plack.psgi )
3) Install the curbside pickup plugin, 1.0.9:
   https://github.com/bywatersolutions/koha-plugin-curbside-pickup/releases/tag/v1.0.9
4) Restart all the things!
5) Verify that the curbside pickup plugin's config and tools interfaces appear.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Galen Charlton <gmc@equinoxinitiative.org>
Comment 18 Galen Charlton 2020-08-27 02:19:21 UTC
The patch works for me with the curbside pickup plugin, and I'd be comfortable treating the kitchen sink plugin as not handling its interior modules as it should. Consequently, I've attached my signoff and tweaked the commit message a bit.

That said... this may not be necessary after all. I've worked out an approach to have plugins register their own custom DBIx result classes without requiring that koha-plack munge PERL5LIB in advance. See the pull request on the plugin here:

https://github.com/bywatersolutions/koha-plugin-curbside-pickup/pull/7

If that pull request holds up, the approach it outlines may well obviate any need for Koha itself to change. Consequently, I'm setting the bug status to in discussion for the moment.
Comment 19 Tomás Cohen Arazi 2020-08-27 02:28:21 UTC
(In reply to Galen Charlton from comment #18)
> The patch works for me with the curbside pickup plugin, and I'd be
> comfortable treating the kitchen sink plugin as not handling its interior
> modules as it should. Consequently, I've attached my signoff and tweaked the
> commit message a bit.
> 
> That said... this may not be necessary after all. I've worked out an
> approach to have plugins register their own custom DBIx result classes
> without requiring that koha-plack munge PERL5LIB in advance. See the pull
> request on the plugin here:
> 
> https://github.com/bywatersolutions/koha-plugin-curbside-pickup/pull/7
> 
> If that pull request holds up, the approach it outlines may well obviate any
> need for Koha itself to change. Consequently, I'm setting the bug status to
> in discussion for the moment.

That's great, Galen. Glad to see you submitting code!
Comment 20 David Cook 2020-08-27 06:18:08 UTC
(In reply to Galen Charlton from comment #18)
> If that pull request holds up, the approach it outlines may well obviate any
> need for Koha itself to change. Consequently, I'm setting the bug status to
> in discussion for the moment.

I reckon Galen's approach here is a more robust solution than relying on koha-plack to "do the right thing".

This approach could probably be streamlined a bit to reduce the boilerplate needed in each plugin as well.

+1 for the idea from me
Comment 21 Kyle M Hall 2020-08-27 11:30:47 UTC
(In reply to Galen Charlton from comment #18)
> The patch works for me with the curbside pickup plugin, and I'd be
> comfortable treating the kitchen sink plugin as not handling its interior
> modules as it should. Consequently, I've attached my signoff and tweaked the
> commit message a bit.
> 
> That said... this may not be necessary after all. I've worked out an
> approach to have plugins register their own custom DBIx result classes
> without requiring that koha-plack munge PERL5LIB in advance. See the pull
> request on the plugin here:
> 
> https://github.com/bywatersolutions/koha-plugin-curbside-pickup/pull/7
> 
> If that pull request holds up, the approach it outlines may well obviate any
> need for Koha itself to change. Consequently, I'm setting the bug status to
> in discussion for the moment.

This is fantastic Galen! Thanks! I've merged your pull requests into the plugin.

I am quite happy to close out this bug report, as Galen's solution is definitely the better one.