Bug 7904 - SIP modules handling of @INC is confused
Summary: SIP modules handling of @INC is confused
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Colin Campbell
QA Contact:
URL:
Whiteboard:
Keywords: rel_3_16_candidate
Depends on: 13432
Blocks: 12820
  Show dependency treegraph
 
Reported: 2012-04-05 13:58 UTC by Colin Campbell
Modified: 2016-12-05 21:23 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Proposed patch (44.62 KB, patch)
2014-10-31 12:52 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 7904 - remove unnecessary path from SIP script (1.09 KB, patch)
2014-11-09 22:42 UTC, Robin Sheat
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7904 Change SIP modules to use standard LIB path (45.45 KB, patch)
2015-01-21 16:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7904 - remove unnecessary path from SIP script (1.16 KB, patch)
2015-01-21 16:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Rebase main path (44.69 KB, patch)
2015-01-21 17:03 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 7904 - remove unnecessary path from SIP script (1.21 KB, patch)
2015-01-28 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7904 Change SIP modules to use standard LIB path (44.80 KB, patch)
2015-01-28 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7904 Change SIP modules to use standard LIB path (44.80 KB, patch)
2015-01-28 10:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7904: (QA followup) fix tests (1.58 KB, patch)
2015-02-05 18:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2012-04-05 13:58:25 UTC
The modules which constitute the SIPserver use the same PERL5LIB as the rest of the system but they themselves use a non koha namespace which has to be passed to server (equivalent to setting PERL5LIB to $PERL5LIB/C4/SIP ). It should be more consistent which would make it easier to write tests for the modules and lessen need to write around this in startup scripts making startup and configuration less arcane.
Comment 1 Colin Campbell 2014-10-31 12:52:46 UTC Comment hidden (obsolete)
Comment 2 Colin Campbell 2014-10-31 12:56:40 UTC
Changes are small and apart from adding the modules back into the 00-load tests do not alter functionality. Quite a number of files are updated though. (including some which should probably be reloacated or removed but that should be done in separate commits)
Comment 3 Robin Sheat 2014-11-09 22:37:45 UTC
<huginn> Sent 5 days, 6 hours, and 39 minutes ago: <ashimema> Could you take a look at bug 7904 possibly.. I'd be interested to hear your view on how it may affect packages.

There'll be a minor change needed, but that's OK, it's a worthwhile fix.

I'll make a patch...
Comment 4 Robin Sheat 2014-11-09 22:42:03 UTC Comment hidden (obsolete)
Comment 5 Robin Sheat 2014-11-09 22:47:43 UTC
I haven't actually tested this myself (catching up on everything after a week away), so I'm hoping that it's simple enough that it just works. If I get time later today/this week...
Comment 6 Martin Renvoize 2014-11-12 10:19:23 UTC
Cheers Robin..

I'll try and promote some interest on list today.. see if we can get someone to signoff :)
Comment 7 Martin Renvoize 2015-01-21 16:41:35 UTC
Test Plan

This patch is all about plumbing.. basically having a working SIP server config before the patches, should work exactly the same after a service restart after the patches :)
Comment 8 Kyle M Hall 2015-01-21 16:56:24 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2015-01-21 16:56:32 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2015-01-21 16:57:44 UTC
Tested by launching sip server with just

./C4/SIP/SIPServer.pm /path/to/SIPconfig.xml

Used sip_cli_emulator to make some SIP requests, everything looks good!
Comment 11 Colin Campbell 2015-01-21 17:03:26 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-01-27 12:35:43 UTC
The qa script fails with

1/ tab chars
2/ n FAIL   C4/SIP/t/SIPtest.pm
   FAIL   valid
       Name "Sip::error_detection" used only once: possible typo
3/
 FAIL   C4/SIP/Sip/MsgType.pm
   FAIL   valid
      UNIVERSAL->import is deprecated and will be removed in a future perl

There are not valid, they existed before this patch set.

But t/00-load.t now fails:
% prove t/00-load.t
t/00-load.t .. UNIVERSAL->import is deprecated and will be removed in a future perl at C4/SIP/Sip/MsgType.pm line 22.
t/00-load.t .. 1/? Bailout called.  Further testing stopped:  ***** PROBLEMS LOADING FILE 'C4::SIP::SIPServer'

#   Failed test 'use C4::SIP::SIPServer;'
#   at t/00-load.t line 47.
#     Tried to use 'C4::SIP::SIPServer'.
#     Error:  Could not find 00-load.xml in t/ at C4/SIP/Sip/Configuration.pm line 32.
# Compilation failed in require at t/00-load.t line 47.
# BEGIN failed--compilation aborted at t/00-load.t line 47.
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 1.
FAILED--Further testing stopped: ***** PROBLEMS LOADING FILE 'C4::SIP::SIPServer'

To test, I did:
diff --git a/t/00-load.t b/t/00-load.t
index 7569606..238a349 100644
--- a/t/00-load.t
+++ b/t/00-load.t
@@ -43,7 +43,7 @@ find({
         $m =~ s{^.*/C4/}{C4/};
         $m =~ s{/}{::}g;
         return if $m =~ /Auth_with_ldap/; # Dont test this, it will fail on use
-        return if $m =~ /SIPServer/; # SIP Server module has old package usage
+        return unless $m =~ /SIPServer/; # SIP Server module has old package usage
         use_ok($m) || BAIL_OUT("***** PROBLEMS LOADING FILE '$m'");
     },
 }, $lib);

Otherwise, all looks good so far!

Marked as Failed QA.
Comment 13 Colin Campbell 2015-01-27 17:22:45 UTC
(In reply to Jonathan Druart from comment #12)
> The qa script fails with
> 
> 1/ tab chars
> 2/ n FAIL   C4/SIP/t/SIPtest.pm
>    FAIL   valid
>        Name "Sip::error_detection" used only once: possible typo
> 3/
>  FAIL   C4/SIP/Sip/MsgType.pm
>    FAIL   valid
>       UNIVERSAL->import is deprecated and will be removed in a future perl
> 
> There are not valid, they existed before this patch set.
> 
> But t/00-load.t now fails:
> % prove t/00-load.t
> t/00-load.t .. UNIVERSAL->import is deprecated and will be removed in a
> future perl at C4/SIP/Sip/MsgType.pm line 22.
> t/00-load.t .. 1/? Bailout called.  Further testing stopped:  ***** PROBLEMS
> LOADING FILE 'C4::SIP::SIPServer'
> 
> #   Failed test 'use C4::SIP::SIPServer;'
> #   at t/00-load.t line 47.
> #     Tried to use 'C4::SIP::SIPServer'.
> #     Error:  Could not find 00-load.xml in t/ at
> C4/SIP/Sip/Configuration.pm line 32.
> # Compilation failed in require at t/00-load.t line 47.
> # BEGIN failed--compilation aborted at t/00-load.t line 47.
> # Tests were run but no plan was declared and done_testing() was not seen.
> # Looks like your test exited with 255 just after 1.
> FAILED--Further testing stopped: ***** PROBLEMS LOADING FILE
> 'C4::SIP::SIPServer'
> 
> To test, I did:
> diff --git a/t/00-load.t b/t/00-load.t
> index 7569606..238a349 100644
> --- a/t/00-load.t
> +++ b/t/00-load.t
> @@ -43,7 +43,7 @@ find({
>          $m =~ s{^.*/C4/}{C4/};
>          $m =~ s{/}{::}g;
>          return if $m =~ /Auth_with_ldap/; # Dont test this, it will fail on
> use
> -        return if $m =~ /SIPServer/; # SIP Server module has old package
> usage
> +        return unless $m =~ /SIPServer/; # SIP Server module has old
> package usage
>          use_ok($m) || BAIL_OUT("***** PROBLEMS LOADING FILE '$m'");
>      },
>  }, $lib);
> 
> Otherwise, all looks good so far!
> 
> Marked as Failed QA.


Please use the current rebased version of patch which runs 00-load.t correctly on master because it skips SIPServer.pm which should not be tested, the change above will make it fail
Comment 14 Jonathan Druart 2015-01-28 08:59:07 UTC
(In reply to Colin Campbell from comment #13)
> Please use the current rebased version of patch which runs 00-load.t
> correctly on master because it skips SIPServer.pm which should not be
> tested, the change above will make it fail

Sorry for that, I misread the diff.
I thought you wanted to launch the test on all /SIPServer/  modules.

So, all is fine for me :)

Note: I am going to add the Kyle's signoff line on the first patch.
Comment 15 Jonathan Druart 2015-01-28 09:00:36 UTC
Created attachment 35571 [details] [review]
Bug 7904 - remove unnecessary path from SIP script

With the fixing of the namespace in the SIP code, we don't need to
modify the PERL5LIB to have the old one.

To test:
* do a package install using this and the other patches on bug 7904
* enable SIP
* make sure koha-start-sip and koha-stop-sip work

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 16 Jonathan Druart 2015-01-28 09:00:45 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-01-28 10:42:20 UTC
Created attachment 35574 [details] [review]
Bug 7904 Change SIP modules to use standard LIB path

For historical reasons the SIPServer and SIP modules
have used an extra module path in addition to the
standard Koha one. This has caused numerous irritants
in attempting to set up scripts and basic tests. It
does not help in attempting to modify or debug
this code

This patch changes the package value in the modules
under the C4/SIP directory and makes calls to
them use the full package name.

Where the export mechanism was being short circuited
routines have been explicitly exported and imported
declarations of 'use ILS' when that module was
not being used and which only generated warnings
have been removed.

As a lot of the changes affect lines where
an object is instantiated with new. The opportunity
has been taken to replace the ambiguous indirect
syntax with the preferred direct call

In intializing ILS the full path is added as this
will not require any changes to existing configs.
I suspect this feature is unused, and adds
obfuscation rather than flexibility but have kept
the feature as we need this change in order to
rationalize and extend the testing of the server.

The visible difference is that with the normal Koha
PERL5LIB setting. Compilation of Modules under C4/SIP
should be successful and not fail with unlocated modules,
allowing developers to see any perl warnings

All the SIP modules can now be run through the tests
in t/00-load.t now except for SIPServer itself

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 18 Jonathan Druart 2015-01-28 10:43:06 UTC
I reuploaded a patch based on top of bug 13432 (since it's a bugfix and should be pushed before).
Comment 19 Tomás Cohen Arazi 2015-02-05 17:58:25 UTC
Patches pushed to master.

Thanks Robin and Colin!
Comment 20 Tomás Cohen Arazi 2015-02-05 18:52:55 UTC
Created attachment 35692 [details] [review]
Bug 7904: (QA followup) fix tests

The package name for SIP wasn't fixed in the tests by the original patches.

This patch fixes it.

To test:
- Run
  $ prove t/db_dependent/SIP_ILS.t
- Tests should pass with the patch.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 21 Colin Campbell 2015-02-06 10:19:36 UTC
As a footnote the reason SIPServer.pm's package statement was not changed was it is not a module.(nothing uses or requires it) 
The name does create that illusion and I think (like a few other things) stems from some confusion in the initial implementation. I think a separate bug should open renaming it to avoid confusion and moving it out of its current location in the library modules.
Comment 22 Chris Cormack 2015-05-20 22:25:03 UTC
Pushed to 3.18.x will be in 3.18.7
Comment 23 Mason James 2015-05-27 22:36:37 UTC
Pushed to 3.16.x, will be in 3.16.11