Bug 25527

Summary: Package does not build because of missing log4perl.conf
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: PackagingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: blocker    
Priority: P5 - low CC: 1joynelson, kyle, martin.renvoize, mirko, mtj, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 25172    
Bug Blocks: 25529    
Attachments: Bug 25527: Initialize the logger when required
Bug 25527: Initialize the logger in the constructor in ExternalContent
Bug 25527: Add logger to Koha::ExternalContent
Bug 25527: Initialize the logger when required
Bug 25527: Add logger to Koha::ExternalContent
Bug 25527: Initialize the logger when required
Bug 25527: Add logger to Koha::ExternalContent

Description Jonathan Druart 2020-05-18 12:06:19 UTC
Build breaks due to recent changes in Koha::Logger. It fails because of absence of Log4Perl.conf
Comment 1 Tomás Cohen Arazi 2020-05-18 12:12:39 UTC
Steps to reproduce in KTD without packaging in the middle:

 $ rm /etc/koha/sites/kohadev/log4perl.conf
 $ kshell
k$ prove t/00-load.t
t/00-load.t .. 6/?
#   Failed test 'use C4::Breeding;'
#   at t/00-load.t line 46.
#     Tried to use 'C4::Breeding'.
#     Error:  Cannot open /etc/koha/sites/kohadev/log4perl.conf (No such file or directory) at /usr/share/perl5/Log/Log4perl/Config/BaseConfigurator.pm line 51.
# Compilation failed in require at /kohadevbox/koha/C4/Breeding.pm line 29.
# BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Breeding.pm line 29.
# Compilation failed in require at t/00-load.t line 46.
# BEGIN failed--compilation aborted at t/00-load.t line 46.
Bailout called.  Further testing stopped:  ***** PROBLEMS LOADING FILE 'C4::Breeding'
FAILED--Further testing stopped: ***** PROBLEMS LOADING FILE 'C4::Breeding'
kohadev-koha@8c9d066462c7:/kohadevbox/koha$
Comment 2 Tomás Cohen Arazi 2020-05-18 12:32:07 UTC
Created attachment 105013 [details] [review]
Bug 25527: Initialize the logger when required

In an OO package, the logger initialization should happen in the
constructor. This is not an OO package and the initialization is
happening on loading it. This is a wrong behaviour and certainly breaks
in environments where initialization cannot happen (package building,
for example). There could be several options to solve this, as it is
used in a single sub on this package, I opted for initializing on that
sub.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2020-05-18 12:32:12 UTC
Created attachment 105014 [details] [review]
Bug 25527: Initialize the logger in the constructor in ExternalContent

This patch makes Koha::Logger initialization happen in the ->new method
for the OverDrive handler. In the case of RecordedBooks, it doesn't look
like it is used so I just removed it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2020-05-18 12:33:18 UTC
I'm currently rebuilding my dev env to see if I broke anything. But it looks correct and should be tested :-D
Comment 5 Tomás Cohen Arazi 2020-05-18 13:08:22 UTC
Created attachment 105017 [details] [review]
Bug 25527: Add logger to Koha::ExternalContent

This patch makes Koha::Logger initialization happen in the ->new method
for the Koha::ExternalContent-derived classes. In the case of RecordedBooks,
it doesn't look like it is used at all.

In the case of OverDrive, it will now use the Koha::ExternalContent
exported logger accessor.

I added tests for this addition to Koha::ExternalContent to the
OverDrive tests.

I also removed references to Test::DBIx::Class as it is not used at all.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha_ExternalContent_OverDrive.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Victor Grousset/tuxayo 2020-05-18 13:56:58 UTC
Created attachment 105018 [details] [review]
Bug 25527: Initialize the logger when required

In an OO package, the logger initialization should happen in the
constructor. This is not an OO package and the initialization is
happening on loading it. This is a wrong behaviour and certainly breaks
in environments where initialization cannot happen (package building,
for example). There could be several options to solve this, as it is
used in a single sub on this package, I opted for initializing on that
sub.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Victor Grousset/tuxayo 2020-05-18 13:57:01 UTC
Created attachment 105019 [details] [review]
Bug 25527: Add logger to Koha::ExternalContent

This patch makes Koha::Logger initialization happen in the ->new method
for the Koha::ExternalContent-derived classes. In the case of RecordedBooks,
it doesn't look like it is used at all.

In the case of OverDrive, it will now use the Koha::ExternalContent
exported logger accessor.

I added tests for this addition to Koha::ExternalContent to the
OverDrive tests.

I also removed references to Test::DBIx::Class as it is not used at all.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha_ExternalContent_OverDrive.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 8 Victor Grousset/tuxayo 2020-05-18 13:58:56 UTC
It works!

additional test: My package build fails but at a much later step and doesn't seems at all related to this.
Comment 9 Jonathan Druart 2020-05-18 14:16:44 UTC
> Bug 25527: Initialize the logger when required

I should note that this patch revert a pref optimization (bug 14597 comment 17).
But I don't think it's valid enough to block this.
Comment 10 Jonathan Druart 2020-05-18 14:17:28 UTC
Created attachment 105020 [details] [review]
Bug 25527: Initialize the logger when required

In an OO package, the logger initialization should happen in the
constructor. This is not an OO package and the initialization is
happening on loading it. This is a wrong behaviour and certainly breaks
in environments where initialization cannot happen (package building,
for example). There could be several options to solve this, as it is
used in a single sub on this package, I opted for initializing on that
sub.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2020-05-18 14:17:31 UTC
Created attachment 105021 [details] [review]
Bug 25527: Add logger to Koha::ExternalContent

This patch makes Koha::Logger initialization happen in the ->new method
for the Koha::ExternalContent-derived classes. In the case of RecordedBooks,
it doesn't look like it is used at all.

In the case of OverDrive, it will now use the Koha::ExternalContent
exported logger accessor.

I added tests for this addition to Koha::ExternalContent to the
OverDrive tests.

I also removed references to Test::DBIx::Class as it is not used at all.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha_ExternalContent_OverDrive.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Martin Renvoize 2020-05-18 14:29:18 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 13 Joy Nelson 2020-05-19 22:28:43 UTC
missing dependency - not backported to 19.11.x