Bug 13758

Summary: KOHAVERSION should be statically set
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, m.de.rooy, mtompset, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9006
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14427    
Attachments: Bug 13758: Move the Koha version from kohaversion.pl
Bug 13758: Koha::Version introduced (counterpatch)
Bug 13758: Koha::Version introduced (counterpatch)
Bug 13758: Add POD
Bug 13758: Move the Koha version from kohaversion.pl
[SIGNED OFF] Bug 13758: Move the Koha version from kohaversion.pl
[SIGNED OFF] Bug 13758: Add POD
[PASSED QA] Bug 13758: Move the Koha version from kohaversion.pl
[PASSED QA] Bug 13758: Add POD
Bug 13758: (QA followup) Make Makefile.PL aware of Koha.pm
Bug 13758: (QA followup) revert case change that broke the tests
Bug 13758: Correct KOHA::VERSION in OverDrive.pm

Description Tomás Cohen Arazi 2015-02-24 19:37:42 UTC
Currently, C4::Context::KOHAVERSION calculates the version number by running kohaversion.pl on a new Perl interpreter:

sub KOHAVERSION {
    my $cgidir = C4::Context->intranetdir;

    # Apparently the GIT code does not run out of a CGI-BIN subdirectory
    # but distribution code does?  (Stan, 1jan08)
    if(-d $cgidir . "/cgi-bin"){
        my $cgidir .= "/cgi-bin";
    }
    
    do $cgidir."/kohaversion.pl" || die "NO $cgidir/kohaversion.pl";
    return kohaversion();
}

There's no point on doing this, as it is a hardcoded value we write on each DB update.

It adds several milliseconds of latency to each request as the version comparisson is done on each request to detect needed DB updates.

It should be statically set as $VERSION is (hmpf) on C4::Context, and if for some reason we want to keep kohaversion.pl we should definitely read C4::Context::VERSION (or KOHAVERSION if we rename it).
Comment 1 Jonathan Druart 2015-03-17 15:23:41 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-03-17 15:27:04 UTC
I did not find any time saving using a new package to store the version.

# Without the patch
$ more t.pl
use Modern::Perl;
use C4::Context;
say C4::Context->KOHAVERSION;

$ time perl t.pl => 0.150s

# With the patch
$ more t.pl
use Modern::Perl;
use C4::Context;
use Koha;
say Koha::version;

$ time perl t.pl => 0.150s

To be fair I kept the use of C4::Context, if I remove it I get, of course, ~0.024s
Comment 3 Tomás Cohen Arazi 2015-03-17 15:36:14 UTC
(In reply to Jonathan Druart from comment #2)
> I did not find any time saving using a new package to store the version.
> 
> # Without the patch
> $ more t.pl
> use Modern::Perl;
> use C4::Context;
> say C4::Context->KOHAVERSION;
> 
> $ time perl t.pl => 0.150s
> 
> # With the patch
> $ more t.pl
> use Modern::Perl;
> use C4::Context;
> use Koha;
> say Koha::version;
> 
> $ time perl t.pl => 0.150s
> 
> To be fair I kept the use of C4::Context, if I remove it I get, of course,
> ~0.024s

Could you try the opac home page using ntyprof?
Comment 4 Tomás Cohen Arazi 2015-03-17 15:37:11 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> (In reply to Jonathan Druart from comment #2)
> > I did not find any time saving using a new package to store the version.
> > 
> > # Without the patch
> > $ more t.pl
> > use Modern::Perl;
> > use C4::Context;
> > say C4::Context->KOHAVERSION;
> > 
> > $ time perl t.pl => 0.150s
> > 
> > # With the patch
> > $ more t.pl
> > use Modern::Perl;
> > use C4::Context;
> > use Koha;
> > say Koha::version;
> > 
> > $ time perl t.pl => 0.150s
> > 
> > To be fair I kept the use of C4::Context, if I remove it I get, of course,
> > ~0.024s
> 
> Could you try the opac home page using ntyprof?

BTW, I would simplify getting the Koha version just for the sake of cleaning the code.
Comment 5 Jonathan Druart 2015-03-17 15:56:37 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> Could you try the opac home page using ntyprof?

I would not expect any difference. If the simple script I tested give the same execution time, it should be the same on the opac main page.

(In reply to Tomás Cohen Arazi from comment #4)
> BTW, I would simplify getting the Koha version just for the sake of cleaning
> the code.

Yep, that's a sufficient reason :)
Comment 6 Tomás Cohen Arazi 2015-03-17 18:08:18 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2015-03-17 18:17:19 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-03-18 08:25:21 UTC
I prefer my version :)
It cleans much more code, and Koha::version() (or $Koha::VERSION) make more sense that Koha::Version::VERSION.
Besides, your patch introduces another way to know the Koha version, mine replaces the existing one.
Comment 9 Jonathan Druart 2015-03-18 14:51:47 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2015-04-29 12:38:37 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2015-04-29 12:39:59 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2015-04-29 12:40:07 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-05-01 13:20:19 UTC
Created attachment 38738 [details] [review]
[PASSED QA] Bug 13758: Move the Koha version from kohaversion.pl

It will permit not to run another perl interpreter.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2015-05-01 13:20:29 UTC
Created attachment 38739 [details] [review]
[PASSED QA] Bug 13758: Add POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Tomás Cohen Arazi 2015-05-07 15:25:33 UTC
Created attachment 38952 [details] [review]
Bug 13758: (QA followup) Make Makefile.PL aware of Koha.pm

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 16 Tomás Cohen Arazi 2015-05-07 15:38:18 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 17 Tomás Cohen Arazi 2015-05-07 17:21:20 UTC
Created attachment 38958 [details] [review]
Bug 13758: (QA followup) revert case change that broke the tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 18 Marcel de Rooy 2015-05-08 08:20:59 UTC
Created attachment 38971 [details] [review]
Bug 13758: Correct KOHA::VERSION in OverDrive.pm

Correct $KOHA::VERSION to $Koha::VERSION.
Also, passing this string to LWP::UserAgent is wrong. It expects key/value
pairs. Since this string is apparently intended as an agent, this patch
passes it as such.
Note: The OverDrive has unfortunately no unit tests.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested this change in the perl debugger with just these lines:
    use C4::External::OverDrive;
    C4::External::OverDrive::_request();
Printed $ua->agent while stepping into sub _request.
Without the agent change, the adjusted Koha string would just be ignored
and I would still have "libwww-perl/6.04" as agent.
Comment 19 Marcel de Rooy 2015-05-08 08:22:09 UTC
Tomas: Please see the last QA follow-up.
Comment 20 Mark Tompsett 2015-05-09 17:25:32 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> Patches pushed to master.
> 
> Thanks Jonathan!

Uh... no it isn't?
prove t/db_dependent/Context.t kaboom, because C4::Context::KOHAVERSION is not there yet.
Comment 21 Tomás Cohen Arazi 2015-05-09 18:12:30 UTC
(In reply to M. Tompsett from comment #20)
> (In reply to Tomás Cohen Arazi from comment #16)
> > Patches pushed to master.
> > 
> > Thanks Jonathan!
> 
> Uh... no it isn't?
> prove t/db_dependent/Context.t kaboom, because C4::Context::KOHAVERSION is
> not there yet.

git fetch
git reset--hard origin/master

Maybe?
Comment 22 Mark Tompsett 2015-05-10 02:28:09 UTC
(In reply to Tomás Cohen Arazi from comment #21)
[SNIP]
> Maybe?

Remedied somehow. Confusion probably started with rebase of bug 5010. :)
Comment 23 Tomás Cohen Arazi 2015-05-11 13:09:20 UTC
Patch pushed to master.

Thanks for the followup Marcel!