The package will need a bit more work before I can approve it.


OK: source files match upstream:
aefe6c6ddb46fb5eb5927974f0080518f63cf61a
OK: dist tag is present.
OK: build root is correct.
OK: license field matches the actual license.
OK: license is open source-compatible.
 GPLv2+
OK: license text included in package.
OK: latest version is being packaged.
OK: BuildRequires are proper.
OK: compiler flags are appropriate.
OK: %clean is present.
OK: package builds in mock.
OK: debuginfo package looks complete.
OK: no shared libraries are added to the regular linker search paths.
OK: doesn't own any directories it shouldn't.
OK: no duplicates in %files.
OK: file permissions are appropriate.
OK: code, not content.
OK: documentation is small, so no -docs subpackage is necessary.
OK: %docs are not necessary for the proper functioning of the package.
OK: no headers.
OK: no pkgconfig files.
OK: no libtool .la droppings.
OK: desktop files valid and installed properly not necessary.


PASS: package meets naming and versioning guidelines.
 Did you consider naming ncidd differently? Maybe ncid-server to fit in with 
 the ncid-client?


NOK: specfile is properly named, is cleanly written and uses macros
consistently.

The .spec/srpm file is a bit messy:

 * Please do use full URLs for Source0.
   Instead of "Source0:        %{name}-%{version}-src.tar.gz" please use 
   "Source0:        http://downloads.sourceforge.net/%{name}/%{name}-%
   {version}-src.tar.gz"
 * Please name your patches accordingly:
   ncid-0.71-no-default-runlevels.patch would be a good convention to follow.
 * Please start your patches with the id 0.
 * Trim the descriptions. They are way too long. More then 10 lines should 
   never be necessary.
 * I'd prefer you to move all the %post scripts to the bottom of the .spec 
   between %clean and %file. It's again a convention most packagers seem to 
   follow and it makes reading .spec files so much easier.
 * Swap the /usr hardcode in the %build and %install part for %{_prefix}

NOK: rpmlint is silent.
 ncid-client.i386: W: spurious-executable-perm /usr/share/doc/ncid-client-0.71/ncid-mythtv
 ncidd.i386: E: non-ghost-file /var/log/cidcall.log
 ncidd.i386: E: zero-length /var/log/cidcall.log
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/ncidd $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/ncidd $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/ncidd $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/ncidsip $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/ncidsip $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/ncidsip $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/sip2ncid $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/sip2ncid $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/sip2ncid $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/yac2ncid $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/yac2ncid $prog
 ncidd.i386: W: incoherent-subsys /etc/rc.d/init.d/yac2ncid $prog
 ncid-page.i386: W: no-documentation
 ncid-popup.i386: W: no-documentation
 ncid-samba.i386: W: no-documentation
 ncid-speak.i386: W: no-documentation
 8 packages and 0 specfiles checked; 2 errors, 17 warnings.

Please remove the executable bit from the mythtv initscript. You might wanna drop it completely though.
Please consider dropping the cidcall.log file and %ghost it instead if it will be created by the daemon if non-existant.
The incoherent-subsys warnings should be ignoreable, possible the same for the plugin subpackages.


NOK: package installs properly.
 The ncid-page tool will not install, the Require for mail is invalid.
 [root@workstation ncid]# repoquery --whatprovides mail
 [root@workstation ncid]# 
 Either use mailx or /bin/mail as a dep.


PASS: scriptlets present and work okay.
 It would be great though if you could rework the initscripts to support 
 the newer upstart syntax.


NOK: final provides and requires are sane:
Requires:
 /bin/bash  
 /bin/sh  
 config(ncid-client) = 0.71-1.fc9
 config(ncidd) = 0.71-1.fc9
 festival  
 kdebase  
 kdelibs  
 kdemultimedia  
 libc.so.6  
 libc.so.6(GLIBC_2.0)  
 libc.so.6(GLIBC_2.1)  
 libc.so.6(GLIBC_2.3)  
 libc.so.6(GLIBC_2.3.4)  
 libc.so.6(GLIBC_2.4)  
 libpcap.so.0.9  
 mail  
 ncid-client = 0.71-1.fc9
 ncid-speak = 0.71-1.fc9
 perl(Data::Dumper)  
 perl(Getopt::Long)  
 perl(Getopt::Std)  
 perl(IO::Socket::INET)  
 perl(Net::Pcap)  
 perl(Pod::Usage)  
 perl(POSIX)  
 perl(strict)  
 perl(warnings)  
 rpmlib(CompressedFileNames) <= 3.0.4-1
 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
 rtld(GNU_HASH)  
 smbclient  
 /usr/bin/perl  
Provides:
 config(ncid-client) = 0.71-1.fc9
 ncid-client = 0.71-1.fc9
 config(ncidd) = 0.71-1.fc9
 ncidd = 0.71-1.fc9
 ncid-debuginfo = 0.71-1.fc9
 ncid-page = 0.71-1.fc9
 ncid-popup = 0.71-1.fc9
 ncid-samba = 0.71-1.fc9
 ncid-speak = 0.71-1.fc9

Problem is the mail dependency again.


NOK: owns the directories it creates.
 /etc/ncid
 /usr/share/ncid
 are shared by the packages but no package owns these directories.


Another observation:
 Why did you remove the shebang line from the ncidrotate-script? If it's 
 supposed to be executable, drop it in /usr/libexec/%{name}/.


A general suggestions: I found upstream .spec files often too much work to adapt to the fedora packaging guidelines which is why I'm rolling my own usually. Might be wort considering the next time. :)

Thanks for the thorough review; I'll look over the questions & suggestions in a bit.

At this point I don't actually remember if I wrote the specfile from scratch or used the upstream one :)

-Eric

Thanks. Just upload a new srpm and I'll take a look and approve if possible. 

It looks a lot like the spec in the Fedora subdir of the upstream tarball. So I just assumed.

regards,
 andreas

Finally trying to take some time to finish this up.  One question:

> NOK: owns the directories it creates.
> /etc/ncid
> /usr/share/ncid
> are shared by the packages but no package owns these directories.

If both ncidd and ncid-client put files here, but neither one requires the other, can they both "own" these directories?

Oh, also:

> Another observation:
>  Why did you remove the shebang line from the ncidrotate-script? If it's 
>  supposed to be executable, drop it in /usr/libexec/%{name}/.

It really isn't executable, it's just sourced by... er... another script which isn't packaged or installed.  Grr.  I should just install ncidd.logrotate and remove the other one.

Regarding initscripts, I have a least one other package w/ initscripts, I'll probably convert all to upstart when appropriate and when I have some time.

I'll have to check on the cidcall.log issue; ISTR that the daemon didn't properly create it if it didn't already exist (maybe even failed to start), and make install touches it:

install-log:
        touch $(CALLLOG)

so... *shrug*  I could echo something into it to shut up rpmlint ;)

I have to say that the package itself isn't particularly tidy... I may consider trying to get some of these fixes upstream before I decide that I really want to maintain this in fedora ...  I've been using it at home though, and it does seem reasonably functional & useful, at least :)

Thanks,
-Eric

Sorry for the extremely long delay here, I have some time, and a new upstream version of ncid, and I'll try to get this finished up.

Updated stuff here, hopefully addressing most of these issues:

http://sandeen.fedorapeople.org/ncid/ncid-0.73-2.fc11.src.rpm
http://sandeen.fedorapeople.org/ncid/ncid.spec

Thanks,
-Eric

Andreas, care to take another look at those updated packages?

thanks,
-eric

Upstream has a new release now too.

http://sandeen.fedorapeople.org/ncid/ncid.spec
http://sandeen.fedorapeople.org/ncid/ncid-0.74-1.fc12.src.rpm

Andreas, ping?  I think we're close to having this done.  :)

Andreas, ping?

Stepping in on this one...

1) There are muliple "for SCRIPT in ncidd ncidsip ncidsip" lines in the spec. I doubt ncidsip needs to be done twice.

2) In %files, use standard macros for /usr/bin (%{_bindir}), /usr/sbin (%{_sbindir}), /usr/share (%{_datadir}), etc.

3) I'd argue that ncid-mythtv is an acceptable sub-package. Sure, its useless without mythtv running on your network somewhere, but we have all sorts of software that is in the same boat -- MPD frontends, libvdpau just approved today by FESCo, libXNVCtrl, the totem-mythtv plugin (probably the best example, as to be really useful, you need a mythtv backend on your network, and likely assorted codecs mythtv doesn't ship to play back mythtv recordings in totem), etc.

...assorted codecs FEDORA doesn't ship...

Updates for those:

http://sandeen.fedorapeople.org/ncid/ncid.spec
http://sandeen.fedorapeople.org/ncid/ncid-0.74-2.fc12.src.rpm

-Eric

Jarod, ping? :)

At a quick glance:

- %preun client uses an explicit list of services to stop, but %postun client uses a somewhat odd method of looking at /usr/share/ncid/ncid-* to figure out what to condrestart. I'd make %postun client use the same list as %preun client.

- The %files lists still use /etc, that should be replaced with %{_sysconfdir}

Once more, with feeling! :)

http://sandeen.fedorapeople.org/ncid/ncid.spec
http://sandeen.fedorapeople.org/ncid/ncid-0.74-3.fc12.src.rpm

For consistency's sake, I'd make %_initrddir be %{_initrddir}, but otherwise, I think we've got everything covered now. All the bits in comment #1 are sorted out, as is everything I've brought up, and a breeze through everything one last time looks good.

Package APPROVED.

Thanks Jarod!  I'll fix up that last little bit before committing.

CVS:

New Package CVS Request
=======================
Package Name: ncid
Short Description: Caller ID distributed over a network
Owners: sandeen
Branches: F-11 F-12 EL-5
InitialCC: 

Thanks!

-Eric

cvs done.

ncid-0.74-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/ncid-0.74-3.fc11

ncid-0.74-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/ncid-0.74-3.fc12

ncid-0.74-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.