Bug 20103 - Readonly::XS version not detected
Summary: Readonly::XS version not detected
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Mark Tompsett
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-30 18:21 UTC by Mark Tompsett
Modified: 2022-05-12 07:02 UTC (History)
7 users (show)

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


Attachments
Bug 20103: Remove Readonly::XS from PerlDependencies (1.32 KB, patch)
2018-01-30 18:30 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20103: Remove Readonly::XS from PerlDependencies (1.42 KB, patch)
2018-03-09 06:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20103: Readonly::XS is completely bogus post perl 5.8 (1.64 KB, patch)
2018-03-28 22:49 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20103: Readonly::XS is completely bogus post perl 5.8 (1.03 KB, patch)
2018-03-28 22:59 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20103: Readonly::XS is completely bogus post perl 5.8 (1.09 KB, patch)
2018-04-09 16:02 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 20103: Readonly::XS is completely bogus post perl 5.8 (1.15 KB, patch)
2018-04-22 16:14 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2018-01-30 18:21:34 UTC
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.
Comment 1 Mark Tompsett 2018-01-30 18:30:17 UTC
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
Comment 2 Marcel de Rooy 2018-03-09 06:58:39 UTC
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>
Comment 3 Marcel de Rooy 2018-03-09 06:59:08 UTC
Combining SO+QA / trivial
Comment 4 Jonathan Druart 2018-03-28 19:02:16 UTC
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).
Comment 5 Mark Tompsett 2018-03-28 22:38:52 UTC
(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 }
Comment 6 Mark Tompsett 2018-03-28 22:49:31 UTC
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
Comment 7 Mark Tompsett 2018-03-28 22:59:02 UTC
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
Comment 8 Mark Tompsett 2018-03-28 23:46:07 UTC
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.
Comment 9 Mark Tompsett 2018-03-28 23:47:38 UTC
(In reply to M. Tompsett from comment #8)
> please include the removal of Readonly::XS on the other bug.

It is, nevermind. :)
Comment 10 Mark Tompsett 2018-04-05 02:02:02 UTC
Comment on attachment 72546 [details] [review]
Bug 20103: Remove Readonly::XS from PerlDependencies

Counter patch is better. :)
Comment 11 Mark Tompsett 2018-04-09 14:28:35 UTC
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.
Comment 12 Victor Grousset/tuxayo 2018-04-09 16:02:21 UTC
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>
Comment 13 Katrin Fischer 2018-04-22 16:14:10 UTC
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>
Comment 14 Jonathan Druart 2018-05-03 16:40:59 UTC
Pushed to master for 18.05, thanks to everybody involved!