Bug 219873 - Review Request: kio_sword - lightweight Sword front-end for KDE
Review Request: kio_sword - lightweight Sword front-end for KDE
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Deji Akingunola
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-12-15 15:25 EST by David Anderson
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-01-15 19:03:03 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description David Anderson 2006-12-15 15:25:21 EST
This is my first package - I am seeking a sponsor.

Spec URL: http://david.dw-perspective.org.uk/tmp/
SRPM URL: http://david.dw-perspective.org.uk/tmp/

Description: 
A lightweight easy-to-use Bible tool for KDE.
Kio-Sword provides access to Bibles, commentaries
and other texts in an easy to use and attractive
interface -- the Konqueror web browser.  It does so
using the SWORD Bible project and implementing a KDE
ioslave, providing the sword:/ protocol.
Comment 2 Chitlesh GOORAH 2006-12-17 05:01:57 EST
#1. Parallel make should be used
see
http://fedoraproject.org/wiki/Packaging/Guidelines#head-525c7d76890cb22df33b759c65c35c82bf434d2e

#2. gcc-c++ is among the exceptions, remove it from BR
see
http://fedoraproject.org/wiki/Packaging/Guidelines#head-4cadce5e79d38a63cad3941de1dadc9d25d67d30

#3: is there a specific reason why you used:
[ -n "$RPM_BUILD_ROOT" -a "$RPM_BUILD_ROOT" != / ] && %{__rm} -rf
--preserve-root $RPM_BUILD_ROOT
The buildroot is already defined.

#4: you should use
see: Scriptlets
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

#5: during rpmbuild, I have:
/var/tmp/rpm-tmp.59067: line 30: kdeconfig: command not found
+ export KDEDIR=
+ KDEDIR=                                                                      
                                             

is it useful to use kdeconfig ?
Comment 3 David Anderson 2006-12-19 15:08:33 EST
Thanks for the review. All issues have been addressed:

New versions (note the upstream version bump):

Spec URL: http://david.dw-perspective.org.uk/tmp/kio_sword.spec
SRPM URL: http://david.dw-perspective.org.uk/tmp/kio_sword-0.3-1.fc5.src.rpm

Fixed 1, 2, 3, 4. (The reason for #3 was paranoia).

For 5, the value of it is used in the autoconf scripts in the tarball. (My 
error in using kdeconfig instead of kde-config (now fixed) was leading to a 
pre-programmed default being used, which happened to be the right thing).
Comment 4 Mamoru TASAKA 2007-01-08 10:51:59 EST
(In reply to comment #0)
> This is my first package - I am seeking a sponsor.

(Just noticing that now I am sponsoring and currently
 this bug does not block FE-NEEDSPONSOR)
Comment 5 Deji Akingunola 2007-01-12 10:49:44 EST
Hi David,
A couple of needswork and nitpicks,

NEEDSWORK:
* Packages doesn't build in x86_64 mock
* Needs to call ldconfig in post and postun
* Ought to use the %find_lang for locales/translations
* Packages must NOT contain any .la libtool archives

NITS:
* You should really require sword >=1.5.8
* Setting the CFLAGS in the spec is not necessary, it's already set by rpmbuild
* Also setting the $PATH for QT is not necesaary, already done /etc/profile.d/qt.sh
(I notice you only set this variables, you didn't export them)
* I'm not sure what '--preserve-root' option does when you clean the buildroot,
but I think its unnecesary.
* I'm not 
Comment 6 David Anderson 2007-01-12 11:46:44 EST
Thanks, Deji.

New files:
Spec URL: http://david.dw-perspective.org.uk/tmp/kio_sword.spec
SRPM URL: http://david.dw-perspective.org.uk/tmp/kio_sword-0.3-2.src.rpm

> NEEDSWORK:
> * Packages doesn't build in x86_64 mock

Fixed using patch (thanks again!).

> * Needs to call ldconfig in post and postun
> * Packages must NOT contain any .la libtool archives

I don't believe these two are true. All kioslaves need the libtool archive and 
won't work without them. Fedora ships these in kdebase for the standard 
kioslaves. (I believe that in KDE4 they'll go away).

> * Ought to use the %find_lang for locales/translations

There's no localisation upstream, so this is a no-op, but I've implemented it 
anyway (as I believe upstream is introducing it in future).

> NITS:
> * You should really require sword >=1.5.8

Done.

> * Setting the CFLAGS in the spec is not necessary, it's already set by 
rpmbuild

Removed.

> * Also setting the $PATH for QT is not necesaary, already 
done /etc/profile.d/qt.sh

Removed.

> * I'm not sure what '--preserve-root' option does when you clean the 
buildroot, but I think its unnecesary.

Paranoia - it tells "rm" to cancel the command if told to "rm -rf /" ! Anyway, 
gone now.
Comment 7 Rex Dieter 2007-01-12 13:55:18 EST
> kioslaves need the libtool archive

yup, for now anyway, until kde(kdelibs) is fixed/patched to do otherwise.
Comment 8 Chitlesh GOORAH 2007-01-12 14:22:14 EST
(In reply to comment #4)
> (In reply to comment #0)
> > This is my first package - I am seeking a sponsor.
> 
> (Just noticing that now I am sponsoring and currently
>  this bug does not block FE-NEEDSPONSOR)


I've blocked this bug as FE-NEEDSPONSOR.
Deji Akingunola, are you sponsoring?
Comment 9 Deji Akingunola 2007-01-12 14:26:05 EST
Hi Chitlesh,

I think Mamoru fixed that in comment #4
Comment 10 David Anderson 2007-01-12 14:28:53 EST
Confirmed - Mamoru Tasaka is my sponsor (curlftpfs, my first package, is 
already in Extras). I don't need a sponsor any more for this.
Comment 11 Chitlesh GOORAH 2007-01-12 14:35:33 EST
Thanks for such info :)
Comment 12 Deji Akingunola 2007-01-13 01:15:46 EST
Almost done, rpmlint is not satisfactorily silent on the binary rpm

[deji@agape deji]$ rpmlint kio_sword-0.3-2.fc7.x86_64.rpm
W: kio_sword dangling-symlink /usr/share/doc/HTML/en/kio_sword/common
/usr/share/doc/HTML/en/common
W: kio_sword symlink-should-be-relative /usr/share/doc/HTML/en/kio_sword/common
/usr/share/doc/HTML/en/common

The first can be ignored (knowing the target is there), the following patch fix
the second warning;
>>
--- kio_sword.spec     2007-01-13 01:13:05.000000000 -0500
+++ kio_sword.spec.new      2007-01-13 01:16:41.000000000 -0500
@@ -37,6 +37,11 @@
 
 make DESTDIR=$RPM_BUILD_ROOT install
 
+#Fix symlink relative
+pushd $RPM_BUILD_ROOT%{_datadir}/doc/HTML/en/kio_sword/
+ln -sf ../common common
+popd
+
 %find_lang kio_sword || touch kio_sword.lang
 HTML_DIR=$(kde-config --expandvars --install html)
 if [ -d $RPM_BUILD_ROOT/$HTML_DIR ]; then
<<
Comment 13 Deji Akingunola 2007-01-13 01:48:36 EST
I'm sorry that patch should really look like below;
>>
--- /home/deji/Desktop/kio_sword-0.3-2.src.rpm_FILES/kio_sword.spec    
2007-01-13 01:48:38.000000000 -0500
+++ kio_sword.spec      2007-01-13 01:32:54.000000000 -0500
@@ -37,6 +37,12 @@
 
 make DESTDIR=$RPM_BUILD_ROOT install
 
+#Fix symlink to be relative
+rm -rf $RPM_BUILD_ROOT%{_datadir}/doc/HTML/en/kio_sword/common
+pushd $RPM_BUILD_ROOT%{_datadir}/doc/HTML/en/kio_sword/
+ln -s ../common common
+popd
+
 %find_lang kio_sword || touch kio_sword.lang
 HTML_DIR=$(kde-config --expandvars --install html)
 if [ -d $RPM_BUILD_ROOT/$HTML_DIR ]; then
<<
I guess its time to go to bed.
Comment 14 David Anderson 2007-01-13 11:19:59 EST
Thanks again, Deji. Patch added.

New files:
Spec URL: http://david.dw-perspective.org.uk/tmp/kio_sword.spec
SRPM URL: http://david.dw-perspective.org.uk/tmp/kio_sword-0.3-3.src.rpm

Personally I'm not sure of whether silencing trivial rpmlint warnings is any 
better than cluttering up the specfile, but seeing as you've helped me so much 
I'll go with you!
Comment 15 Deji Akingunola 2007-01-13 11:54:45 EST
(In reply to comment #14)

> Personally I'm not sure of whether silencing trivial rpmlint warnings is any 
> better than cluttering up the specfile,
I agree with you, but the packaging guildeline says the rpmlint must be
satisfied as much as possible, "for thus it becometh us to fulfill all
righteousness" ;).

GOOD:
* rmplint silent mock built binary
* rpmlint warning on srpm can be ignored;
(W: kio_sword dangling-relative-symlink /usr/share/doc/HTML/en/kio_sword/common
../common)
* source tarball matches upstream's
md5sum: 957c563737d47900f67661086732ee12  kio_sword-0.3.tar.gz
* package name meets the Package Naming Guidelines.
* Satifies the Packaging Guidelines.
* Builds in mock (x86_64 development)
* Installs OK
* License: GPL
* Spec file generally OK.

It includes a .la, but that's OK for kde packages for now (coments #6 & #7)

APPROVED.



 
Comment 16 David Anderson 2007-01-15 19:03:03 EST
Thanks to all who have helped.

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