Description
Tomás Cohen Arazi (tcohen)
2015-02-24 19:37:42 UTC
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! |