* Build failure:

make[2]: Entering directory `/home/qa/tmp/rpm/BUILD/lash-0.5.1/lashd'
test -z "/usr/bin" || mkdir -p --
"/home/qa/tmp/rpm/tmp/lash-0.5.1-1-root-qa/usr/bin"
  /bin/sh ../libtool --mode=install /usr/bin/install -c 'lashd'
'/home/qa/tmp/rpm/tmp/lash-0.5.1-1-root-qa/usr/bin/lashd'
libtool: install: warning: `../liblash/liblash.la' has not been installed in
`/usr/lib'
/usr/bin/install -c .libs/lashd
/home/qa/tmp/rpm/tmp/lash-0.5.1-1-root-qa/usr/bin/lashd
if ! grep -q ^lash /etc/services; then \
  echo -e "\nlash\t\t14541/tcp\t\t\t# LASH client/server protocol" >>
/etc/services; \
fi
/bin/sh: /etc/services: Permission denied


* Configure warning: (would be "BuildRequires: texi2html")

checking for texi2html... no
configure: WARNING: texi2html not found, manual will not be built

* Wrong "BuildRequires: tetex"

* pkgconfig file contains hardcoded rpath linker options

* Missing -devel package "Requires" for at least jack and alsa
(see pkgconfig file) -devel packages, and e2fsprogs-devel
(see installed C headers)

* Don't install the .la libtool archive file unless you like
libtool dependency hell

* Use %_infodir instead of %_datadir/info/

* Add "rm -f %{buildroot}%{_infodir}/dir" at end of %install section

* Install info files with install-info:
  http://fedoraproject.org/wiki/ScriptletSnippets

I believe all this is fixed, and more.

Spec URL: http://people.redhat.com/green/FE/FC5/lash.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/lash-0.5.1-2.src.rpm

Thanks!

I'm sponsering Nando from CCNRMA and attempting to help him to get a bit upto
steam, since one of his packages needs fluidsynth which in turn needs this one
I'm reviewing this one. Callum I hope you don't mind.

MUST:
=====
0 rpmlint output is:
W: lash incoherent-version-in-changelog 0.4.0-2 0.5.1-2
W: lash devel-file-in-non-devel-package /usr/lib/pkgconfig/lash-1.0.pc
E: lash zero-length /usr/share/doc/lash-0.5.1/docs/html-manual-stamp
E: lash postin-without-ldconfig /usr/lib/liblash.so.2.0.0
E: lash library-without-ldconfig-postun /usr/lib/liblash.so.2.0.0
E: lash zero-length /usr/share/doc/lash-0.5.1/docs/texinfo.tex
W: lash-devel no-documentation
These all must be fixed except for the last Warning which can be ignored
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License (GPL) ok, license file included
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel-i386
0 BR: ok (see below)
* No locales
0 shared libraries but no ldconfig, see MUST fix (and rpmlint output).
* Not relocatable
* Package owns / or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
0 -devel as needed, .pc file must be moved to -devel
0 gui .desktop files required Fix please.


MUST fix:
=========
* the following rpmlint output:
W: lash incoherent-version-in-changelog 0.4.0-2 0.5.1-2
W: lash devel-file-in-non-devel-package /usr/lib/pkgconfig/lash-1.0.pc
E: lash zero-length /usr/share/doc/lash-0.5.1/docs/html-manual-stamp
E: lash postin-without-ldconfig /usr/lib/liblash.so.2.0.0
E: lash library-without-ldconfig-postun /usr/lib/liblash.so.2.0.0
E: lash zero-length /usr/share/doc/lash-0.5.1/docs/texinfo.tex
* Redundant BuildRequires: libtermcap-devel (already implied by readline-devel
  Please remove the libtermcap-devel BR.
* This can't be right can it? :
 BuildRequires: gtk+-devel, gtk2-devel
 Usually a package uses either gtk+ (gtk1) or gtk2, not both.
 Judging from the rpm -q --requires lash output it uses gtk2 and thus gtk+-devel
 can be dropped.
* Lash contains gtk apps which are gui apps and should properly install
 .desktop files for this, see:
http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d
 for info on howto properly install .desktop files.
* The lash_XXpx.png icons are installed /usr/share/lash/icons that is not
  according to the freedesktop.org icon standard, it should go under:
  %{_datadir}/icons/hicolor/32x32/apps
  Where 32x32 is the size of the icon, the ixons then should be named just 
  lash.png (not lash_XXpx.png. Please do ls /usr/share/icons/hicolor/
  to see the available valid sizes, if the icon doesn't match any pick the 
  closest. I think lash doesn't use the other format icons in that case they
  should be dropped in %doc and the /usr/share/lash/icons dir should be removed
  (or atleast not packaged).
* Once the icon is in the proper case you must add %post(un) script to update 
  the icon-cache see:
http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d
* The dtd file should be installed under /usr/share/xml/lash/dtds instead of
 under /usr/share/lash/dtds. Notice that after these changes there is no more
 use of /usr/share/lash needed!
* The: "Requires: jack-audio-connection-kit-devel alsa-lib-devel"
 for the devel package should be above %description, now its part of the
 description:
[hans@guest-dhcp-048 ~]$ rpm -qi lash-devel
Name        : lash-devel                   Relocations: (not relocatable)
Version     : 0.5.1                             Vendor: (none)
Release     : 2                             Build Date: Thu 15 Jun 2006 12:26:42
PM CEST
Install Date: Thu 15 Jun 2006 12:31:44 PM CEST      Build Host:
guest-dhcp-048.si.hhs.nl
Group       : Development/Libraries         Source RPM: lash-0.5.1-2.src.rpm
Size        : 25430                            License: GPL
Signature   : (none)
URL         : http://www.nongnu.org/lash/
Summary     : Development files for LASH
Description :
Development files for the LASH library.
Requires: jack-audio-connection-kit-devel alsa-lib-devel
[hans@guest-dhcp-048 ~]$
 

Should fix:
===========
* Please use %{version} in Source0 this makes life easier for yourself when you 
  want to upgrade to a newer upstream version.

(In reply to comment #3)
> I'm sponsering Nando from CCNRMA and attempting to help him to get a bit upto
> steam, since one of his packages needs fluidsynth which in turn needs this one
> I'm reviewing this one. Callum I hope you don't mind.

Thanks for the review.  Updates here:

Spec URL: http://people.redhat.com/green/FE/FC5/lash.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/lash-0.5.1-3.src.rpm

> Should fix:
> ===========
> * Please use %{version} in Source0 this makes life easier for yourself when you 
>   want to upgrade to a newer upstream version.
> 

I've been told not to do this in the past because some people want Source0 to be
directly wget-able.

Thanks again!

According to the packaging guidelines, macros in Source: is a matter of style.
An example of how to use RPM to expand the macros for you is given, but
ultimately you get to do what you want.

Much better unfortunatly there is one new blocker

MUST fix:
-Under %files it currently says: "%{_datadir}/xml/lash/dtds"
That should be: "%{_datadir}/xml/lash" otherwise that dir is unowned.
-update-desktop-database is only needed when a .desktop file which defines a
 mimetype is installed, that doesn't happen here so please remove the calling
 of update-desktop-database from %post(un)
-and also remove because of the above change:
Requires(post):   desktop-file-utils
Requires(postun): desktop-file-utils

About using %version in Source0 either way is fine it was just an advice, people
who want it to be directly wget-able should learn to use "spectool -g bla.spec"
which will expand the macros, do the copy paste and wget call for them.

(In reply to comment #6)
> Much better unfortunatly there is one new blocker
>

Ok, all done.  Thanks for all the effort you put into this.  Updates here...

Spec URL: http://people.redhat.com/green/FE/FC5/lash.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/lash-0.5.1-4.src.rpm

Looks good now, Approved!

One last note though in the %post(un) scripts you should add "  || :" at the end
of the touch and gtk-update-icon-cache lines as documented here:
http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d
This is so that if for whatever reason they fail that doesn't make the script
have a non zero return value and does the whole rpm (un)install transaction fail.

(In reply to comment #8)
> Looks good now, Approved!

Thanks.  I've made your last suggested change as well.