Bug 25527 - Package does not build because of missing log4perl.conf
Summary: Package does not build because of missing log4perl.conf
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 25172
Blocks: 25529
  Show dependency treegraph
 
Reported: 2020-05-18 12:06 UTC by Jonathan Druart
Modified: 2020-05-19 22:28 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 25527: Initialize the logger when required (1.34 KB, patch)
2020-05-18 12:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25527: Initialize the logger in the constructor in ExternalContent (3.25 KB, patch)
2020-05-18 12:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25527: Add logger to Koha::ExternalContent (6.25 KB, patch)
2020-05-18 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25527: Initialize the logger when required (1.40 KB, patch)
2020-05-18 13:56 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25527: Add logger to Koha::ExternalContent (6.30 KB, patch)
2020-05-18 13:57 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25527: Initialize the logger when required (1.47 KB, patch)
2020-05-18 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25527: Add logger to Koha::ExternalContent (6.37 KB, patch)
2020-05-18 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review

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