Bug 612719 - Review Request: recoll - Desktop full text search tool with a qt gui
Summary: Review Request: recoll - Desktop full text search tool with a qt gui
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ankur Sinha (FranciscoD)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
: 590473 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-07-08 20:47 UTC by Terje Røsten
Modified: 2015-01-04 21:55 UTC (History)
6 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2010-08-10 20:30:47 UTC
sanjay.ankur: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Terje Røsten 2010-07-08 20:47:45 UTC
spec: http://terjeros.fedorapeople.org/recoll/recoll.spec
srpm: http://terjeros.fedorapeople.org/recoll/recoll-1.13.04-3.fc12.src.rpm
description:
Recoll is a personal full text search package for Linux, FreeBSD and
other Unix systems. It is based on a very strong backend (Xapian), for
which it provides an easy to use, feature-rich, easy administration
interface.

Comment 1 Terje Røsten 2010-07-08 20:51:01 UTC
*** Bug 590473 has been marked as a duplicate of this bug. ***

Comment 2 Ankur Sinha (FranciscoD) 2010-07-09 07:06:32 UTC
A preliminary rpmlint check:


[Ankur@localhost SPECS]$ rpmlint recoll.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[Ankur@localhost SPECS]$ rpmlint ../SRPMS/recoll-1.13.04-3.fc12.src.rpm 
recoll.src: W: spelling-error Summary(en_US) gui -> GUI, goo, gi
recoll.src: W: spelling-error %description -l en_US backend -> backed, backbend, back end
recoll.src: W: invalid-url URL: http://www.recoll.org/ <urlopen error timed out>
1 packages and 0 specfiles checked; 0 errors, 3 warnings.


Small issues, worth correcting though. I'll do a detailed review in the next 24 hours.

Ankur

Comment 4 Ankur Sinha (FranciscoD) 2010-07-09 18:42:28 UTC
I was on my way to a formal review when I noticed some things that are worth clarifying..

wrt
https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

1. I see a unac directory with a "stripped down version of unac". You need to package unac separately and add it as a build requires IMO. 

2. I also see xdg-utils bundled with the source. What is the purpose of this? You need to use the system wide package, not a local copy. 

These are only two that I've found yet. 

Will you please contact upstream and work these issues out? (and also check for other packages that have been used locally and provided with the tar)

regards,
Ankur

Comment 5 Terje Røsten 2010-07-09 23:28:56 UTC
Ok, Ankur. 

I will have a look, thanks for the help so far.

Comment 6 Jean-Francois Dockes 2010-07-10 06:53:28 UTC
 > 1. I see a unac directory with a "stripped down version of unac". You need to
 > package unac separately and add it as a build requires IMO. 

Parts of unac are significantly modified for Recoll use. The comment is misleading, this is not just a "stripped down version of unac". I fixed the README (for the next version). Recoll could not use the standard package.

You can find the new README.recoll file at the following link. I hope this
clarifies things:  http://bitbucket.org/medoc/recoll/src/tip/unac/

 > 2. I also see xdg-utils bundled with the source. What is the purpose of this?
 > You need to use the system wide package, not a local copy. 

xdg-utils is bundled because it was found useful on some platform (don't
remember which), probably because of a buggy system version. Recoll
installs a copy of xdg-open to $prefix/share/recoll/filters/ If this is a problem, can it be removed by the rpm after the install step ?
Recoll will use the system version then.

 > These are only two that I've found yet. 
 > 
 > Will you please contact upstream and work these issues out? (and also
 > check for other packages that have been used locally and provided with
 > the tar) 

There is also a partial and modified copy of binc imap. As
far as I know this is not packaged on Fedora and not available as a
library, so including the code is the only way to reuse it.

Comment 7 Terje Røsten 2010-07-10 12:14:51 UTC
Updated package:

- use system xdg-open
- trim changelog

spec: http://terjeros.fedorapeople.org/recoll/recoll.spec
srpm: http://terjeros.fedorapeople.org/recoll/recoll-1.13.04-5.fc12.src.rpm

I don't see the unac issue as a blocker it' simple application with low risk, upstream seems dead too.

what is recoll using binc for? why is imap _server_ code needed in recoll?

Comment 8 Jean-Francois Dockes 2010-07-10 14:07:34 UTC
Recoll just includes the rfc 822 + mime analysis part. The code is both compact and proven robust (imap _server_). The daemon, network, and protocol parts are not in there. A few years ago no email lib that I looked at would fit the bill (either big and full of deps or unreliable/unproven). There has been a few modifications/fixes to the original code to fit Recoll needs, but less than in the unac part.

Comment 9 Ankur Sinha (FranciscoD) 2010-07-11 05:54:07 UTC
(In reply to comment #7)
> Updated package:
> 
> - use system xdg-open
> - trim changelog
> 
> spec: http://terjeros.fedorapeople.org/recoll/recoll.spec
> srpm: http://terjeros.fedorapeople.org/recoll/recoll-1.13.04-5.fc12.src.rpm
> 
> I don't see the unac issue as a blocker it' simple application with low risk,
> upstream seems dead too.
> 
> what is recoll using binc for? why is imap _server_ code needed in recoll?    

hi,

I don't think the unac issue is a blocker, but I will confirm with the devel list first. I'll ping back when I get something.

regards,
Ankur

Comment 10 Ankur Sinha (FranciscoD) 2010-07-13 11:48:48 UTC
hi,

I got this from -devel

>I think it would be best to talk to upstream of this project and
> see if there is any chance of them upstreaming any of their patches for
> these projects. If not, then getting to admit that they are forking
> them and only using them internally would at least be good to know. 

It looks like it's okay. Formal review coming up!

+ OK
? ISSUE
- NA

=================================================================================

+ Package meets naming and packaging guidelines
+ Spec file matches base package name.
+ Spec has consistant macro usage.
+ Meets Packaging Guidelines.
+ License
+ License field in spec matches
+ License file included in package
+ Spec in American English
+ Spec is legible.
+ Sources match upstream md5sum:
[Ankur@localhost rpmbuild]$ md5sum recoll-1.13.04.tar.gz SOURCES/recoll-1.13.04.tar.gz 
eba9d639dbac149996a92b9b2dffc5ee  recoll-1.13.04.tar.gz
eba9d639dbac149996a92b9b2dffc5ee  SOURCES/recoll-1.13.04.tar.gz


- Package needs ExcludeArch
+ BuildRequires correct
- Spec handles locales/find_lang
- Package is relocatable and has a reason to be.
+ Package has %defattr and permissions on files is good.
+ Package has a correct %clean section.
+ Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+ Package is code or permissible content.
- Doc subpackage needed/used.
+ Packages %doc files don't affect runtime.

- Headers/static libs in -devel subpackage.
- Spec has needed ldconfig in post and postun
- .pc files in -devel subpackage/requires pkgconfig
- .so files in -devel subpackage.
- -devel package Requires: %{name} = %{version}-%{release}
- .la files are removed.

+ Package is a GUI app and has a .desktop file

? Package compiles and builds on at least one arch.
+ Package has no duplicate files in %files.
+ Package doesn't own any directories other packages own.
+ Package owns all the directories it creates.
- No rpmlint output.
- final provides and requires are sane:
(include output of for i in *rpm; do echo $i; rpm -qp --provides $i; echo =; rpm -qp --requires $i; echo; done
manually indented after checking each line.  I also remove the rpmlib junk and anything provided by glibc.)

SHOULD Items:

? Should build in mock.
? Should build on all supported archs
? Should function as described.
+ Should have sane scriptlets.
- Should have subpackages require base package with fully versioned depend.
+ Should have dist tag
+ Should package latest version
- check for outstanding bugs on package. (For core merge reviews)

Issues:

1. I wasn't able to build it for rawhide. You'll have to correct this, since the first branch you'll build in will be *rawhide*.

2. I haven't been able to build it correctly, and so haven't tested it's working.

No rpmlint output on the spec and the source. 


A few issues, but should be small enough to quickly be solved. 


Ankur

Comment 11 Jean-Francois Dockes 2010-07-14 20:51:58 UTC
To correct the build failure on rawhide: recoll needs a small patch (which fixes some include files) to build with the new xapian 1.2:
http://www.lesbonscomptes.com/recoll/files/xapian12.patch

It then works for me (at least build, indexing and command line query, couldn't test the GUI as my rawhide's X11 seems to be broken).

Comment 12 Terje Røsten 2010-07-14 20:57:56 UTC
Thanks Jegan-Francois,

is the patch rawhide only or does it work for F-12 and F-13 too?

Comment 13 Jean-Francois Dockes 2010-07-14 21:13:50 UTC
It's necessary to compile with the latest Xapian, but doesn't break compatibility with older Xapian versions, so you can use it everywhere.

Comment 15 Ankur Sinha (FranciscoD) 2010-07-31 12:50:58 UTC
Okay, 

Builds with mock. 

I do recall some discussion about the code reuse, Jean, can you please update us on it? Do we need a FESCO exception etc. here?

If not, I'll approve the package. 

Ankur

Comment 16 Jean-Francois Dockes 2010-08-05 21:33:57 UTC
The exception for recoll library bundling (which I'll persist in calling code reuse :) ) was granted by Fesco on july 20:

http://lists.fedoraproject.org/pipermail/devel/2010-July/139191.html

Comment 17 Ankur Sinha (FranciscoD) 2010-08-07 14:04:10 UTC
Ah,

I've been a little off then :P

XXX APPROVED XXX

Comment 18 Terje Røsten 2010-08-09 11:59:17 UTC
Thanks guys!


New Package SCM Request
=======================
Package Name: recoll
Short Description: Desktop full text search tool with Qt GUI
Owners: terjeros
Branches: F13 F14
InitialCC:

Comment 19 Kevin Fenzi 2010-08-09 17:24:44 UTC
Git done (by process-git-requests).

Comment 20 Terje Røsten 2010-08-09 18:46:00 UTC
Sorry, forgot F12 branch.


New Package SCM Request
=======================
Package Name: recoll
Short Description: Desktop full text search tool with Qt GUI
Owners: terjeros
Branches: F12
InitialCC:

Comment 21 Jason Tibbitts 2010-08-09 18:56:54 UTC
You should file a change request if the package already exists.

Comment 22 Terje Røsten 2010-08-10 06:59:11 UTC
Thanks for the tip, Jason.


Package Change Request
======================
Package Name: recoll 
New Branches: F12
Owners: terjeros
InitialCC:

Forgot F12 branch in the initial request.

Comment 23 Kevin Fenzi 2010-08-10 17:56:33 UTC
Git done (by process-git-requests).

Comment 24 Terje Røsten 2010-08-10 20:30:47 UTC
Thanks again, Kevin.

recoll imported, built and pushed for all active branches.

Comment 25 Paolo Bonzini 2011-12-19 10:36:54 UTC
Package Change Request
======================
Package Name: recoll 
New Branches: el6
Owners: bonzini
InitialCC:

Comment 26 Gwyn Ciesla 2011-12-19 13:01:33 UTC
Git done (by process-git-requests).

Comment 27 Bojan Smojver 2015-01-04 21:55:55 UTC
Any chance of getting an EPEL7 branch/build?


Note You need to log in before you can comment on or make changes to this bug.