Bug 17880

Summary: C4::Installer::PerlModules lexicographical comparison is incorrect
Product: Koha Reporter: David Cook <dcook>
Component: Installation and upgrade (command-line installer)Assignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: katrin.fischer, kyle, m.de.rooy, mtj, mtompset, ohiocore, vanoudt
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17986
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17986    
Attachments: Bug 17880 - Add test to check version number comparison
Bug 17880 - Use version.pm to parse version numbers in C4::Installer::PerlModules
Bug 17880 - Add test to check version number comparison
Bug 17880 - Use version.pm to parse version numbers in C4::Installer::PerlModules
Bug 17880 - Add test to check version number comparison
Bug 17880 - Use version.pm to parse version numbers in C4::Installer::PerlModules
Fix copy/paste error
Bug 17880: Fix copy/paste error

Description David Cook 2017-01-11 05:15:44 UTC
C4::Installer::PerlModules compares a modules VERSION against the min_version specified in C4::Installer::PerlDependencies using the lexicographical comparison "lt". 

This means that 0.9.12 will be considered to be less than 0.9.3:

PDF::Table                                    0.9.12            0.9.3                   Yes

The above line shows up in koha_perl_deps.pl when I look to see what modules I need to upgrade. 

It doesn't prevent me from installing/running Koha, but it's wrong. 

In theory, we could use version.pm for our comparisons (http://search.cpan.org/~jpeacock/version-0.9917/lib/version.pod#How_to_compare_version_objects). I think it can sometimes have surprising results, although in this case it should be OK since we're comparing version numbers against version numbers of the same module. So in theory it should have a consistent scheme...
Comment 1 David Cook 2017-01-11 06:10:43 UTC
I've actually written a patch for this, but I found another bug in C4::Installer::PerlModules::version_info() while writing a unit test, so I'll open another bug for that...
Comment 2 David Cook 2017-01-11 23:32:53 UTC
Removed the dependency on bug 17882 as they can actually be done separately...
Comment 3 David Cook 2017-01-11 23:33:19 UTC
(In reply to David Cook from comment #2)
> Removed the dependency on bug 17882 as they can actually be done
> separately...

Although whichever one comes second will need to be rebased...
Comment 4 David Cook 2017-01-12 00:18:21 UTC
Created attachment 58857 [details] [review]
Bug 17880 - Add test to check version number comparison
Comment 5 David Cook 2017-01-12 00:18:27 UTC
Created attachment 58858 [details] [review]
Bug 17880 - Use version.pm to parse version numbers in C4::Installer::PerlModules
Comment 6 David Cook 2017-01-12 00:18:46 UTC
Test Plan:

1) Apply patch "Bug 17880 - Add test to check version number comparison"
2) Run "perl t/Installer_PerlModules.t"
3) Note the following output:

not ok 19 - Version 0.9.13 is greater than 0.9.3, so no upgrade needed

#   Failed test 'Version 0.9.13 is greater than 0.9.3, so no upgrade needed'
#   at t/Installer_PerlModules.t line 51.
#          got: '1'
#     expected: '0'
# Looks like you failed 1 test of 19.

4) Apply patch "Bug 17880 - Use version.pm to parse version numbers in C4::Installer::PerlModules"
5) Run "perl t/Installer_PerlModules.t"
6) Note the following output:

ok 19 - Version 0.9.13 is greater than 0.9.3, so no upgrade needed
Comment 7 Mark Tompsett 2017-01-16 02:46:32 UTC
Created attachment 58998 [details] [review]
Bug 17880 - Add test to check version number comparison

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 8 Mark Tompsett 2017-01-16 02:46:36 UTC
Created attachment 58999 [details] [review]
Bug 17880 - Use version.pm to parse version numbers in C4::Installer::PerlModules

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 9 Mark Tompsett 2017-01-16 02:47:59 UTC
Oops. Didn't notice the Academy until after I signed off. Thanks for fixing this. I remember hitting this problem with some actual Koha stuff while playing between OS's and versions.
Comment 10 Jonathan Druart 2017-01-17 09:23:11 UTC
Created attachment 59074 [details] [review]
Bug 17880 - Add test to check version number comparison

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2017-01-17 09:23:15 UTC
Created attachment 59075 [details] [review]
Bug 17880 - Use version.pm to parse version numbers in C4::Installer::PerlModules

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2017-01-17 09:23:33 UTC
*** Bug 5599 has been marked as a duplicate of this bug. ***
Comment 13 Jonathan Druart 2017-01-17 09:23:40 UTC
*** Bug 16638 has been marked as a duplicate of this bug. ***
Comment 14 Kyle M Hall 2017-01-20 13:47:43 UTC
Pushed to master for 17.05, thanks David!
Comment 15 Katrin Fischer 2017-01-22 19:56:03 UTC
These patches have been pushed to 16.11.x and will be in 16.11.02.
.
Comment 16 David Cook 2017-01-24 23:02:24 UTC
Ooops... it looks like I actually created a new bug while fixing that bug!

The test works but I made a copy/paste error which means koha_perl_deps.pl now doesn't work correctly! I'm just about to attach a patch to fix that.
Comment 17 David Cook 2017-01-24 23:03:21 UTC
Created attachment 59533 [details] [review]
Fix copy/paste error

It looks like I made a copy/paste error in a previous patch.

While the fix was working when you pass the param "module" to
version_info, it wasn't populating the version correctly
for the "all" param, which causes koha_perl_deps.pl to
think all OK modules actually need an upgrade.

TEST PLAN

0) Be on a system where you know your Koha Perl dependencies are
mostly up-to-date

1) Run ./koha_perl_deps.pl -a -c
2) Note that most modules say they need an upgrade even when
the installed version is the same as the minimum version

3) Apply patch

4) Run ./koha_perl_deps.pl -a -c
5) Note that most moduls say they're OK, especially when the
installed version is the same or greater than the minimum version
Comment 18 David Cook 2017-01-24 23:06:51 UTC
Created attachment 59534 [details] [review]
Bug 17880: Fix copy/paste error

It looks like I made a copy/paste error in a previous patch.

While the fix was working when you pass the param "module" to
version_info, it wasn't populating the version correctly
for the "all" param, which causes koha_perl_deps.pl to
think all OK modules actually need an upgrade.

TEST PLAN

0) Be on a system where you know your Koha Perl dependencies are
mostly up-to-date

1) Run ./koha_perl_deps.pl -a -c
2) Note that most modules say they need an upgrade even when
the installed version is the same as the minimum version

3) Apply patch

4) Run ./koha_perl_deps.pl -a -c
5) Note that most moduls say they're OK, especially when the
installed version is the same or greater than the minimum version
Comment 19 David Cook 2017-01-24 23:28:20 UTC
See Bug 17986 for a separate bug report and fix
Comment 20 Marcel de Rooy 2017-01-25 10:43:03 UTC
Comment on attachment 59534 [details] [review]
Bug 17880: Fix copy/paste error

Deleting this patch here, since it is on a new report
Comment 21 Mason James 2017-01-27 03:22:43 UTC
Pushed to 16.05.x, for 16.05.08 release
Comment 22 Mason James 2017-01-27 03:46:28 UTC
(In reply to Mason James from comment #21)
> Pushed to 16.05.x, for 16.05.08 release

Blocked by BZ-17986, skipping for 16.05.08
Comment 23 Mason James 2017-01-31 09:28:25 UTC
Pushed to 16.05.x, for 16.05.09 release