I tested the solr work at kohacon12 and it worked very well.

I note that this patch allows us to toggle the syspref and use solr, and toggle it back and be back using zebra.

Currently it only effects the opac search, but that is a great place to start. I see no harm in getting this into master as soon as possible so others can work on the Koha::SearchEngine::Zebra more easily. Especially since if you do not choose solr this has no affect on your current system. Exactly the way it should be.

I have signed off, and will now send a couple of follow ups to fix updatedatabase and few unconditional warns.

But I would urge people to not fail QA on those issues, it is more important to get this up so it can be tested by lots of people.. now while we still have 4 months before the 3.10.0 release.

Actually the warns are only in the Koha::SearchEngine::Zebra::* area, which is totally still being developed, so I think leaving them unqualified for now is ok and as that code is fleshed out and developed we can remove them.

(In reply to comment #6)
> Actually the warns are only in the Koha::SearchEngine::Zebra::* area, which
> is totally still being developed, so I think leaving them unqualified for
> now is ok and as that code is fleshed out and developed we can remove them.

For what it's worth, I think unconditional warns in Koha::SearchEngine::Zebra::* are a good idea.

(In reply to comment #7)
> (In reply to comment #6)
> > Actually the warns are only in the Koha::SearchEngine::Zebra::* area, which
> > is totally still being developed, so I think leaving them unqualified for
> > now is ok and as that code is fleshed out and developed we can remove them.
> 
> For what it's worth, I think unconditional warns in
> Koha::SearchEngine::Zebra::* are a good idea.

It *is* a good idea. Sorry, I haven't eaten lunch yet and apparently that's affecting my grammar.

*** Bug 5360 has been marked as a duplicate of this bug. ***

Display of Solr configuration page in Admin is unconditional; will show if Zebra is the search engine... not sure if that's a bad thing or not.

opac/opac-search.pl makes use of "highly experimental" Perl syntax: given and when.  http://perldoc.perl.org/perlsyn.html#Switch-Statements.  While this should be compatible with Koha's minimum Perl version (5.10), it's still listed as experimental in 5.16, which makes me nervous.  I'm all for switch statements, but they're not as fundamental in Perl as they are in other languages, and for an either/or situation like this, doing it another way would be safer.

Reintroduces Test::MockModule

Given the above, I'm not ready to pass this through QA, but I'm not entirely convinced it should fail, either.  Replacing "given" with "for", and adding a 'default' case should clear up the issue.

Otherwise, all the code is new and in it's own area, making it delightfully low-impact on folks who aren't ready to experiment with Solr support.

Hi Ian, 
Thanks for your review.

About Switch vs given statements, Switch is buggy and deprecated : "Switch is buggy and should be avoided. You may find Perl's new given /when feature a suitable replacement."

http://perldoc.perl.org/perl5120delta.html#Deprecations

Right, I wasn't advocating for Switch itself, so much as supporting the syntactic structure of switch that can be found in many programming languages.  Looks like Perl does this most stably (right now) with this structure:

for ($var) {
  when (/^abc/) { $abc = 1 }
  when (/^def/) { $def = 1 }
  when (/^xyz/) { $xyz = 1 }
  default { $nothing = 1 }
}

and, "highly experimentally", with

given ($var) {
  when (/^abc/) { $abc = 1 }
  when (/^def/) { $def = 1 }
  when (/^xyz/) { $xyz = 1 }
  default { $nothing = 1 }
}

While I'm sure using given would in most cases be safe, it could very well introduce a difficult-to-trace problem for a non-typical install (where a different Perl version could have snuck in).  I'm probably being overly paranoid, but that's what a QAM is for!

For replacing the when, and hiding the configuration both work well. Back to signed off

QA comments have been addressed in followup patches, thank you!

One additional issue not caught by earlier scans:  the new modules introduce dependencies on Moose (and sub-modules), Test::MockModule and Data::Pagination, none of which are found on my default install.  These dependencies will need to be added to koha_perl_deps.pl and the Debian packages lists (assuming they are indeed packaged).

Please provide this additional followup, and then we should safe to push this to master, and debug further from there.

Hi Ian, 

I don't know if we want to add all solr's dependencies for koha currently.

If I add Data::Pagination, I would have to add all of dependencies listed on http://wiki.koha-community.org/wiki/SearchEngine_Layer_RFC#Install_perl_libs.

And I think it is not what we want for Zebra's installations, isn't it ?

Maybe we want a test in C4::Installer::PerlDependencies to distinguish the search engine ?

The dependencies can be marked as optional and for what purpose, so if folks don't want to install them, they don't need to.

A more complex set up of looking install options and picking dependencies based on that would be nice, but probably more work than we need right now, with our level of available customization.  But, say when Koha is RDBMS-agnostic, then we may find it handy to have different dependency paths in the scripts.

(In reply to comment #20)
> Please provide this additional followup, and then we should safe to push
> this to master, and debug further from there.

Follow-up provided, I agree it passes QA now, so pushing !

Checked again that I can't see any difference when we're running zebra

not ok 125 - use Koha::SearchEngine::Zebra;
#   Failed test 'use Koha::SearchEngine::Zebra;'
#   at t/00-load.t line 46.
#     Tried to use 'Koha::SearchEngine::Zebra'.
#     Error:  Can't locate Data/SearchEngine/Zebra.pm in @INC (@INC contains: /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .) at /usr/share/perl5/Module/Runtime.pm line 205.
#  at /usr/lib/perl5/Moose.pm line 63
# Compilation failed in require at (eval 1313) line 2.
# BEGIN failed--compilation aborted at (eval 1313) line 2.
Bail out!  ***** PROBLEMS LOADING FILE 'Koha::SearchEngine::Zebra'
# Tests were run but no plan was declared and done_testing() was not seen.

Is what is causing jenkins to complain, I will fix it on jenkins but we probably need a patch to fix it properly pretty soon.

Ok that's fixed (missing modules, easy fix)

However now I am getting this

not ok 129 - use Koha::SearchEngine::Solr;
#   Failed test 'use Koha::SearchEngine::Solr;'
#   at t/00-load.t line 46.
#     Tried to use 'Koha::SearchEngine::Solr'.
#     Error:  Illegal inherited options => (is) at /usr/lib/perl5/Moose/Meta/Class.pm line 646.
#       Moose::Meta::Class::_process_inherited_attribute('Moose::Meta::Class=HASH(0x5a38b40)', 'url', 'required', 1, 'builder', '_build_url', 'isa', 'Str', 'definition_context', 'HASH(0x5e9bba8)', 'lazy', 1, 'is', 'ro') called at /usr/lib/perl5/Moose/Meta/Class.pm line 628
#       Moose::Meta::Class::_process_attribute('Moose::Meta::Class=HASH(0x5a38b40)', '+url', 'required', 1, 'builder', '_build_url', 'isa', 'Str', 'definition_context', 'HASH(0x5e9bba8)', 'lazy', 1, 'is', 'ro') called at /usr/lib/perl5/Moose/Meta/Class.pm line 300
#       Moose::Meta::Class::add_attribute('Moose::Meta::Class=HASH(0x5a38b40)', '+url', 'required', 1, 'builder', '_build_url', 'isa', 'Str', 'definition_context', 'HASH(0x5e9bba8)', 'lazy', 1, 'is', 'ro') called at /usr/lib/perl5/Moose.pm line 70
#       Moose::has('Moose::Meta::Class=HASH(0x5a38b40)', '+url', 'is', 'ro', 'isa', 'Str', 'lazy', 1, 'builder', '_build_url', 'required', 1) called at /usr/lib/perl5/Moose/Exporter.pm line 294
#       Moose::has('+url', 'is', 'ro', 'isa', 'Str', 'lazy', 1, 'builder', '_build_url', 'required', 1) called at Koha/SearchEngine/Solr.pm line 7
#       require Koha/SearchEngine/Solr.pm called at (eval 1428) line 2
#       main::BEGIN() called at Koha/SearchEngine/Solr.pm line 0
#       eval {...} called at Koha/SearchEngine/Solr.pm line 0
#       eval 'package main;
# use Koha::SearchEngine::Solr @{$args[0]};
# 1;
#
# ;' called at /usr/local/share/perl/5.10.1/Test/More.pm line 885
#       Test::More::_eval('package main;\x{a}use Koha::SearchEngine::Solr @{$args[0]};\x{a}1;\x{a}', 'ARRAY(0x3f55558)') called at /usr/local/share/perl/5.10.1/Test/More.pm line 860
#       Test::More::use_ok('Koha::SearchEngine::Solr') called at t/00-load.t line 46
#       main::__ANON__() called at /usr/share/perl/5.10/File/Find.pm line 958
#       File::Find::_find_dir('HASH(0x8c2f60)', '/home/jenkins/jobs/Koha_master/workspace/Koha', 7) called at /usr/share/perl/5.10/File/Find.pm line 722
#       File::Find::_find_opt('HASH(0x8c2f60)', '/home/jenkins/jobs/Koha_master/workspace/Koha') called at /usr/share/perl/5.10/File/Find.pm line 1297
#       File::Find::find('HASH(0x8c2f60)', '/home/jenkins/jobs/Koha_master/workspace/Koha') called at t/00-load.t line 49
# Compilation failed in require at (eval 1428) line 2.
# BEGIN failed--compilation aborted at (eval 1428) line 2.
Bail out!  ***** PROBLEMS LOADING FILE 'Koha::SearchEngine::Solr'

>#     Error:  Can't locate Data/SearchEngine/Zebra.pm in @INC
> Ok that's fixed (missing modules, easy fix)

Hi Chris!
How have you fixed that ?
I can't reproduce your second error:

% export PERL5LIB=/home/koha/src

% prove t/00-load.t
t/00-load.t .. 15/? Too late to run INIT block at /home/koha/src/C4/Barcodes/hbyymmincr.pm line 41.
Too late to run INIT block at /home/koha/src/C4/External/BakerTaylor.pm line 42.

#   Failed test 'use Koha::SearchEngine::Zebra;'
#   at t/00-load.t line 46.
#     Tried to use 'Koha::SearchEngine::Zebra'.
#     Error:  Can't locate Data/SearchEngine/Zebra.pm in @INC (@INC contains: /home/koha/src /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib
[...]
/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .). at /usr/lib/perl5/Class/MOP.pm line 135
Bailout called.  Further testing stopped:  ***** PROBLEMS LOADING FILE 'Koha::SearchEngine::Zebra'
# Tests were run but no plan was declared and done_testing() was not seen.
FAILED--Further testing stopped: ***** PROBLEMS LOADING FILE 'Koha::SearchEngine::Zebra'

I get the same error. But if I export the good path:

% git clone git://github.com/xercode/Data-SearchEngine-Zebra.git /home/koha/Data-SearchEngine-Zebra
% export PERL5LIB="/home/koha/src:/home/koha/Data-SearchEngine-Zebra"
% prove t/00-load.t
t/00-load.t .. 1/? Too late to run INIT block at /home/koha/src/C4/Barcodes/hbyymmincr.pm line 41.
Too late to run INIT block at /home/koha/src/C4/External/BakerTaylor.pm line 42.
t/00-load.t .. ok     
All tests successful.
Files=1, Tests=148,  1 wallclock secs ( 0.04 usr  0.00 sys +  1.00 cusr  0.06 csys =  1.10 CPU)
Result: PASS

Yes, I fixed the first error by adding the missing module to the server that jenkins is running on.

It still is totally busted on Jenkins, with that second error, and builds won't continue until it is fixed.

I suspect the version of Moose, or MooseX::Something is too low on debian squeeze.
Will investigate more

Yes, it's moose is too old in debian squeeze. We should test this in wheezy, to make sure it will work with the next release.

But jenkins can run the tests again now.

This new code introduces some rather serious problems. The packages required for Solr do not work on Squeeze, which means that t/00-load.t bails out without some serious black magic. Since "all tests should pass" is supposed to be one of the things that people who are signing off and doing QA are checking, this is a huge problem. In order to work around it when signing off, I ran relevant tests individually, but that is not a long-term solution, and certainly will not be a viable option when it comes time to actually release 3.10. Perhaps the packaging manager could be told which modules need to be packaged for Squeeze, so that we could get Koha working again?

Jared, you're right. With Robin at Kohacon12, we did some packages I put here for the moment (except xml::easy wich is a core lib) https://depot.biblibre.com/chernandez/lib/all/ you can give it a try. To be really sure, I would need to test on a fresh install. I'll have a look.

(In reply to comment #31)
> This new code introduces some rather serious problems. The packages required
> for Solr do not work on Squeeze, which means that t/00-load.t bails out
> without some serious black magic. Since "all tests should pass" is supposed
> to be one of the things that people who are signing off and doing QA are
> checking, this is a huge problem. In order to work around it when signing
> off, I ran relevant tests individually, but that is not a long-term
> solution, and certainly will not be a viable option when it comes time to
> actually release 3.10. Perhaps the packaging manager could be told which
> modules need to be packaged for Squeeze, so that we could get Koha working
> again?

As solr stuff is experimental, couldn't we just skip Koha::SearchEngine::Solr for now, like what is done for cache:
return if $m =~ /Cache/; # Cache modules are a WIP, add the tests back when we are using them more

?

QA comment: tiny follow-up to please jenkins, passed QA

(but I agree it's a workaround, valid because solr is experimental)

Please sign so we can have .deb packages.

Created attachment 12408 [details] [review]
Bug 8233 - SolR-related /etc/ files break deb building

Remove them using debian/rules when building .deb packages until
we figure out what to do with them in a .deb install.

To do this I ran the build script like this:

sudo DEB_BUILD_OPTIONS=nocheck ./debian/build-git-snapshot -distribution precise -D precise -r ~/ubuntu -v 3.9.0-046~git -d

Sponsored-by: Universidad Nacional de Córdoba
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

I didn't test but the code seems safe

(In reply to comment #39)
> Created attachment 12408 [details] [review]
> Bug 8233 - SolR-related /etc/ files break deb building
> 
> Remove them using debian/rules when building .deb packages until
> we figure out what to do with them in a .deb install.
> 
> To do this I ran the build script like this:
> 
> sudo DEB_BUILD_OPTIONS=nocheck ./debian/build-git-snapshot -distribution
> precise -D precise -r ~/ubuntu -v 3.9.0-046~git -d
> 
> Sponsored-by: Universidad Nacional de Córdoba
> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>


i'm happy to pass QA on this patch


$ koha-qa.pl -c 1

testing 1 commit(s) (applied to commit 0a35b26)
 * 626c660 Bug 8233 - SolR-related /etc/ files break deb building
      debian/rules

follow-up Bug 8233 - SolR-related /etc/ files break deb building pushed to master

Question:

Attachment #10480 [details] brings in the follwoing dependency: 
+    'JSON::Any' => {
+        'usage'    => 'Core',
+        'required' => '0',
+        'min_ver'  => '1.28',
+    },

JSON::Any seems to be deprecated, see:
http://search.cpan.org/~perigrin/JSON-Any-1.29/lib/JSON/Any.pm#DEPRECATION

Where is it used?

Marc

(In reply to comment #43)
> Question:
> 
> Attachment #10480 [details] brings in the follwoing dependency: 
> +    'JSON::Any' => {
> +        'usage'    => 'Core',
> +        'required' => '0',
> +        'min_ver'  => '1.28',
> +    },
> 
> JSON::Any seems to be deprecated, see:
> http://search.cpan.org/~perigrin/JSON-Any-1.29/lib/JSON/Any.pm#DEPRECATION
> 
> Where is it used?
> 
> Marc

Hi Marc,

JSON::Any is recommended by MooseX::Storage

aptitude show libmoosex-storage-perl
Depend: perl, libmoose-perl (>= 0.99), libstring-rewriteprefix-perl
Recommande: libjson-any-perl, libyaml-perl

I think it is why I added it

Released in 3.10.0