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).
Created attachment 36951 [details] [review] Bug 13758: Move the Koha version from kohaversion.pl It will permit not to run another perl interpreter.
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
(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?
(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.
(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 :)
Created attachment 36962 [details] [review] Bug 13758: Koha::Version introduced (counterpatch) This patch introduces a trivial package: Koha::Version. It only contains a constant: VERSION. It is set the current version number. The idea is that we stop relying on calling the kohaversion.pl script to get the current version number. The workfl Sponsored-by: Universidad Nacional de Cordoba
Created attachment 36963 [details] [review] Bug 13758: Koha::Version introduced (counterpatch) This patch introduces a trivial package: Koha::Version. It only contains a constant: VERSION. It is set the current version number. The idea is that we stop relying on calling the kohaversion.pl script to get the current version number. The workflow for RM/RMaints versioning their updates should be slightly changed, but overall is the same effort. To test: 1 - Run $ perl Makefile.PL (it doesn't matter which version, just continue till the end) => SUCCESS: The printed version, is the correct one (3.19.00.016). 2 - Tweak the Koha/Version.pm file, shifting the version number - Launch the opac interface => SUCCESS: The OPAC shows the maintenance page - Launch the intranet: => SUCCESS: You are offered the update Those are the most important use cases, because the rest just uses C4::Context->KOHAVERSION() to access the version number, which is proven to work by the use cases. Edit: The commit message got truncated at some point, probably because I recovered the version prior to completing the commit message after the power outage
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.
Created attachment 36988 [details] [review] Bug 13758: Add POD
Created attachment 38661 [details] [review] 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>
Created attachment 38662 [details] [review] [SIGNED OFF] 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>
Created attachment 38663 [details] [review] [SIGNED OFF] Bug 13758: Add POD Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
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>
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>
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>
Patches pushed to master. Thanks Jonathan!
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>
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.
Tomas: Please see the last QA follow-up.
(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.
(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?
(In reply to Tomás Cohen Arazi from comment #21) [SNIP] > Maybe? Remedied somehow. Confusion probably started with rebase of bug 5010. :)
Patch pushed to master. Thanks for the followup Marcel!