Summary: | Add a 2nd directory for Perl modules | ||
---|---|---|---|
Product: | Koha | Reporter: | David Cook <dcook> |
Component: | Architecture, internals, and plumbing | Assignee: | 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
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. 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 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> 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. (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. The main question to me is: is it safe enough for backport into 21.05.01? (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. (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 (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 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 (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. 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. (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). (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? (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 Thanks for the explanations :D Skipping QA as we need to move forward (needed for 21.05.01). Pushed to master for 21.11.00 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 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 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> Follow-up pushed to master. (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. Pushed to 21.05.x for 21.05.01 (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. 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 ?
Depends on Bug 28489 not in 20.11.x (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? (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? (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. 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). (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. 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 ? * 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. (discussion should continue on bug 17427) (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 (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... 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 |