Bug 14167 - Add Koha::Logger based on Log4perl
Summary: Add Koha::Logger based on Log4perl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 8190 13413 (view as bug list)
Depends on:
Blocks: 15253 16302 16303 16304 16312 16313 16509 14906 15626 16084
  Show dependency treegraph
 
Reported: 2015-05-07 15:20 UTC by Kyle M Hall
Modified: 2016-12-05 21:22 UTC (History)
10 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 14167 - Add Koha::Logger based on Log4perl (6.50 KB, patch)
2015-05-07 15:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167 - Add Koha::Logger based on Log4perl (6.50 KB, patch)
2015-05-07 15:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167 - Add Koha::Logger based on Log4perl (6.51 KB, patch)
2015-05-08 13:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167 - Add Koha::Logger based on Log4perl (6.52 KB, patch)
2015-05-08 13:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167 - Add Koha::Logger based on Log4perl (6.52 KB, patch)
2015-05-08 16:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167 - Add Koha::Logger based on Log4perl (6.58 KB, patch)
2015-05-12 18:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14167 [QA Followup] - Add template and changes needed for packages (2.52 KB, patch)
2015-05-14 11:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167 - Add Koha::Logger based on Log4perl (5.33 KB, patch)
2015-06-05 07:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167 - [DO_NOT_PUSH] Renewal example patch for logging (2.00 KB, patch)
2015-06-05 07:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167 [QA Followup] - Add template and changes needed for packages (2.65 KB, patch)
2015-06-05 07:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167 - Add Koha::Logger based on Log4perl (5.33 KB, patch)
2015-06-05 07:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167 - [DO_NOT_PUSH] Renewal example patch for logging (2.00 KB, patch)
2015-06-05 07:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167 [QA Followup] - Add template and changes needed for packages (2.54 KB, patch)
2015-06-05 07:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167 [QA Followup] - Add template and changes needed for packages (2.65 KB, patch)
2015-06-05 07:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167: [QA Follow-up] Adjust category handling in Koha::Logger (1.93 KB, patch)
2015-06-05 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167 - [DO_NOT_PUSH] Renewal example patch for logging (2.09 KB, patch)
2015-06-05 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167 [QA Followup] - Add template and changes needed for packages (2.65 KB, patch)
2015-06-05 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167 [QA Followup] - Embed default config into Koha::LoggerBug (2.06 KB, patch)
2015-06-16 17:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167: [QA Follow-up] Making Koha::Logger bit more crash resistant (6.75 KB, patch)
2015-06-26 12:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167: Add a unit test for Koha::Logger (2.05 KB, patch)
2015-06-26 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14167 - Add Koha::Logger based on Log4perl (5.31 KB, patch)
2015-06-26 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167: [QA Follow-up] Adjust category handling in Koha::Logger (1.89 KB, patch)
2015-06-26 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167 - [DO_NOT_PUSH] Renewal example patch for logging (2.05 KB, patch)
2015-06-26 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167 [QA Followup] - Add template and changes needed for packages (2.62 KB, patch)
2015-06-26 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167 [QA Followup] - Embed default config into Koha::Logger (2.06 KB, patch)
2015-06-26 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14167: [QA Follow-up] Making Koha::Logger bit more crash resistant (6.78 KB, patch)
2015-06-26 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14167: Add a unit test for Koha::Logger (2.09 KB, patch)
2015-06-26 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167 [QA Followup] - Tidy Koha/Logger.pm (5.59 KB, patch)
2015-06-26 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14167: (QA followup) use warn instead of just STDERR (3.50 KB, patch)
2015-07-21 13:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14167: (QA followup) new entries in koha-conf.xml should be tested in about.pl (1.80 KB, patch)
2015-07-21 14:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14167: package dependencies for Log::Log4perl (1.28 KB, patch)
2015-07-23 23:15 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 14167: remove the log4perl.conf file from /etc for packages (759 bytes, patch)
2015-07-23 23:51 UTC, Robin Sheat
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2015-05-07 15:20:43 UTC
Koha needs a better logger, and it seems like the best solution would be to take advantage of Log4perl which is already a fully featured logger. We use Log4perl to selectively decide what statements should be logged, and where they should go!
Comment 1 Kyle M Hall 2015-05-07 15:28:13 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2015-05-07 15:34:10 UTC Comment hidden (obsolete)
Comment 3 M. Tompsett 2015-05-07 19:01:43 UTC
Comment on attachment 38954 [details] [review]
Bug 14167 - Add Koha::Logger based on Log4perl

Review of attachment 38954 [details] [review]:
-----------------------------------------------------------------

::: Koha/Logger.pm
@@ +58,5 @@
> +    my ( $class, $category ) = @_;
> +
> +    croak("No category passed in!") unless $category;
> +
> +    my $interface = C4::Context->interface();

What if it isn't set?
Comment 4 Kyle M Hall 2015-05-08 09:57:56 UTC
(In reply to M. Tompsett from comment #3)
> Comment on attachment 38954 [details] [review] [review]
> Bug 14167 - Add Koha::Logger based on Log4perl
> 
> Review of attachment 38954 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/Logger.pm
> @@ +58,5 @@
> > +    my ( $class, $category ) = @_;
> > +
> > +    croak("No category passed in!") unless $category;
> > +
> > +    my $interface = C4::Context->interface();
> 
> What if it isn't set?

The idea is that we want the category to be required. That is absolutely no reason to log a line without specifying a category.
Comment 5 Kyle M Hall 2015-05-08 13:52:32 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2015-05-08 13:54:32 UTC
> > ::: Koha/Logger.pm
> > @@ +58,5 @@
> > > +    my ( $class, $category ) = @_;
> > > +
> > > +    croak("No category passed in!") unless $category;
> > > +
> > > +    my $interface = C4::Context->interface();
> > 
> > What if it isn't set?

I think I missed your meaning. That's a very good question. By default if the interface isn't 'staff' it's 'opac'. I think maybe we should be able to pass in an interface parameter as well so we can set the interface to 'cli' or 'cron' or other such things.
Comment 7 Kyle M Hall 2015-05-08 13:55:53 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2015-05-08 16:16:07 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2015-05-12 18:16:25 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2015-05-12 18:19:06 UTC
Nice concise bit of code here adding a long needed logger to koha in as simple a way as possible using a well developed lib.

Lets make the choice and get this rolling!

Minor comments, I can't fully remember how the package building get put together.. but I 'think' we need a copy of the log4perl.conf file that's added here also adding under the debian dir somewhere.

I'll try and dig out my notes to advise, assuming Tomas or Robin don't beat me to it. ;)
Comment 11 Tomás Cohen Arazi 2015-05-12 18:23:52 UTC
(In reply to Martin Renvoize from comment #10)
> Nice concise bit of code here adding a long needed logger to koha in as
> simple a way as possible using a well developed lib.
> 
> Lets make the choice and get this rolling!
> 
> Minor comments, I can't fully remember how the package building get put
> together.. but I 'think' we need a copy of the log4perl.conf file that's
> added here also adding under the debian dir somewhere.
> 
> I'll try and dig out my notes to advise, assuming Tomas or Robin don't beat
> me to it. ;)

Indeed, for the packages we will need to:
- Add a template file into debian/templates/(logger4perl-site.conf.in ?)
- Tweak the debian/scripts/koha-create script so it correctly puts the
  template file with the right variable substitutions.
- Maybe add some switches controlling the logging configuration, but this could
  go into a separate bug.
Comment 12 Robin Sheat 2015-05-12 22:03:04 UTC
How does this compare to bug 13413?

As for the packaging stuff, what Tomás says looks like the right idea to me, it shouldn't be too complicated.
Comment 13 Kyle M Hall 2015-05-13 11:05:01 UTC
(In reply to Robin Sheat from comment #12)
> How does this compare to bug 13413?
> 
> As for the packaging stuff, what Tomás says looks like the right idea to me,
> it shouldn't be too complicated.

The biggest advantages of this patch vs bug 13413 are
a) Simplicity of implementation
b) Incredibly powerful configurability by just editing a configuration file
Comment 14 Kyle M Hall 2015-05-14 11:05:33 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2015-06-04 12:33:27 UTC
QA: Looking at this one now..
Comment 16 Marcel de Rooy 2015-06-05 07:48:20 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2015-06-05 07:48:26 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2015-06-05 07:48:29 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2015-06-05 07:55:13 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2015-06-05 07:55:18 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2015-06-05 07:55:22 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2015-06-05 07:55:52 UTC
Please wait..not ready yet!
Comment 23 Marcel de Rooy 2015-06-05 07:58:49 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2015-06-05 08:56:35 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2015-06-05 08:56:39 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2015-06-05 08:56:43 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2015-06-05 09:36:04 UTC
QA Comment:
Nice feature. Economic implementation of logging.
Although the code looks good to me, I still have some concerns:

[1] For logging level and category handling, I made some tiny adjustments via amending or follow-up.

[2] A bigger problem could be permissions on the logfile. 
If you do not have sufficient permissions, Log4perl will make a crash. So this is a very fundamental issue. In Debian the koha instance user should have permissions on the file, but I noticed that after a forced logrotate, the file permissions for the new log file were changed to root and Koha crashed on that.
What about other distros btw?

[3] The BEGIN section of Koha::Logger assumes that it will have either a conf file via $ENV{LOG4PERL_CONF} or the koha-conf.xml.
If you do not have either one, Koha will again crash on you (Configuration not defined).
It is safer imo to go always via the koha-conf. If there is nothing in koha-conf, set some flag, warn once and do nothing or perhaps fall back to warn. (Note that you should trap all calls to warn, trace etc. in that case.) 
Note that a new installation would be fine. But an upgrade does not guarantee the presence of a config file.
Also note that you can pass some default configuration string to init() too. 

By conclusion, we could argue that a Koha administrator should just take care of the perl module and the log4perl config file and push this as-is. But we also know that this will result in problems at various locations when upgrading. What would be the best way to go here?

I will be happy to follow through on this report, but not in the next two weeks. (But another QAer is welcome too.) 

Changing status to reflect need for changes or clarification..
Comment 28 Kyle M Hall 2015-06-16 16:42:32 UTC
(In reply to Marcel de Rooy from comment #27)
> QA Comment:
> Nice feature. Economic implementation of logging.
> Although the code looks good to me, I still have some concerns:
> 
> [1] For logging level and category handling, I made some tiny adjustments
> via amending or follow-up.

Excellent! 
> 
> [2] A bigger problem could be permissions on the logfile. 
> If you do not have sufficient permissions, Log4perl will make a crash. So
> this is a very fundamental issue. In Debian the koha instance user should
> have permissions on the file, but I noticed that after a forced logrotate,
> the file permissions for the new log file were changed to root and Koha
> crashed on that.
> What about other distros btw?

Is logrotate creating the files? I don't see a create command in /etc/logrotate.d/koha-common.

It's possible that this could be fixed with an addition like:

create 660 root koha

where the owner can be root as long as we were to create a koha group that each koha-* user belonged to. However, I don't have any idea how to write the upgrade patch for that.

http://www.thecave.info/logrotate-set-file-permissions/
 
> [3] The BEGIN section of Koha::Logger assumes that it will have either a
> conf file via $ENV{LOG4PERL_CONF} or the koha-conf.xml.
> If you do not have either one, Koha will again crash on you (Configuration
> not defined).
> It is safer imo to go always via the koha-conf. If there is nothing in
> koha-conf, set some flag, warn once and do nothing or perhaps fall back to
> warn. (Note that you should trap all calls to warn, trace etc. in that
> case.) 
> Note that a new installation would be fine. But an upgrade does not
> guarantee the presence of a config file.
> Also note that you can pass some default configuration string to init() too. 

That could be fixed with a followup. Basically we should check for the existance of the log4perl config file, and if it doesn't exist pass it a default config that can be hard coded into the module ( and would be the same as the config we have here ).

> By conclusion, we could argue that a Koha administrator should just take
> care of the perl module and the log4perl config file and push this as-is.
> But we also know that this will result in problems at various locations when
> upgrading. What would be the best way to go here?

I would say we should do what is in our power and is reasonable to make the upgrade path as smooth as possible. It may be that we can't automate everything, and we'll have to provide upgrade instructions, but none of this seems too onerous.
 
> I will be happy to follow through on this report, but not in the next two
> weeks. (But another QAer is welcome too.) 
> 
> Changing status to reflect need for changes or clarification..
Comment 29 Kyle M Hall 2015-06-16 17:33:26 UTC Comment hidden (obsolete)
Comment 30 Martin Renvoize 2015-06-17 07:42:15 UTC
I'm happy with Marcels and Kyles follow-ups, just the file permissions issue Marcel brought to our attention that needs addressing now, and then I think we should be good to go with this one.
Comment 31 Katrin Fischer 2015-06-21 11:16:56 UTC
Moving to "Failed QA" to indicate there is something missing here still - please address! :)
Comment 32 Marcel de Rooy 2015-06-26 08:53:44 UTC
(In reply to Katrin Fischer from comment #31)
> Moving to "Failed QA" to indicate there is something missing here still -
> please address! :)

Now working on this one again. Hope to get this further!
Comment 33 Marcel de Rooy 2015-06-26 12:21:28 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2015-06-26 12:23:07 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2015-06-26 12:28:28 UTC
Added two patches. See the commit messages.
How do we proceed from here?
Since the last two follow-ups add/change some more lines, I will first ask for a repeated signoff from Martin or Kyle. (But I still consider them as QA follow-ups.)
If they agree (read: signoff), I will pass QA.
Comment 36 Kyle M Hall 2015-06-26 13:59:31 UTC
Created attachment 40657 [details] [review]
Bug 14167 - Add Koha::Logger based on Log4perl

Koha needs a better logger, and it seems like the best solution would be
to take advantage of Log4perl which is already a fully featured logger.
We use Log4perl to selectively decide what statements should be logged,
and where they should go!

Test plan:
0) Install Log::Log4perl via packages or cpan
1) Apply this patch and the example renewal patch
2) Copy etc/log4perl.conf to your koha conf directory, edit the paths
   to match your current error logs
3) Edit your koha-conf file and add the
   <log4perl_conf>/path/to/log4perl.conf</log4perl_conf> line
4) Watch your intranet and opac error logs
5) Perform a renewal via the staff interface, note there is nothing new
   in the log file
7) Update the log4perl.conf, change the log level from WARN to TRACE
   for both the staff and opac sides
8) Perform a renewal via the staff interface, note the logged lines
9) Perform a renewal via the opac, note the logged lines

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended this patch: Moved the renewal stuff to a separate example patch.
And upgraded the DEBUG level to WARN in the log4perl config file.
Comment 37 Kyle M Hall 2015-06-26 13:59:38 UTC
Created attachment 40658 [details] [review]
Bug 14167: [QA Follow-up] Adjust category handling in Koha::Logger

The first patch makes the category parameter mandatory.
This patch makes it optional, falling back to the current package. This is
the expected log4perl model. It would not be necessary to pass the class
name everywhere (check subclasses..)
It also adds a delimiter between the interface and the class name.

This allows you to add config lines like:

log4perl.logger.opac.C4.Auth = DEBUG, OPAC
log4perl.logger.intranet.C4.Circulation = TRACE, INTRANET

The first line would make the logger more sensitive to C4::Auth log
messages while staying at the WARN level for other messages, etc. The
second line makes the trace visible from the renewal example.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Kyle M Hall 2015-06-26 13:59:41 UTC
Created attachment 40659 [details] [review]
Bug 14167 - [DO_NOT_PUSH] Renewal example patch for logging

This patch adds logging to the renewal process to test the first patch.
Note to RM: Do not push this patch.

Test plan:
See the first patch.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended the get call in light of the QA follow-up for category.
Comment 39 Kyle M Hall 2015-06-26 13:59:43 UTC
Created attachment 40660 [details] [review]
Bug 14167 [QA Followup] - Add template and changes needed for packages

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: upgraded DEBUG level to WARN in config file.
Comment 40 Kyle M Hall 2015-06-26 13:59:46 UTC
Created attachment 40661 [details] [review]
Bug 14167 [QA Followup] - Embed default config into Koha::Logger
Comment 41 Kyle M Hall 2015-06-26 13:59:49 UTC
Created attachment 40662 [details] [review]
[SIGNED-OFF] Bug 14167: [QA Follow-up] Making Koha::Logger bit more crash resistant

Moving the BEGIN block to _init for the most part. We only need to
initialize when we actually start using the logger.
Removed the third init part. If we do not have a log4perl_conf in the
koha config, we are not using it yet.
Method get uses hash parameters now. It calls init. If we do not have a
config or the logfile is not writable, we will not use log4perl.
Using AUTOLOAD as a wrapper around Log4perl in order to add some checks
that log4perl does not have.
If a logrotate would change file permissions on a default logfile, we
should catch that now too (see recheck).

Test plan:
Run the previous tests again.
Will still add a unit test.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Kyle M Hall 2015-06-26 13:59:51 UTC
Created attachment 40663 [details] [review]
[SIGNED-OFF] Bug 14167: Add a unit test for Koha::Logger

Test plan:
Run the test.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Kyle M Hall 2015-06-26 13:59:54 UTC
Created attachment 40664 [details] [review]
Bug 14167 [QA Followup] - Tidy Koha/Logger.pm
Comment 44 Marcel de Rooy 2015-06-26 14:26:49 UTC
Thanks for your quick signoff, Kyle.

QA Comment:
All issues mentioned before have been addressed in various follow-ups. An upgrade without a log4perl config file will no longer be a problem.

Passed QA
Comment 45 Marcel de Rooy 2015-06-26 14:33:06 UTC
Robin,

What do you think of adding a line like
create 660 [koha-instance-user] [koha-instance-group]
to koha-common.logrotate ?
Comment 46 Robin Sheat 2015-07-20 22:36:48 UTC
(In reply to Marcel de Rooy from comment #45)
> Robin,
> 
> What do you think of adding a line like
> create 660 [koha-instance-user] [koha-instance-group]
> to koha-common.logrotate ?

Having it as the koha-instance-group is a bit redundant, it perhaps should be adm like most syslog things (I know it's effectively like that at the moment, that's probably wrong.)
Comment 47 Robin Sheat 2015-07-20 22:48:39 UTC
(In reply to Marcel de Rooy from comment #44)
> All issues mentioned before have been addressed in various follow-ups. An
> upgrade without a log4perl config file will no longer be a problem.

It is possible, and perhaps the best solution, to drop files in /etc/koha/sites/* rather than assume a default. This'd be done in debian/koha-common.postinst.

This is really just a heads up in case anyone wants to do it, I don't really insist on it :)
Comment 48 Tomás Cohen Arazi 2015-07-21 13:59:41 UTC
Created attachment 41109 [details] [review]
Bug 14167: (QA followup) use warn instead of just STDERR

This patch makes Koha::Logger use 'warn' instead of just printing
to STDERR. It introduces tests for this warnings too.

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 49 Tomás Cohen Arazi 2015-07-21 14:55:41 UTC
Created attachment 41111 [details] [review]
Bug 14167: (QA followup) new entries in koha-conf.xml should be tested in about.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 50 Tomás Cohen Arazi 2015-07-21 14:57:35 UTC
Enhancement pushed to master.

Thanks Kyle and everyone involved.
Comment 51 Tomás Cohen Arazi 2015-07-21 14:58:23 UTC
*** Bug 13413 has been marked as a duplicate of this bug. ***
Comment 52 Tomás Cohen Arazi 2015-07-21 14:58:38 UTC
*** Bug 8190 has been marked as a duplicate of this bug. ***
Comment 53 Robin Sheat 2015-07-22 03:42:10 UTC
A bit about log4perl here: http://blogs.perl.org/users/dean/2015/07/getting-modern-with-logging-via-log4perl.html
Comment 54 Robin Sheat 2015-07-23 23:15:55 UTC
Created attachment 41145 [details] [review]
Bug 14167: package dependencies for Log::Log4perl
Comment 55 Robin Sheat 2015-07-23 23:16:36 UTC
Just adding the dependencies patch here to save creating a whole new bug for it.
Comment 56 Robin Sheat 2015-07-23 23:33:27 UTC
...and this bug breaks a package build:

make[1]: Leaving directory `/tmp/buildd/koha-3.21~git+20150724112213.255c84e7'
   dh_install -O--fail-missing
dh_install: etc/koha/log4perl.conf exists in debian/tmp but is not installed to anywhere
dh_install: missing files, aborting
make: *** [binary] Error 2
dpkg-buildpackage: error: fakeroot debian/rules binary gave error exit status 2


shouldn't be a big deal though.
Comment 57 Robin Sheat 2015-07-23 23:51:57 UTC
Created attachment 41146 [details] [review]
Bug 14167: remove the log4perl.conf file from /etc for packages

We don't need that file in the package build because it's generated on a
per-site basis from a template.
Comment 58 Tomás Cohen Arazi 2015-07-24 17:08:45 UTC
Followups pushed to master.

Thanks Robin!
Comment 59 Olli-Antti Kivilahti 2016-04-19 16:33:10 UTC
Good job with the Log4perl!
Comment 60 Olli-Antti Kivilahti 2016-04-20 17:35:14 UTC
I found a few things to improve to be able to use Log4perl with cronjobs/commandline scripts.

They all come with good regression and performance test coverage since Koha::Logger is a critical subsystem component.


If you made it this far, the correct order of applying the dependant patches is as follows:

Bug 16313 - Koha::Logger doesn't forward the Log::Log4perl::Logger return v~
Bug 16312 - Koha::Logger overload configuration for command line scripts ~
Bug 16306 - C4::Context->setCommandlineEnvironment() convenience function
Bug 16304 - Koha::Logger, lazy load loggers so environment has time to get set
Bug 16303 - Koha::Logger appenders for interface 'commandline'
Bug 16302 - Koha::Logger performance tests
Bug 14167 - Add Koha::Logger based on Log4perl.