Bug 223586

Created attachment 147667 [details]
Mock build log of strigi-0.3.11-1.fc7

Mock build log of strigi-0.3.11-1 on FC7 i386 is attached.

Well,
* compiler flags
  - does not pass fedora specific compilation flags.

* conditional dependency
  - Check:
------------------------------------------
-- Xerces-C was not found.
------------------------------------------

* desktop file
  - /usr/bin/strigiclient seems to be a GUI program
    and desktop file for this program should be added,
    perhaps.

* File location
  - All header files under /usr/include in -devel package
    should be moved to %{_includedir}/%{name}. From I
    checked the #include entry, this is no problem and
    putting header files directly under /usr/include should
    be avoied.

* Dependency
  - Check the dependency for -devel package. I have never
    checked the header files for Qt4 package. However,
    /usr/include/strigi/qtdbus/strigidbus.h includes:
---------------------------------------------
#include <QtCore/QByteArray>
---------------------------------------------
    This means that -devel package should require
    some Qt4 related packages.

(In reply to comment #1)

> * compiler flags
>   - does not pass fedora specific compilation flags.
Well, i did pass it, but the cmake stuff refused to use it, will check if I can
force it to.

> 
> * conditional dependency
>   - Check:
> ------------------------------------------
> -- Xerces-C was not found.
> ------------------------------------------
Must have missed this one, will fix.
> 
> * desktop file
>   - /usr/bin/strigiclient seems to be a GUI program
>     and desktop file for this program should be added,
>     perhaps.
 No, there's a separate package (stigiapplet), that provides GUI support for
strigi as an applet. It is also integrated with konqueror and can thus
optionally be used from it (using some form of strigi:// protocol)
> 
> * File location
>   - All header files under /usr/include in -devel package
>     should be moved to %{_includedir}/%{name}. From I
>     checked the #include entry, this is no problem and
>     putting header files directly under /usr/include should
>     be avoied.
I don't really understand what you're saying here, but i get the idea, will look
into fixing it.

> * Dependency
>   - Check the dependency for -devel package. I have never
>     checked the header files for Qt4 package. However,
>     /usr/include/strigi/qtdbus/strigidbus.h includes:
> ---------------------------------------------
> #include <QtCore/QByteArray>
> ---------------------------------------------
Strigi also optionally depends on dbus-qt; but at the time I first packaged and
put it up, dbus-qt was not available in fedora (devel) repo. Now that it
(dbus-qt) is available, I'll consider adding it as buildrequire.

>     This means that -devel package should require
>     some Qt4 related packages.
> 

(In reply to comment #2)
> > * desktop file
> >   - /usr/bin/strigiclient seems to be a GUI program
> >     and desktop file for this program should be added,
> >     perhaps.
>  No, there's a separate package (stigiapplet), that provides GUI support for
> strigi as an applet. It is also integrated with konqueror and can thus
> optionally be used from it (using some form of strigi:// protocol)

Well, I know that you are thinking of KDE. And I am thinking
of GNOME, actually.

When I check KDE application, I also check if this can be used
for GNOME and, for this package it can be used in GNOME as far as
I tried (well, I want to use KDE application on GNOME usually!!)
So I think the desktop for this package should be
added for GNOME user.

Actually, strigiclient is not a search frontend, it's more like a daemon
configuration tool. I think it should have a desktop file and be in the menu (in
the configuration submenu maybe ?)

(In reply to comment #4)
> Actually, strigiclient is not a search frontend, it's more like a daemon
> configuration tool. I think it should have a desktop file and be in the menu (in
> the configuration submenu maybe ?)

Oh.. I misunderstood. Maybe "System" or something like would
be good.

> Strigi also optionally depends on dbus-qt; but at the time I first packaged
> and put it up, dbus-qt was not available in fedora (devel) repo. Now that it
> (dbus-qt) is available, I'll consider adding it as buildrequire.

dbus-qt is for Qt 3, if you mean the Qt 4 D-Bus bindings, these are part of the 
qt4 package.

Also, doesn't Strigi also include a KDE 3 frontend? Why is there none of the 
KDE 3 (nor Qt 3) libs in the BuildRequires?

Oh, if the reason was that the KDE 3 frontend needs the Qt 3 dbus-qt, then the 
puzzle pieces match up. ;-) Still, now that dbus-qt is available again, it can 
probably be enabled.

I don't know well about this, however, according to
README, this program can use hyperestraier, of which the review
request I submitted (bug 229647).
hyperestraier depends on qdbm (bug 229478)

Spec URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi-0.3.11-2.src.rpm

Here is an updated package that addresses all(?) posted comments (except the
last one).

hyperestraier is imported into FE-devel/6/5
(currently available from
 http://buildsys.fedoraproject.org/plague-results/ )

Chitlesh Goorah has asked me to do an informal package review on this package.
Upon installing all BuildRequires needed to build this package, I find that
development package "file-devel" is not available for Fedora Core 6.

This leads up to the package requiring file-devel which is not available using
rpmbuild --rebuild strigi-0.3.11-2.src.rpm and not being able to build this
package on Fedora Core 6.

I've uploaded new spec and srpm files that requires "file-devel" for releases
after FC6 and "file" otherwise.

Spec URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi-0.3.11-3.src.rpm

Upstream released 0.5.1 today: http://www.vandenoever.info/software/strigi/
This is required for KDE 4.0 alpha 1 (3.90.1).

The KDE 3 Strigiapplet is no longer included in the Strigi tarball, but there's 
a strigiapplet-0.3.11 tarball for that.

Thanks Kelvin for the heads-up. AFAIK, strigiapplet has always been in its own
separate package. The latest release is up at;

Spec URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi-0.5.1-1.src.rpm

informal review:

1. SHOULD remove 
BR: dbus-qt-devel
(that's the qt3 dbus bindings, qt4 has integrated DBus binding support)

2. SHOULD add to -devel 
Requires: pkgconfig

3. cmake in fedora has been updated to include convenience rpm macros, so you
can simplify it's usage, see
http://fedoraproject.org/wiki/PackagingDrafts/cmake

I believe the Qt 3 dbus-qt-devel is needed for the KDE 3 strigiapplet, but the 
regular strigi package shouldn't need it.

Also IMHO the cmake BuildRequires should be:
BuildRequires: cmake >= 2.4.5
because CMakeLists.txt says:
CMAKE_MINIMUM_REQUIRED(VERSION 2.4.5 FATAL_ERROR)

(In reply to comment #15)
> informal review:
> 
> 1. SHOULD remove 
> BR: dbus-qt-devel
> (that's the qt3 dbus bindings, qt4 has integrated DBus binding support)
>
strigi is also useful with qt3, i believe.
 
> 2. SHOULD add to -devel 
> Requires: pkgconfig

Will do.

> 
> 3. cmake in fedora has been updated to include convenience rpm macros, so you
> can simplify it's usage, see
> http://fedoraproject.org/wiki/PackagingDrafts/cmake
The cmake rpm macro doesn't play nicely with strigi, the build fails somewhere
along the way with it. I think it particularly has to do with the macro
definning -DCMAKE-SHARED-LIBS=ON. 

As far as I can tell, Qt3 programs like strigiapplet simply use Strigi over 
D-Bus, there's no check for Qt 3 anywhere in strigi, so it's not a 
BuildRequires.

With corrections from comment #15 and #17,

Spec URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi-0.5.1-2.src.rpm

As this is a BR of the KDE 4 packages I'm working on, I'm going to review this.

Let's start with rpmlint output:

From rpmlint strigi-0.5.1-2.src.rpm:
W: strigi non-coherent-filename strigi-0.5.1-2.src.rpm
Probably due to no disttag in the SRPM name. Not a real problem.

No output from rpmlint on strigi-0.5.1-2.fc6.i386.rpm and 
strigi-debuginfo-0.5.1-2.fc6.i386.rpm, that's good.

On strigi-devel-0.5.1-2.fc6.i386.rpm, I get this:
W: strigi-devel no-documentation
Not a real problem, the documentation is in the main package.

So the rpmlint output is OK, I need to do the manual verifications before I can 
approve this though.

(In reply to comment #22)
> Let's start with rpmlint output:
> 
> From rpmlint strigi-0.5.1-2.src.rpm:
> W: strigi non-coherent-filename strigi-0.5.1-2.src.rpm
> Probably due to no disttag in the SRPM name. Not a real problem.
> 
Sorry that's caused by me renaming it while copying to the ftp server so as not
to confuse some people who might want to build it on fc6 or earlier. Here is the
actual srpm;

SRPM URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi-0.5.1-2.fc7.src.rpm

MUST Items: 
+ rpmlint output OK (see comment #22)
+ named and versioned according to the Package Naming Guidelines
+ spec file name matches base package name
+ Packaging Guidelines:
  + License LGPL OK
  + No known patent problems
  + No emulator, no firmware, no binary-only or prebuilt components
  + Complies with the FHS
  + proper changelog, tags, BuildRoot, Requires, BuildRequires, Summary, 
Description
  + no non-UTF-8 characters
  + relevant documentation is included
  + RPM_OPT_FLAGS are used
  + debuginfo package is valid
  + no static libraries nor .la files
  + no duplicated system libraries
  + no rpaths, at least on i386 (I ran strings on a few files)
  + no configuration files, so %config guideline doesn't apply
  + no init scripts, so init script guideline doesn't apply
  + desktop file is valid and installed correctly
  + no timestamp-clobbering file commands
  + _smp_mflags used
  + scriptlets are valid
  + not a web application, so web application guideline doesn't apply
+ complies with all the legal guidelines
MUST FIX: The License tag says "GPL", but the license is actually LGPL 
according to COPYING.
+ COPYING included as %doc
+ spec file written in American English
+ spec file is legible
+ source matches upstream:
  MD5SUM: b976b4f3cf451fc53cd773c338d78994
  SHA1SUM: 0bc32ab1668492ad44d061fd4d5753ec0344cb41
+ builds on at least one arch (FC6 i386)
+ no known non-working arches, so no ExcludeArch needed
+ all build dependencies listed in CMakeLists.txt are listed in BuildRequires, 
however:
SHOULD FIX: openssl-devel is no longer required, please remove BR
SHOULD FIX: expat-devel is not actually required either: CMakeLists.txt 
requires libxml2-devel, then goes on to require expat-devel OR libxml2-devel, 
so this actually means libxml2-devel is required and expat-devel is not really 
used (and rpm -q --requires strigi confirms that)
+ no translations in original tarball, so translation/locale guidelines don't 
apply
+ ldconfig correctly called in %post and %postun
+ package not relocatable
+ ownership correct (owns package-specific directories, doesn't own directories 
owned by another package) except for the following:
MUST FIX: strigiclient.desktop is installed into /usr/share/applications/kde. 
This is owned by kdelibs, which is not a direct or indirect dependency of 
strigi. So this has to be fixed one way or the other. As Strigi is in no way a 
KDE app (strigiclient is a Qt 4 app), please simply install the .desktop file 
into /usr/share/applications instead.
+ no duplicate files in %files
+ permissions set properly
+ %clean section present and correct
+ macros used where possible (%cmake macro does not work according to comment 
#18)
+ no non-code content
+ no large documentation files
+ %doc files not required at runtime
+ all header files in -devel
+ no static libraries, so no -static package needed
+ Requires: pkgconfig present
+ /usr/lib/*.so symlinks are correctly in -devel, however:
MUST FIX: The .so files in /usr/lib/strigi are NOT symlinks, they're loadable 
plugins, so they must be in the main package, not the -devel package.
+ -devel requires %{name} = %{version}-%{release} 
+ no .la files
+ the only GUI program in the package, strigiclient, has a .desktop file
+ buildroot is deleted at the beginning of %install
+ all filenames are valid UTF-8

SHOULD Items: 
+ license already included upstream
+ no translations for description and summary provided by upstream
+ package builds in mock (or at least used to) according to comment #1 (I may 
run the current package through mock later, but that's best done once the 
redundant BRs are removed so we can be sure they really were redundant. ;-) )
+ package builds successfully on my live system, and I checked for missing BRs 
(found none)
+ can't test non-i386 architectures, but all arches are supposed to be 
supported by upstream
+ package appears to work
+ scriptlets are sane
+ no subpackages other than -devel, so "Usually, subpackages other than devel 
should require the base package using a fully versioned dependency." is 
irrelevant
+ .pc files are in -devel
+ no file dependencies

Nitpicks:
* The guidelines say the Summary should not end with a period. This, however, 
doesn't apply to the %description. The sentence at the end of your main 
package %description should end with a period. (The %description devel is not a 
complete sentence and thus should not end with a period though.)
* Why did you set strigiclient.desktop to only get shown in KDE?
* "accessesed" should read "accessed".
* This one's a question more than a request to fix something: How is 
strigidaemon supposed to get run? Is this supposed to be done by the clients 
which need it (such as strigiapplet)? The strigiclient in any case doesn't 
offer to start the daemon, it just spits out warnings on the console when run 
without the daemon running, and some operations even crash in that case. Not 
that I personally care, I only need the indexers to build KDE 4 alpha 1 (used 
by the KDE 4 metadata classes), I hate background-indexing daemons, but people 
who actually want to use the daemon will need it started somehow.

Please fix your License tag, install the .desktop file into 
just %{_datadir}/applications, move %{_libdir}/strigi/ from -devel to the main 
package and remove the redundant BRs, then I'll approve this. (Also please 
consider fixing the nitpicks, but those aren't blockers.)

I should qualify "used by the KDE 4 metadata classes": It's used by the file 
metadata classes in kdelibs/kfile. There's now additional metadata classes for 
RDF stuff (based on Redland and Soprano) in kdelibs/kmetadata, those are not 
related to Strigi.

(In reply to comment #24)

> by the KDE 4 metadata classes), I hate background-indexing daemons, but people 
> who actually want to use the daemon will need it started somehow.
> 
If there's a popular request for it, i can create an autostart desktop file to
start the daemon along with the desktop session.

Fixes from comment #24;
Spec URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/strigi/strigi-0.5.1-3.fc7.src.rpm

+ All raised issues addressed:
  + License tag fixed
  + Unneeded BRs removed
  + .desktop file now in %{_datadir}/applications
  + %{_libdir}/strigi now in main package
  + typo and punctuation in %description fixed
+ No other changes to validate
+ Package still builds

APPROVED

New Package CVS Request
=======================
Package Name: strigi
Short Description: A desktop search program for KDE
Owners: dakingun
Branches: FC-6
InitialCC:

cvs done

> If there's a popular request for it, i can create an autostart desktop file
> to start the daemon along with the desktop session.

If you do that, you should really split off a strigi-libs subpackage so merely 
installing my KDE 4 packages won't force in a daemon some users may not want. 
That's how beagle is packaged, too (as well as other stuff such as mysql), so 
it wouldn't be a first.

Still, I think this (both the .desktop file and the strigi-libs subpackage) 
would be the best solution.

I've split out strigi-libs sub-package from the main package and included an
autostart desktop file for strigidaemon in the main package. Package has been
built for devel and FC-6.
Thanks to all that contributed to the review.

Shouldn't %{_datadir}/strigi/ also be in -libs? Or is that data really not 
needed for the libs to work?

(In reply to comment #32)
> Shouldn't %{_datadir}/strigi/ also be in -libs? Or is that data really not 
> needed for the libs to work?
No, I don't think the fieldproperties are required for the libs to work.

I guess we'll see, if it breaks KDE 3.90.1, I'll just file a separate bug.