Bug 17986 - Perl dependency evaluation incorrect
Summary: Perl dependency evaluation incorrect
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 17880
Blocks: 17990
  Show dependency treegraph
 
Reported: 2017-01-24 23:22 UTC by David Cook
Modified: 2017-01-31 09:54 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17986 - Perl dependency evaluation incorrect (1.90 KB, patch)
2017-01-24 23:24 UTC, David Cook
Details | Diff | Splinter Review
Bug 17986 - Perl dependency evaluation incorrect (1.92 KB, patch)
2017-01-24 23:24 UTC, David Cook
Details | Diff | Splinter Review
Bug 17986: Perl dependency evaluation incorrect (1.94 KB, patch)
2017-01-25 08:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17986: Add tests (2.33 KB, patch)
2017-01-25 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17986: Perl dependency evaluation incorrect (1.90 KB, patch)
2017-01-25 11:40 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2017-01-24 23:22:59 UTC
While I was fixing Bug 17880, I mistakenly created a new bug via a copy/paste error.

At the moment, if you run koha_perl_deps.pl -a -c, or go to /cgi-bin/koha/about.pl and click on Perl modules, most installed modules will be coloured yellow indicating they need an upgrade.

The fix is a simple one line change, but will help solve a lot of headaches and confusion over people trying to meet dependencies.
Comment 1 David Cook 2017-01-24 23:24:15 UTC
Created attachment 59535 [details] [review]
Bug 17986 - Perl dependency evaluation incorrect

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

https://bugs.koha-community.org/show_bug.cgi?id=17880
Comment 2 David Cook 2017-01-24 23:24:40 UTC
Created attachment 59536 [details] [review]
Bug 17986 - Perl dependency evaluation incorrect

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

https://bugs.koha-community.org/show_bug.cgi?id=17880
Comment 3 David Cook 2017-01-24 23:26:18 UTC
TEST PLAN alternative

Instead of running koha_perl_deps.pl, you can visit /cgi-bin/koha/about.pl and click on "Perl modules".
Comment 4 jesus 2017-01-25 08:11:20 UTC
Packaged koha-kommon included in Debian Wheezy seems to be affected by this bug. Upgrades to koha-kommon 16.05.05 package fail complaining of perl "virtual packages" unmet dependencies.
Will this patch be commited to 16.11 or shall we wait to 17.05?

Best,
Jesus
Comment 5 Marcel de Rooy 2017-01-25 08:30:07 UTC
Created attachment 59538 [details] [review]
Bug 17986: Perl dependency evaluation incorrect

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Running koha_perl_deps.pl -u convinced me.
Comment 6 Jonathan Druart 2017-01-25 10:34:00 UTC
Created attachment 59544 [details] [review]
Bug 17986: Add tests
Comment 7 Jonathan Druart 2017-01-25 10:36:24 UTC
I am stuck, I tried to provide tests to highlight the problem, but I do not manage to make them fail.
There is something wrong in the code:
It seems that the same code is duplicated:
  if ( $params{module} ) {
    do something;
  } else {
    do almost the same thing;
  }

Except that in the first block, $_ is used, but is always undefined (not in a loop).
This code sounds ugly and buggy, it may be good to fix it.
Comment 8 Marcel de Rooy 2017-01-25 10:39:43 UTC
(In reply to Jonathan Druart from comment #7)
> I am stuck, I tried to provide tests to highlight the problem, but I do not
> manage to make them fail.
> There is something wrong in the code:
> It seems that the same code is duplicated:
>   if ( $params{module} ) {
>     do something;
>   } else {
>     do almost the same thing;
>   }
> 
> Except that in the first block, $_ is used, but is always undefined (not in
> a loop).
> This code sounds ugly and buggy, it may be good to fix it.

Yeah. I saw the code; it is horrible.
But this seems to fix at least one issue.
Comment 9 Jonathan Druart 2017-01-25 11:22:03 UTC
Ok, I am rewriting it, will move that to another bug report.
Comment 10 Jonathan Druart 2017-01-25 11:40:18 UTC
Created attachment 59545 [details] [review]
Bug 17986: Perl dependency evaluation incorrect

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Running koha_perl_deps.pl -u convinced me.
Comment 11 Jonathan Druart 2017-01-25 11:41:51 UTC
(In reply to Jonathan Druart from comment #9)
> Ok, I am rewriting it, will move that to another bug report.

See bug 17990.
Comment 12 David Cook 2017-01-26 23:11:24 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Jonathan Druart from comment #9)
> > Ok, I am rewriting it, will move that to another bug report.
> 
> See bug 17990.

You're a champ, Jonathan! I have another two reports* that looks at fixing issues with this code, so cheers for rewriting it overall. 

*bug 17882
*bug 17887
Comment 13 Kyle M Hall 2017-01-30 14:17:33 UTC
Pushed to master for 17.05, thanks David!
Comment 14 Katrin Fischer 2017-01-30 14:37:34 UTC
Much better.

This patch has been pushed to 16.11.x and will be in 16.11.03.
Comment 15 Mason James 2017-01-31 09:21:09 UTC
Pushed to 16.05.x, for 16.05.09 release