From the Readonly CPAN page: "Install this module, but do not use it. ... Programs that you write do not need to know whether Readonly::XS is installed or not. They should just "use Readonly" and let Readonly worry about whether or not it can use XS. If the Readonly::XS is present, Readonly will be faster. If not, it won't. Either way, it will still work, and your code will not have to change." This removes the Readonly::XS from the PerlDependencies.
Created attachment 71071 [details] [review] Bug 20103: Remove Readonly::XS from PerlDependencies While having Readonly::XS is a good thing on slower systems the CPAN page for Readonly says it isn't required to be used directly, so we don't need to check for it. Additionally, even reading /usr/share/perl5/Readonly.pm you see "# Modern perl doesn't need Readonly::XS" on an if statement checking for version 5.8! Koha requires 5.20 (see C4::Log::cronlogaction's use of caller). TEST PLAN --------- Can you run a fresh install? Does the about page list Readonly::XS? It shouldn't after the patch. Run koha qa test tools
Created attachment 72546 [details] [review] Bug 20103: Remove Readonly::XS from PerlDependencies While having Readonly::XS is a good thing on slower systems the CPAN page for Readonly says it isn't required to be used directly, so we don't need to check for it. Additionally, even reading /usr/share/perl5/Readonly.pm you see "# Modern perl doesn't need Readonly::XS" on an if statement checking for version 5.8! Koha requires 5.20 (see C4::Log::cronlogaction's use of caller). TEST PLAN --------- Can you run a fresh install? Does the about page list Readonly::XS? It shouldn't after the patch. Run koha qa test tools Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Combining SO+QA / trivial
I do not understand the quote like you. It says it is faster and we should not "use" it, in the sense we should not write use Readonly::XS; but use Readonly; instead. To me it makes sense to keep it in the dependency list (until Readonly is removed, bug 16588).
(In reply to Jonathan Druart from comment #4) > I do not understand the quote like you. > It says it is faster and we should not "use" it, in the sense we should not > write > use Readonly::XS; > but > use Readonly; > instead. > > To me it makes sense to keep it in the dependency list (until Readonly is > removed, bug 16588). From Readonly.pm 18 # These functions may be overridden by Readonly::XS, if installed. 19 use vars qw/$XSokay/; # Set to true in Readonly::XS, if available 20 21 # For perl 5.8.x or higher 22 # These functions are exposed in perl 5.8.x (Thanks, Leon!) 23 # They may be overridden by Readonly::XS, if installed on old perl versions 24 if ($] < 5.008) { # 'Classic' perl 25 *is_sv_readonly = sub ($) {0}; 26 *make_sv_readonly 27 = sub ($) { die "make_sv_readonly called but not overridden" }; 28 29 # See if we can use the XS stuff. 30 $Readonly::XS::MAGIC_COOKIE 31 = "Do NOT use or require Readonly::XS unless you're me."; 32 eval 'use Readonly::XS'; 33 } 34 else { # Modern perl doesn't need Readonly::XS 35 *is_sv_readonly = sub ($) { Internals::SvREADONLY($_[0]) }; 36 *make_sv_readonly 37 = sub ($) { Internals::SvREADONLY($_[0], 1) }; 38 $XSokay = 1; # We're using the new built-ins so this is a white lie 39 }
Created attachment 73406 [details] [review] Bug 20103: Readonly::XS is completely bogus post perl 5.8 This patch continues the whitelie that Readonly tells about $XSokay, so that we don't worry people who try to install everything. ./koha_perl_deps.pl -a -- before patch Readonly::XS is 0 -- after patch it's listed as 0.01 (the minimum version) run koha qa test tools
Created attachment 73407 [details] [review] Bug 20103: Readonly::XS is completely bogus post perl 5.8 A ha! Lie about who we are, and the module will return a version. ./koha_perl_deps.pl -a -- before patch Readonly::XS is 0 -- after patch it has a version number. run koha qa test tools
If we don't want to remove Readonly::XS from the dependencies until after we remove Readonly, then sign off patch 2 and rename this bug "Readonly::XS dies before returning $VERSION" or something similar, and please include the removal of Readonly::XS on the other bug. If we wish to remove Readonly::XS from the dependencies, because more modern versions of Perl (post 5.8) don't need it explicitly, as shown in comment #5, then sign off patch 1.
(In reply to M. Tompsett from comment #8) > please include the removal of Readonly::XS on the other bug. It is, nevermind. :)
Comment on attachment 72546 [details] [review] Bug 20103: Remove Readonly::XS from PerlDependencies Counter patch is better. :)
TEST PLAN --------- In your kohaclone directory run: ./koha_perl_deps.pl -a | grep Readonly::XS -- There will be a "0 *" shown for the installed version Apply the patch and repeat the above command. -- There will be a valid version number shown.
Created attachment 73879 [details] [review] Bug 20103: Readonly::XS is completely bogus post perl 5.8 A ha! Lie about who we are, and the module will return a version. ./koha_perl_deps.pl -a -- before patch Readonly::XS is 0 -- after patch it has a version number. run koha qa test tools Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
Created attachment 74695 [details] [review] Bug 20103: Readonly::XS is completely bogus post perl 5.8 A ha! Lie about who we are, and the module will return a version. ./koha_perl_deps.pl -a -- before patch Readonly::XS is 0 -- after patch it has a version number. run koha qa test tools Signed-off-by: Victor Grousset <victor.grousset@biblibre.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 18.05, thanks to everybody involved!