Bug 26848 - Fix Readonly dependency in cpanfile
Summary: Fix Readonly dependency in cpanfile
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-28 15:39 UTC by Julian Maurice
Modified: 2021-02-11 22:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.02,20.05.08


Attachments
Bug 26848: Fix Readonly dependency in cpanfile (1.47 KB, patch)
2020-10-28 15:40 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 26848: Add Readonly::XS 1.05 as required (732 bytes, patch)
2020-12-09 05:31 UTC, Mason James
Details | Diff | Splinter Review
Bug 26848: Fix Readonly dependency in cpanfile (1.55 KB, patch)
2020-12-09 09:29 UTC, Mason James
Details | Diff | Splinter Review
Bug 26848: Fix Readonly dependency in cpanfile (1.58 KB, patch)
2021-01-11 13:41 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2020-10-28 15:39:32 UTC

    
Comment 1 Julian Maurice 2020-10-28 15:40:16 UTC
Created attachment 112647 [details] [review]
Bug 26848: Fix Readonly dependency in cpanfile

- It is a required module. If it is missing, Koha will not start
- Set the minimum version to 2.00. While not strictly required, it's the
version that states that "Deprecation of Readonly::XS as a requirement
for fast, readonly scalars is complete", it is available in Debian
oldoldstable (jessie, 8), and Readonly::XS was not even needed with Perl
> 5.8, which allows us to:
- Remove dependency on Readonly::XS
Comment 2 Mason James 2020-12-09 05:31:19 UTC
Created attachment 114271 [details] [review]
Bug 26848: Add Readonly::XS 1.05 as required
Comment 3 Mason James 2020-12-09 06:20:49 UTC
hi Julian

i did a quick test with a modified eg/benchmark.pl script, which indicates that Readonly's performance is 10x with the Readonly::XS module installed

 https://metacpan.org/source/SANKO/Readonly-2.05/eg/benchmark.pl

so, i think it's still a good idea to install Readonly::XS - do you agree?


mason@xen1:~/g/cpan/readonly$ perl eg/benchmark2.pl
Benchmark: timing 10000000 iterations of rotie, rotie_xs,...
...
     rotie:  4 wallclock secs ( 4.19 usr +  0.00 sys =  4.19 CPU) @ 2386634.84/s (n=10000000)
  rotie_xs: -1 wallclock secs ( 0.38 usr +  0.00 sys =  0.38 CPU) @ 26315789.47/s (n=10000000)
Comment 4 Julian Maurice 2020-12-09 08:21:47 UTC
Hi Mason,

What did you modify in the benchmark ?
I ran the non-modified benchmark with and without Readonly::XS installed and I saw no differences.

Also, on https://metacpan.org/pod/Readonly#Internals it is written:
> In the years since Readonly::XS was released, the then private internal
> methods have been exposed and can be used in pure perl. Similar modules were
> written to take advantage of this and a patch to Readonly was created. We no
> longer need to build and install another module to make Readonly useful on
> modern builds of perl.
> - You do not need to install Readonly::XS.
> - You should stop listing Readonly::XS as a dependency or expect it to be installed.
> - Stop testing the $Readonly::XSokay variable!

So I think Readonly::XS should be removed from our cpanfile.
Comment 5 Mason James 2020-12-09 09:29:41 UTC
Created attachment 114274 [details] [review]
Bug 26848: Fix Readonly dependency in cpanfile

- It is a required module. If it is missing, Koha will not start
- Set the minimum version to 2.00. While not strictly required, it's the
version that states that "Deprecation of Readonly::XS as a requirement
for fast, readonly scalars is complete", it is available in Debian
oldoldstable (jessie, 8), and Readonly::XS was not even needed with Perl
> 5.8, which allows us to:
- Remove dependency on Readonly::XS

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 6 Mason James 2020-12-09 09:32:02 UTC
(In reply to Julian Maurice from comment #4)
> Hi Mason,
> 
> What did you modify in the benchmark ?
> I ran the non-modified benchmark with and without Readonly::XS installed and
> I saw no differences.
> 
> So I think Readonly::XS should be removed from our cpanfile.

hi Julian, 
i tested again and agree, so i will sign-off :)
Comment 7 Martin Renvoize 2021-01-11 13:41:08 UTC
Created attachment 115027 [details] [review]
Bug 26848: Fix Readonly dependency in cpanfile

- It is a required module. If it is missing, Koha will not start
- Set the minimum version to 2.00. While not strictly required, it's the
version that states that "Deprecation of Readonly::XS as a requirement
for fast, readonly scalars is complete", it is available in Debian
oldoldstable (jessie, 8), and Readonly::XS was not even needed with Perl
> 5.8, which allows us to:
- Remove dependency on Readonly::XS

Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2021-01-11 13:41:40 UTC
Trivial fix.. also agree on the benchmark.. I can't replicate a speedup with ::XS.

Passing QA
Comment 9 Jonathan Druart 2021-01-12 10:43:02 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 10 Fridolin Somers 2021-01-14 16:18:54 UTC
Pushed to 20.11.x for 20.11.02
Comment 11 Andrew Fuerste-Henry 2021-01-20 01:56:39 UTC
Pushed to 20.05.x for 20.05.08
Comment 12 Victor Grousset/tuxayo 2021-01-22 19:54:52 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.