Bug 28519

Summary: Add a 2nd directory for Perl modules
Product: Koha Reporter: David Cook <dcook>
Component: Architecture, internals, and plumbingAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, julian.maurice, kyle, m.de.rooy, nugged, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28617
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.01
Bug Depends on: 28489    
Bug Blocks: 17427, 28589    
Attachments: Bug 28519: Put CGI::Session::Serialize::yamlxs in lib directory
Bug 28519: Put CGI::Session::Serialize::yamlxs in lib directory
Bug 28519: Add lib to koha-shell
Bug 28519: Add lib to koha-shell

Description David Cook 2021-06-07 01:16:47 UTC
Bug 28489 looks to add CGI::Session::Serializer::yamlxs, but it wants to hack it into C4::Auth.

I rather we put it in a Perl "lib" directory. However, /usr/share/koha/lib only exists in a deployed Koha. There is no "lib" directory in the source tree. But we can fix that.
Comment 1 David Cook 2021-06-07 02:23:49 UTC
Created attachment 121649 [details] [review]
Bug 28519: Put CGI::Session::Serialize::yamlxs in lib directory

This patch adds a "lib" directory to the source tree which gets
mapped to the same directory as "C4" and "Koha" for single and
standard installations.

CGI::Session::Serialize::yamlxs is put into this "lib" directory.

This patch also includes some changes so that dev/git installations
work as well.
Comment 2 David Cook 2021-06-07 02:24:07 UTC
Dev test plan:

1. vi /etc/apache2/sites-enabled/kohadev.conf
2. Change "SetEnv PERL5LIB "/kohadevbox/koha" to SetEnv PERL5LIB "/kohadevbox/koha:/kohadevbox/koha/lib"
2b. service apache2 reload
3. cp debian/scripts/koha-functions.sh /usr/share/koha/bin/koha-functions.sh
4. koha-plack --restart kohadev
5. Log into http://localhost:8081/cgi-bin/koha/mainpage.pl
6. Go to CGI script http://localhost:8081/cgi-bin/koha/tools/export.pl 
(Note that this is still run in CGI even when using Plack)

NOTE: We'll need to update koha-gitify (https://gitlab.com/koha-community/koha-gitify) to get that Apache configuration. There's no harm in updating it, since this is a backwards compatible change. See https://gitlab.com/koha-community/koha-gitify/-/merge_requests/2. 

--

Deploy test plan:

Dev:
1. perl Makefile.PL
1a. Installation mode: dev
1b. Configuration directory: /opt/koha-dev
2. make
3. ls blib/PERL_MODULE_LIB_DIR/
4. Note the following: CGI
5. make install
5b. Note the output says the following:
In order to use Koha's command-line batch jobs,
you should set the following environment variables:

export KOHA_CONF=/opt/koha-dev/etc/koha-conf.xml
export PERL5LIB=/kohadevbox/koha:/kohadevbox/koha/lib
6a. vi /opt/koha-dev/etc/koha-httpd.conf
6b. Note the following: SetEnv PERL5LIB "/kohadevbox/koha:/kohadevbox/koha/lib"
7a. vi /opt/koha-dev/etc/koha-worker.service
7b. Note the following: Environment=PERL5LIB=/kohadevbox/koha:/kohadevbox/koha/lib
8a. vi /opt/koha-dev/bin/koha-index-daemon-ctl.sh
8b. Note the following: PERL5LIB="/kohadevbox/koha:/kohadevbox/koha/lib"
9a. vi /opt/koha-dev/misc/koha-install-log
9b. Note the following:
PERL_MODULE_LIB_DIR=/kohadevbox/koha/lib
PERL5LIB_DIRS=/kohadevbox/koha:/kohadevbox/koha/lib
6. rm -rf /opt/koha-dev
7. make clean

Single:
1. perl Makefile.PL
1a. Installation mode: single
1b. Please specify the directory in which to install Koha: /opt/koha
2. make
3. ls blib/PERL_MODULE_LIB_DIR/
4. Note the following: CGI
5. make install
5b. Note the output says the following:
In order to use Koha's command-line batch jobs,
you should set the following environment variables:

export KOHA_CONF=/opt/koha/etc/koha-conf.xml
export PERL5LIB=/opt/koha/lib
6a. vi /opt/koha/etc/koha-httpd.conf
6b. Note the following: SetEnv PERL5LIB "/opt/koha/lib"
7a. vi /opt/koha/etc/koha-worker.service
7b. Note the following: Environment=PERL5LIB=/opt/koha/lib
8a. vi /opt/koha/bin/koha-index-daemon-ctl.sh
8b. Note the following: PERL5LIB="/opt/koha/lib"
9a. vi /opt/koha/misc/koha-install-log
9b. Note the following:
PERL_MODULE_LIB_DIR=/opt/koha/lib
PERL5LIB_DIRS=/opt/koha/lib
10. ls /opt/koha/lib
11. Note the following: C4  CGI  cpanfile  Koha  Koha.pm
12. rm -rf /opt/koha
13. make clean

Standard:
1. perl Makefile.PL
1a. accept all the defaults
2. make
3. ls blib/PERL_MODULE_LIB_DIR/
4. Note the following: CGI
5. make install
5b. Note there is a set_koha_ownership failure because of a missing "koha" user, but that's OK since this is an overly intensive process within koha-testing-docker anyway, and the install has already succeeded at this point.
6a. vi /etc/koha/koha-httpd.conf
6b. Note the following: SetEnv PERL5LIB "/usr/share/koha/lib"
7a. vi /etc/koha/koha-worker.service
7b. Note the following: Environment=PERL5LIB=/usr/share/koha/lib
8a. vi /usr/share/koha/bin/koha-index-daemon-ctl.sh
8b. Note the following: PERL5LIB="/usr/share/koha/lib"
9a. vi /usr/share/koha/misc/koha-install-log
9b. Note the following:
PERL_MODULE_LIB_DIR=/usr/share/koha/lib
PERL5LIB_DIRS=/usr/share/koha/lib
10. ls /usr/share/koha/lib
11. Note the following: C4  CGI  cpanfile  Koha  Koha.pm
12. make clean
Comment 3 Nick Clemens 2021-06-11 13:13:39 UTC
Created attachment 121836 [details] [review]
Bug 28519: Put CGI::Session::Serialize::yamlxs in lib directory

This patch adds a "lib" directory to the source tree which gets
mapped to the same directory as "C4" and "Koha" for single and
standard installations.

CGI::Session::Serialize::yamlxs is put into this "lib" directory.

This patch also includes some changes so that dev/git installations
work as well.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Tomás Cohen Arazi 2021-06-11 20:26:58 UTC
I agree, generally. but:
1. You are adding an unadvertised die in C4::Auth (I agree with it!). Please add a test for that behavior.
2. I think you should simplify your patch by just putting the CGI/ dir in the top level. In that case, you would just do what we do for C4, Koha and the OpenILS library directories in Makefile.PL (& friends). No complex hacks.

I would love to move all libs to a libs/ dir, but lets do it on a separate bug, and discuss the tradeoffs there (my butt has been kicked a couple times when I proposed it, because of all bugzilla patches not applying if we did that).

If you need me to write a follow-up to address my comments, let me know.
Comment 5 Jonathan Druart 2021-06-14 14:04:54 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> I agree, generally. but:
> 1. You are adding an unadvertised die in C4::Auth (I agree with it!). Please
> add a test for that behavior.

This die is obvious (I added it when I wrote a patch for bug 17427 more than 4y ago - https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56504).
Writing a test for it is not trivial and I would not block the whole change for that.

> 2. I think you should simplify your patch by just putting the CGI/ dir in
> the top level. In that case, you would just do what we do for C4, Koha and
> the OpenILS library directories in Makefile.PL (& friends). No complex hacks.

I think it makes sense to have it in lib.

> I would love to move all libs to a libs/ dir, but lets do it on a separate
> bug, and discuss the tradeoffs there (my butt has been kicked a couple times
> when I proposed it, because of all bugzilla patches not applying if we did
> that).

Like having intranet controller scripts in their own directory (bug 9949). Could be part of a roadmap topic, but definitely not something for 21.11.
Comment 6 Jonathan Druart 2021-06-14 14:05:24 UTC
The main question to me is: is it safe enough for backport into 21.05.01?
Comment 7 Tomás Cohen Arazi 2021-06-14 14:24:06 UTC
(In reply to Jonathan Druart from comment #6)
> The main question to me is: is it safe enough for backport into 21.05.01?

I think it is. I would still skip the lib/ dir for now. But have no problem other than that with the patch.
Comment 8 Jonathan Druart 2021-06-14 14:41:59 UTC
(In reply to Tomás Cohen Arazi from comment #7)
> (In reply to Jonathan Druart from comment #6)
> > The main question to me is: is it safe enough for backport into 21.05.01?
> 
> I think it is. I would still skip the lib/ dir for now. But have no problem
> other than that with the patch.

My question was about the lib dir :D
Comment 9 David Cook 2021-06-14 23:35:30 UTC
(In reply to Jonathan Druart from comment #6)
> The main question to me is: is it safe enough for backport into 21.05.01?

I would say yes especially for Debian package installations. 

Dev environments are the ones that will experience the most changes, but we can deal with any pain there. 

Once this change is in place, Koha will be much more powerful.
Comment 10 Julian Maurice 2021-06-15 09:04:34 UTC
Comment on attachment 121836 [details] [review]
Bug 28519: Put CGI::Session::Serialize::yamlxs in lib directory

Review of attachment 121836 [details] [review]:
-----------------------------------------------------------------

::: Makefile.PL
@@ +1804,5 @@
> +            if ($path){
> +                $unique_map{$path} = 1 unless $unique_map{$path};
> +            }
> +        }
> +        my @paths = sort keys %unique_map;

Just a little suggestion: you can use uniq from List::MoreUtils to make the code smaller and easier to read
Comment 11 David Cook 2021-06-16 03:31:59 UTC
(In reply to Julian Maurice from comment #10)
> Just a little suggestion: you can use uniq from List::MoreUtils to make the
> code smaller and easier to read

I don't see how using uniq would make it smaller or easier to read.

I also don't like the idea of adding a non-core dependency to Makefile.PL for such a simple task.

Further, I don't like List::MoreUtils.
Comment 12 Julian Maurice 2021-06-16 06:54:54 UTC
You can replace

    foreach my $code (@dir_codes){
        my $path = $target_dirs->{ $code };
        if ($path){
            $unique_map{$path} = 1 unless $unique_map{$path};
        }
    }
    my @paths = sort keys %unique_map;

by

    my @paths = sort uniq map { $target_dirs->{$_} || () } @dir_codes;

and if you don't like List::MoreUtils or non-core dependencies, List::Util also has a uniq subroutine.

But that was only a suggestion.
Comment 13 Jonathan Druart 2021-06-16 09:47:27 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> I would love to move all libs to a libs/ dir, but lets do it on a separate
> bug, and discuss the tradeoffs there (my butt has been kicked a couple times
> when I proposed it, because of all bugzilla patches not applying if we did
> that).

Quick remark: this is not true. Git handles that (ie. if a file has been "git mv", a "git am" will follow the move).
Comment 14 Victor Grousset/tuxayo 2021-06-16 23:31:34 UTC
(In reply to David Cook from comment #9)
> (In reply to Jonathan Druart from comment #6)
> > The main question to me is: is it safe enough for backport into 21.05.01?
> 
> I would say yes especially for Debian package installations. 
> 
> Dev environments are the ones that will experience the most changes, but we
> can deal with any pain there. 
> 
> Once this change is in place, Koha will be much more powerful.

There are more than 200 production instances based on git, which changes will they experience?
Comment 15 David Cook 2021-06-16 23:45:11 UTC
(In reply to Victor Grousset/tuxayo from comment #14)
> (In reply to David Cook from comment #9)
> > (In reply to Jonathan Druart from comment #6)
> > > The main question to me is: is it safe enough for backport into 21.05.01?
> > 
> > I would say yes especially for Debian package installations. 
> > 
> > Dev environments are the ones that will experience the most changes, but we
> > can deal with any pain there. 
> > 
> > Once this change is in place, Koha will be much more powerful.
> 
> There are more than 200 production instances based on git, which changes
> will they experience?

It depends on how they do their deployments. If they use symlinks for things like their Apache configuration, then it should be automagical after a "make upgrade". But if they don't, then they'll need to manually update their Apache configuration. (Changes to koha-worker.service and koha-index-daemon-ctl.sh would matter less in the short-term but they would need updates too.)

People not using Debian packages are always going to be vulnerable to Apache configuration changes though.

I'll note that my PR for koha-gitify needs to be merged in too for users of koha-gitify (like koha-testing-docker): https://gitlab.com/koha-community/koha-gitify/-/merge_requests/2
Comment 16 Victor Grousset/tuxayo 2021-06-17 00:22:41 UTC
Thanks for the explanations :D
Comment 17 Jonathan Druart 2021-06-17 08:12:29 UTC
Skipping QA as we need to move forward (needed for 21.05.01).
Comment 18 Jonathan Druart 2021-06-17 11:36:43 UTC
Pushed to master for 21.11.00
Comment 19 Jonathan Druart 2021-06-17 11:37:19 UTC
There is change missing in koha-shell

root@kohadevbox:koha$ echo $PERL5LIB
/kohadevbox/koha:/kohadevbox/koha/lib:/kohadevbox/qa-test-tools
root@kohadevbox:koha$ koha-shell kohadev
kohadev-koha@kohadevbox:/kohadevbox/koha$ echo $PERL5LIB
/kohadevbox/koha:/kohadevbox/qa-test-tools
Comment 20 Jonathan Druart 2021-06-17 13:43:44 UTC
Created attachment 122077 [details] [review]
Bug 28519: Add lib to koha-shell

Test plan:
% koha-shell kohadev -c 'echo $PERL5LIB'
=> /kohadevbox/koha:/kohadevbox/koha/lib

% koha-shell kohadev
echo $PERL5LIB
=> /kohadevbox/koha:/kohadevbox/koha/lib:/kohadevbox/qa-test-tools

Note that this is not consistent but at least we have lib now in both
situations
Comment 21 Tomás Cohen Arazi 2021-06-17 13:49:25 UTC
Created attachment 122079 [details] [review]
Bug 28519: Add lib to koha-shell

Test plan:
% koha-shell kohadev -c 'echo $PERL5LIB'
=> /kohadevbox/koha:/kohadevbox/koha/lib

% koha-shell kohadev
echo $PERL5LIB
=> /kohadevbox/koha:/kohadevbox/koha/lib:/kohadevbox/qa-test-tools

Note that this is not consistent but at least we have lib now in both
situations

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Jonathan Druart 2021-06-17 14:03:09 UTC
Follow-up pushed to master.
Comment 23 Marcel de Rooy 2021-06-18 08:42:38 UTC
(In reply to David Cook from comment #0)
> Bug 28489 looks to add CGI::Session::Serializer::yamlxs, but it wants to
> hack it into C4::Auth.
> 
> I rather we put it in a Perl "lib" directory. However, /usr/share/koha/lib
> only exists in a deployed Koha. There is no "lib" directory in the source
> tree. But we can fix that.

Not opposing this change, but isnt this a hack too? We could have fooled Koha too by making our own package and install it in a regular perllib ? Just curious why we exactly need lib.
Comment 24 Kyle M Hall 2021-06-18 15:40:17 UTC
Pushed to 21.05.x for 21.05.01
Comment 25 David Cook 2021-06-21 04:04:51 UTC
(In reply to Marcel de Rooy from comment #23)
> (In reply to David Cook from comment #0)
> > Bug 28489 looks to add CGI::Session::Serializer::yamlxs, but it wants to
> > hack it into C4::Auth.
> > 
> > I rather we put it in a Perl "lib" directory. However, /usr/share/koha/lib
> > only exists in a deployed Koha. There is no "lib" directory in the source
> > tree. But we can fix that.
> 
> Not opposing this change, but isnt this a hack too? We could have fooled
> Koha too by making our own package and install it in a regular perllib ?
> Just curious why we exactly need lib.

It's the opposite of a hack. C4 and Koha should never have been top-level directories in the source tree. We should always have had a "lib" top-level directory in which to place modules. This change is about creating a better standard and using it.
Comment 26 Marcel de Rooy 2021-06-21 09:52:48 UTC
Hi David,
I understand 'We do not have a lib folder' point.
But still wondering (especially since this report just creates a CGI::Session variant in lib): 

> We could have fooled Koha too by making our own package and install it in a
> regular perllib ?
Comment 27 Fridolin Somers 2021-06-21 13:45:19 UTC
Depends on Bug 28489 not in 20.11.x
Comment 28 David Cook 2021-06-21 23:13:31 UTC
(In reply to Marcel de Rooy from comment #26)
> Hi David,
> I understand 'We do not have a lib folder' point.
> But still wondering (especially since this report just creates a
> CGI::Session variant in lib): 
> 
> > We could have fooled Koha too by making our own package and install it in a
> > regular perllib ?

I still don't know what you mean by "we could have fooled Koha", or "our own package", or "regular perllib". Do you mean a Perl package or a Debian package? What do you mean by "perllib"? Do you mean PERL5LIB, site Perl, vendor Perl, Koha's top-level directory where we store C4 and Koha dirs, or something else? How would it be fooling Koha?
Comment 29 Kyle M Hall 2021-06-22 12:37:51 UTC
(In reply to David Cook from comment #28)
> (In reply to Marcel de Rooy from comment #26)
> > Hi David,
> > I understand 'We do not have a lib folder' point.
> > But still wondering (especially since this report just creates a
> > CGI::Session variant in lib): 
> > 
> > > We could have fooled Koha too by making our own package and install it in a
> > > regular perllib ?
> 
> I still don't know what you mean by "we could have fooled Koha", or "our own
> package", or "regular perllib". Do you mean a Perl package or a Debian
> package? What do you mean by "perllib"? Do you mean PERL5LIB, site Perl,
> vendor Perl, Koha's top-level directory where we store C4 and Koha dirs, or
> something else? How would it be fooling Koha?

I think he means having our own debian package for CGI::Session::Serialize::yamlxs that would be installed to /usr/share/perl5/CGI/Session/Serialize/yamlxs.pm

This seems like a bad thing to do though, as it would possibly break other Perl applications, right?
Comment 30 Marcel de Rooy 2021-06-22 12:48:52 UTC
(In reply to Kyle M Hall from comment #29)
> I think he means having our own debian package for
> CGI::Session::Serialize::yamlxs that would be installed to
> /usr/share/perl5/CGI/Session/Serialize/yamlxs.pm
> 
> This seems like a bad thing to do though, as it would possibly break other
> Perl applications, right?

Hi Kyle,
Right. Not that hard. This should not break other Perl apps as it should not break Koha :) We put it in our PERL5LIB path!
Note that if you do not set the serializer parameter to this new one, you probably wont ever notice it is there.
And when it is only in a specific Koha container, even more.
Comment 31 Jonathan Druart 2021-06-22 12:57:13 UTC
Not directly related to the discussion, but see also bug 17427 comment 29.
When we will move (finally) to Data::Session we will hit a problem with the package not in /usr/share/perl5/CGI/Session/Serialize/ (not along with Serialize.pm at least).
Comment 32 David Cook 2021-06-22 23:35:54 UTC
(In reply to Jonathan Druart from comment #31)
> Not directly related to the discussion, but see also bug 17427 comment 29.
> When we will move (finally) to Data::Session we will hit a problem with the
> package not in /usr/share/perl5/CGI/Session/Serialize/ (not along with
> Serialize.pm at least).

That's a good point (although wouldn't it be "/usr/share/perl5/Data/Session/Serialize"?). That said, I feel like this is an argument against moving to Data::Session. That get_my_serializers function is a dreadful bit of code. 

I'm not seeing any packages for Data::Session in Debian or Ubuntu and it seems like it has a lot of dependencies. Also, Ron Savage took over CGI::Session and self-declared that his Data::Session was the successor to CGI::Session. Do we have any evidence that anyone actually uses it?

I'd be tempted to either use Plack::Middleware::Session and finally put the nail in CGI, or write our own Koha::Session module. It doesn't need to be very complicated.
Comment 33 Julian Maurice 2021-06-23 06:42:59 UTC
I think I agree with David here: writing our own Koha::Session should be relatively easy, and it would remove the need of a separate {CGI|Data}::Session::Serialize::yamlxs. And having less dependencies is a good thing IMO.

But using Plack::Middleware::Session would prevent using koha with mojolicious servers like morbo or hypnotoad, right ?
Comment 34 Jonathan Druart 2021-06-23 08:44:46 UTC
* Mason already packaged Data::Session (on bug 17427)
* CGI::Session is no longer maintained and has a bad bug (see bug 17427 comment 0)
* Patch is there on bug 17427 (not fully working but close), when we are talking about an hypothetical rewrite
* Less dependency but more code to maintain on our side.
* Getting such feedback after the 3+ years 17427 has been written is a bit frustrating...

That being said I would support you if you are volunteering for an alternative (writing Koha::Session or using Plack::Middleware::Session), I will test, provide feedback and make it ready for 21.11.

But it needs to be top priority for you.
Comment 35 Jonathan Druart 2021-06-23 08:45:07 UTC
(discussion should continue on bug 17427)
Comment 36 David Cook 2021-06-25 04:40:20 UTC
(In reply to Julian Maurice from comment #33)
> But using Plack::Middleware::Session would prevent using koha with
> mojolicious servers like morbo or hypnotoad, right ?

Probably
Comment 37 David Cook 2021-06-25 04:46:04 UTC
(In reply to Jonathan Druart from comment #34)
> But it needs to be top priority for you.

Fair enough. I have very little to give to Koha at the moment...
Comment 38 Victor Grousset/tuxayo 2021-06-29 09:10:22 UTC
I guess this should go in the technical release notes?

Here is a first try:

### Perl modules
A new lib directory has been added for Perl modules.
This might requires some changes in your webserver configuration if you don't install Koha via the Debian packages. Here are more info about these changes
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28519#c2
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28519#c15

This ticket says: «Pushed to 21.05.x for 21.05.01»
Shouldn't it be also in the 21.05 tech release notes? I guess they are published and immutable?

https://annuel.framapad.org/p/koha_21.11_tech_release_notes