[Bug 7904] New: SIP modules handling of @INC is confused
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Priority: P5 - low Change sponsored?: --- Bug ID: 7904 Assignee: colin.campbell@ptfs-europe.com Summary: SIP modules handling of @INC is confused QA Contact: koha.sekjal@gmail.com Severity: enhancement Classification: Unclassified OS: All Reporter: colin.campbell@ptfs-europe.com Hardware: All Status: NEW Version: master Component: SIP2 Product: Koha 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #1 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 33057 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33057&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Medium patch --- Comment #2 from Colin Campbell <colin.campbell@ptfs-europe.com> --- 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) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |robin@catalyst.net.nz --- Comment #3 from Robin Sheat <robin@catalyst.net.nz> --- <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... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #4 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 33400 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33400&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #5 from Robin Sheat <robin@catalyst.net.nz> --- 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... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Cheers Robin.. I'll try and promote some interest on list today.. see if we can get someone to signoff :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33057|0 |1 is obsolete| | Attachment #33400|0 |1 is obsolete| | --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35447 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35447&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35448 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35448&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com --- Comment #10 from 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! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35447|0 |1 is obsolete| | --- Comment #11 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 35449 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35449&action=edit Rebase main path rebased original patch against current master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@biblibre.co | |m --- Comment #12 from Jonathan Druart <jonathan.druart@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #13 from Colin Campbell <colin.campbell@ptfs-europe.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA --- Comment #14 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35448|0 |1 is obsolete| | Attachment #35449|0 |1 is obsolete| | --- Comment #15 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 35571 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35571&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #16 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 35572 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35572&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |13432 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35572|0 |1 is obsolete| | --- Comment #17 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 35574 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35574&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #18 from 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). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|Passed QA |Pushed to Master --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Thanks Robin and Colin! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 35692 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35692&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 --- Comment #21 from Colin Campbell <colin.campbell@ptfs-europe.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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |12820 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris@bigballofwax.co.nz Status|Pushed to Master |Pushed to Stable Severity|enhancement |normal --- Comment #22 from Chris Cormack <chris@bigballofwax.co.nz> --- Pushed to 3.18.x will be in 3.18.7 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_3_16_candidate CC| |mtj@kohaaloha.com --- Comment #23 from Mason James <mtj@kohaaloha.com> --- Pushed to 3.16.x, will be in 3.16.11 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7904 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14151 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org