Bug 13758 - KOHAVERSION should be statically set
Summary: KOHAVERSION should be statically set
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 14427
  Show dependency treegraph
 
Reported: 2015-02-24 19:37 UTC by Tomás Cohen Arazi
Modified: 2016-06-21 21:36 UTC (History)
5 users (show)

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


Attachments
Bug 13758: Move the Koha version from kohaversion.pl (13.21 KB, patch)
2015-03-17 15:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13758: Koha::Version introduced (counterpatch) (4.02 KB, patch)
2015-03-17 18:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13758: Koha::Version introduced (counterpatch) (4.65 KB, patch)
2015-03-17 18:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13758: Add POD (1.63 KB, patch)
2015-03-18 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13758: Move the Koha version from kohaversion.pl (13.29 KB, patch)
2015-04-29 12:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13758: Move the Koha version from kohaversion.pl (13.31 KB, patch)
2015-04-29 12:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13758: Add POD (1.69 KB, patch)
2015-04-29 12:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[PASSED QA] Bug 13758: Move the Koha version from kohaversion.pl (13.37 KB, patch)
2015-05-01 13:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13758: Add POD (1.77 KB, patch)
2015-05-01 13:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13758: (QA followup) Make Makefile.PL aware of Koha.pm (952 bytes, patch)
2015-05-07 15:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13758: (QA followup) revert case change that broke the tests (2.03 KB, patch)
2015-05-07 17:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13758: Correct KOHA::VERSION in OverDrive.pm (1.36 KB, patch)
2015-05-08 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!