Bug 218599

Hmm... It seems the domain name records (snoekie.com) have not finished
propagating across the Internet, so please give it some time if at first the
links don't work. :-/

still not able to download SRPM

Can you check this package in mock build? its not building and asking X
error: Can't find X includes.

Oops. Silly mistake, actually had the necessary BR's in there, but for some
strange reason removed it just before I uploaded the -2 release... :-? That's
what you get for being hasty...

Anyhow, here's the new build:
Spec URL: http://www.snoekie.com/rpm/klibido.spec
SRPM URL: http://www.snoekie.com/rpm/klibido-0.2.5-3.src.rpm

Changes:
- Added missing BuildRequires: kdelibs-devel db4-devel

Ping? 
Please note, I'll be on holiday from tomorrow (22 Dec) till the 6th of January
2007, with very little (if any) Internet access, so I won't be able to respond
on any comments until that time.

New build:
Spec URL: http://www.snoekie.com/rpm/klibido.spec
SRPM URL: http://www.snoekie.com/rpm/klibido-0.2.5-4.src.rpm

Changes:
- Added proper handling of .desktop file
- Added BuildRequires: desktop-file-utils

This server does not respond :/

Indeed. It's been awhile since I've checked it... will fix and update.

Sigh. Free hosting - can't live with it, can't live without it. ;-)
The server seems to be working now, although it's a bit unstable as of late.

Also, I've updated the SRPM:

Spec URL: http://www.snoekie.com/rpm/klibido.spec
SRPM URL: http://www.snoekie.com/rpm/klibido-0.2.5-5.src.rpm

Changes:
- Create a 48x48 icon
- Added BuildRequires: ImageMagick

Ok, the content has moved to a new host:

Spec URL: http://fna.vel4.com/rpm/klibido.spec
SRPM URL: http://fna.vel4.com/rpm/klibido-0.2.5-5.src.rpm

Again, a new (hopefully more reliable) host... *sigh* Please excuse the spec
file naming, the asp-based host seems to have an issue with .spec file extensions.

Spec URL: http://www4.webng.com/faucamp/rpm/klibido_spec.txt
SRPM URL: http://www4.webng.com/faucamp/rpm/klibido-0.2.5-5.src.rpm

I'll review this as long as nobody asks why....

Overall, this is a clean package.  rpmlint says only:
   W: klibido dangling-relative-symlink /usr/share/doc/HTML/en/klibido/common 
      ../common
This is normal for KDE packages; they depend on the base KDE package which will
provide the link target.

One primary issue is that this links statically against libuu.  Not that you
have much choice as it's only provided as a static library.  (That's unfortunate
as it's going to see untrusted content and has had a buffer overflow issue in
the past.)

So, to make this obvious, please make the build dependency on uulib-static
instead of uulib so that we can grep for it.  I'll raise the static linking
issue with FESCo.  (All instances of static linking must be explicitly approved.)

You use both $RPM_BUILD_ROOT and %{buildroot} at various points in the spec. 
You can use whichever you like, but you should be consistent.

The /usr/share/icons/hicolor/*/apps directories are unowned.  You should have a
runtime dependency on hicolor-icon-theme to bring them in.

The .desktop file has no GenericName entry; it uses Comment instead.  I also
believe that it's common KDE usage to capicalize the 'K' in the program name for
the menu.

Review:
* source files match upstream:
   7bd35c5cf6f1b9b26f802e78d63e9ae7a66763ca4c0c4a78ecc87d45a20b1d22  
   klibido-0.2.5.tar.gz
* package meets naming and versioning guidelines.
X specfile is properly named, is cleanly written but does not consistently use 
  $RPM_BUILD_ROOT/%{buildroot}.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
X BuildRequires are proper (please use uulib-static to make the static linking 
  obvious.)
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
* rpmlint has only acceptable complaints.
X final provides and requires:
   klibido = 0.2.5-5.fc7
  =
   libDCOP.so.4()(64bit)
   libICE.so.6()(64bit)
   libSM.so.6()(64bit)
   libX11.so.6()(64bit)
   libXext.so.6()(64bit)
   libXrender.so.1()(64bit)
   libdb-4.5.so()(64bit)
   libdb_cxx-4.5.so()(64bit)
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libkdecore.so.4()(64bit)
   libkdefx.so.4()(64bit)
   libkdesu.so.4()(64bit)
   libkdeui.so.4()(64bit)
   libkio.so.4()(64bit)
   libkmdi.so.1()(64bit)
   libkparts.so.2()(64bit)
   libkutils.so.1()(64bit)
   libkwalletclient.so.1()(64bit)
   libpthread.so.0()(64bit)
   libpthread.so.0(GLIBC_2.2.5)(64bit)
   libqt-mt.so.3()(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)
   libutempter.so.0()(64bit)
   libutil.so.1()(64bit)
   libz.so.1()(64bit)
 X Missing hicolor-icon-theme dependency.
* %check is not present; no test suite upstream.  I installed it on my desktop 
   and it seemed to work well enough.
* no shared libraries are added to the regular linker search paths.
X owns the directories it creates (/usr/share/icons/hicolor/*)
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.
X Desktop file has minor issues.

Thanks for the review comments!

New package:
Spec URL: http://www4.webng.com/faucamp/rpm/spec.txt
SRPM URL: http://www4.webng.com/faucamp/rpm/klibido-0.2.5-6.src.rpm
(again, I apologize for the dodgy spec naming scheme and rpm mime type info)

Changes:
- Spec now consistently uses RPM_BUILD_ROOT
- Changed uulib BuildRequires to uulib-static for Fedora 7 and up
- Added hicolor-icon-theme dependency
- Added "desktop_file" patch to fix some minor issues in the .desktop file

> - Added hicolor-icon-theme dependency

imo, extraneous and unnecessary, kdelibs already Requires it.

Rex is right, of course; I haven't reviewed many KDE packages lately and lost
track of the dependency chain.  You can drop or keep the dependency as you wish.

The specfile looks much better.  Sorry about the conditional use of
uulib-static; I didn't know you wanted to build for FC6.

I built and installed and the desktop file looks good as well.

So at this point I'd approve this, except for the static linking bit.  We'll
know more about that on Thursday.

(In reply to comment #16)
> Rex is right, of course; I haven't reviewed many KDE packages lately and lost
> track of the dependency chain.  You can drop or keep the dependency as you wish.

I actually picked this up when I originally created the package, but had
forgotten about it, so that makes 2 of us :-).  I'll drop the dependency before
importing the package, if it is approved.

> The specfile looks much better.  Sorry about the conditional use of
> uulib-static; I didn't know you wanted to build for FC6.

No problem, anyhow it's better this way if the uulib-static issue ever gets solved.

> I built and installed and the desktop file looks good as well.
> 
> So at this point I'd approve this, except for the static linking bit.  We'll
> know more about that on Thursday.

Thanks for raising the issue with FESCo. Unfortunately I do not have irc access
(don't ask), so I can't participate directly in this, but please post/mail me if
I should do anything.

OK, let's do this:

Just drop the fancy uulib-devel/uulib-static bits and just require uulib-devel.
 That way if uulib does ever grow a dynamic version you won't need to do
anything (other than rebuild) to make use of it.

And of course drop that extra hicolor-icon-theme bit.  But otherwise,

APPROVED

Thanks for the review!

I'll drop the conditional uulib dependencies and hicolor-icon-theme dep before
importing.

New Package CVS Request
=======================
Package Name: klibido
Short Description: NNTP (Usenet) file grabber for KDE
Owners: faucamp.za
Branches: F-7 FC-6
InitialCC: 

cvs done.

Imported and built, closing ticket:
http://koji.fedoraproject.org/koji/buildinfo?buildID=8861

Thanks again to everyone for the review and comments.