Created attachment 99299 [details] [review] Bug 24693: Move GD from recommends to requires in cpanfile Without it, catalogue/detail.pl dies (and probably other scripts too) Test plan: 1. Uninstall GD, reload starman 2. Go to catalogue/detail.pl. Confirm that it dies 3. Reinstall GD, reload starman 4. Go to catalogue/detail.pl. Confirm that it work normally
Created attachment 99475 [details] [review] Bug 24693: Move GD from recommends to requires in cpanfile Without it, catalogue/detail.pl dies (and probably other scripts too) Test plan: 1. Uninstall GD, reload starman 2. Go to catalogue/detail.pl. Confirm that it dies 3. Reinstall GD, reload starman 4. Go to catalogue/detail.pl. Confirm that it work normally Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> catalogue/detail.pl dies without GD. Seems correct switch to require No errors
It does not die for me, it dies if I enable LocalCoverImages, then try to upload an image.
Maybe we should clarify what does *required* mean for us. Personally I believe that every Perl module that we directly `use` or `require` without checking its presence should be flagged as required. Even if the require is behind a syspref check or something like that. Put more simply, if Koha dies because of a missing Perl module, that module is a required dependency. Maybe later we can build something based on cpanfile's `feature` (https://metacpan.org/pod/distribution/Module-CPANfile/lib/cpanfile.pod#feature) to warn when an optional feature is enabled and the required modules for this feature are missing.
(In reply to Julian Maurice from comment #4) > Maybe we should clarify what does *required* mean for us. > > Personally I believe that every Perl module that we directly `use` or > `require` without checking its presence should be flagged as required. Even > if the require is behind a syspref check or something like that. Put more > simply, if Koha dies because of a missing Perl module, that module is a > required dependency. > > Maybe later we can build something based on cpanfile's `feature` > (https://metacpan.org/pod/distribution/Module-CPANfile/lib/cpanfile. > pod#feature) to warn when an optional feature is enabled and the required > modules for this feature are missing. The whole 'feature' stuff from the cpanfile move came up earlier that I was expecting.. but I suspected it would raise it's head at some point. I'm really not sure what way to fall on this one.. I don't like that changing a syspref can completely 'break', i.e lead to a die, koha.. but at the same time if a set of functionality is optional, the dependencies it requires really aught to be too.
(In reply to Jonathan Druart from comment #3) > It does not die for me, it dies if I enable LocalCoverImages, then try to > upload an image. That was wrong, I removed libgd-perl, that removed koha-common. The alias restart_all got broken, as it did not correctly restart plack, etc. So yes, detail.pl crashes with GD.
Created attachment 99510 [details] [review] Bug 24693: Move GD from recommends to requires in cpanfile Without it, catalogue/detail.pl dies (and probably other scripts too) Test plan: 1. Uninstall GD, reload starman 2. Go to catalogue/detail.pl. Confirm that it dies 3. Reinstall GD, reload starman 4. Go to catalogue/detail.pl. Confirm that it work normally Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> catalogue/detail.pl dies without GD. Seems correct switch to require No errors Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
We will make it (really) optional if there is a real need for that.
Nice work everyone! Pushed to master for 20.05
Does not apply to 19.11.x branch. Not backported