Bug 218599 - Review Request: klibido - NNTP (Usenet) file grabber for KDE
Review Request: klibido - NNTP (Usenet) file grabber for KDE
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-06 07:46 EST by Francois Aucamp
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-15 06:55:15 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Francois Aucamp 2006-12-06 07:46:34 EST
Spec URL: http://www.snoekie.com/rpm/klibido.spec
SRPM URL: http://www.snoekie.com/rpm/klibido-0.2.5-2.src.rpm
Description:
KLibido (KDE LInux BInaries DOwnloader) is a KDE program to download encoded
articles from the usenet news service, using the nntp protocol. It supports
multiple servers, multiple download threads per server, newzbin (.nzb) files,
automatic joining and decoding of articles.

Notes:
rpmlint gives:
W: klibido dangling-relative-symlink /usr/share/doc/HTML/en/klibido/common ../common

- That symlink does not dangle, it points to /usr/share/doc/HTML/en/common, which is owned by kdelibs
Comment 1 Francois Aucamp 2006-12-06 08:14:19 EST
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. :-/
Comment 2 Parag AN(पराग) 2006-12-06 10:42:40 EST
still not able to download SRPM
Comment 3 Parag AN(पराग) 2006-12-07 02:44:55 EST
Can you check this package in mock build? its not building and asking X
error: Can't find X includes.
Comment 4 Francois Aucamp 2006-12-07 04:55:07 EST
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
Comment 5 Francois Aucamp 2006-12-21 10:13:44 EST
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.
Comment 6 Francois Aucamp 2007-01-17 03:18:45 EST
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
Comment 7 Johan Cwiklinski 2007-04-14 19:05:39 EDT
This server does not respond :/
Comment 8 Francois Aucamp 2007-04-16 03:18:02 EDT
Indeed. It's been awhile since I've checked it... will fix and update.
Comment 9 Francois Aucamp 2007-04-16 05:15:21 EDT
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
Comment 10 Francois Aucamp 2007-04-19 09:42:14 EDT
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

Comment 11 Francois Aucamp 2007-05-16 04:32:58 EDT
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
Comment 12 Jason Tibbitts 2007-06-05 00:17:12 EDT
I'll review this as long as nobody asks why....
Comment 13 Jason Tibbitts 2007-06-05 01:51:06 EDT
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.
Comment 14 Francois Aucamp 2007-06-05 08:58:29 EDT
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
Comment 15 Rex Dieter 2007-06-05 09:14:15 EDT
> - Added hicolor-icon-theme dependency

imo, extraneous and unnecessary, kdelibs already Requires it.
Comment 16 Jason Tibbitts 2007-06-05 19:56:42 EDT
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.
Comment 17 Francois Aucamp 2007-06-06 02:59:13 EDT
(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.
Comment 18 Jason Tibbitts 2007-06-14 14:45:56 EDT
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
Comment 19 Francois Aucamp 2007-06-14 15:37:50 EDT
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@csir.co.za
Branches: F-7 FC-6
InitialCC: 
Comment 20 Kevin Fenzi 2007-06-14 20:38:57 EDT
cvs done.
Comment 21 Francois Aucamp 2007-06-15 06:55:15 EDT
Imported and built, closing ticket:
http://koji.fedoraproject.org/koji/buildinfo?buildID=8861

Thanks again to everyone for the review and comments.

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