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.
Created attachment 33057 [details] [review] Proposed patch This changes the package declarations and use statements and adds the module back into t/00-load so that we can check at least that this stuff compiles. Some kludgy interfaces remain (notably in MsgType) but we should now be able to address these issues and add tests to verify our solutions
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)
<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...
Created attachment 33400 [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
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...
Cheers Robin.. I'll try and promote some interest on list today.. see if we can get someone to signoff :)
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 :)
Created attachment 35447 [details] [review] [SIGNED-OFF] 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>
Created attachment 35448 [details] [review] [SIGNED-OFF] 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>
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!
Created attachment 35449 [details] [review] Rebase main path rebased original patch against current master
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.
(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
(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.
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>
Created attachment 35572 [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>
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>
I reuploaded a patch based on top of bug 13432 (since it's a bugfix and should be pushed before).
Patches pushed to master. Thanks Robin and Colin!
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>
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.
Pushed to 3.18.x will be in 3.18.7
Pushed to 3.16.x, will be in 3.16.11