Bug 459855 - (ncid) Review Request: ncid - Caller ID distributed over a network to a variety of devices
Review Request: ncid - Caller ID distributed over a network to a variety of d...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jarod Wilson
Fedora Extras Quality Assurance
http://ncid.sourceforge.net
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-22 22:11 EDT by Eric Sandeen
Modified: 2009-10-20 20:53 EDT (History)
6 users (show)

See Also:
Fixed In Version: 0.74-3.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-10-20 20:53:07 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jarod: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Eric Sandeen 2008-08-22 22:11:38 EDT
Spec URL: http://sandeen.fedorapeople.org/ncid/ncid.spec
SRPM URL: http://sandeen.fedorapeople.org/ncid/ncid-0.71-1.fc8.src.rpm
Description:
NCID is Caller ID (CID) distributed over a network to a variety of devices
and computers. NCID consists of a server called ncidd, a universal client
called ncid, various client output modules, two SIP gateways, and a YAC
gateway.

The server, ncidd, monitors either a modem, device or gateway for the
CID data. The data is collected and sent, via TCP, to one or more connected
clients.

The client, ncid, normally displays the Caller ID data and the ServerCaller
ID log in a GUI window.  The client output can be changed with output modules.
One module can speak the CID, another can send the CID to a pager or cell
phone.  There are other output modules, including ones that display the CID
on a TiVo or MythTV.

The SIP gateways obtain the Caller ID information from a VOIP system, using
SIP Invite. The YAC gateway obtains the Caller ID information from a YAC
server.

---------------

I've broken up the sub-packages quite a lot, because they depend on things like kde, samba, festival... things which are rather large and not everyone will want to pull in.

Hope it looks ok :)

Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=779926

Thanks,
-Eric
Comment 1 Andreas Thienemann 2008-10-20 13:50:42 EDT
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. :)
Comment 2 Eric Sandeen 2008-10-20 14:41:28 EDT
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
Comment 3 Andreas Thienemann 2008-10-20 15:05:08 EDT
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
Comment 4 Eric Sandeen 2008-11-02 22:10:31 EST
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?
Comment 5 Eric Sandeen 2008-11-02 22:21:03 EST
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
Comment 6 Eric Sandeen 2009-03-29 12:04:50 EDT
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.
Comment 7 Eric Sandeen 2009-03-29 18:22:45 EDT
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
Comment 8 Eric Sandeen 2009-07-29 21:48:31 EDT
Andreas, care to take another look at those updated packages?

thanks,
-eric
Comment 10 Eric Sandeen 2009-08-08 12:30:03 EDT
Andreas, ping?  I think we're close to having this done.  :)
Comment 11 Eric Sandeen 2009-09-03 22:49:03 EDT
Andreas, ping?
Comment 12 Jarod Wilson 2009-09-04 15:05:06 EDT
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.
Comment 13 Jarod Wilson 2009-09-04 15:07:56 EDT
...assorted codecs FEDORA doesn't ship...
Comment 15 Eric Sandeen 2009-09-28 11:25:42 EDT
Jarod, ping? :)
Comment 16 Jarod Wilson 2009-09-29 11:33:21 EDT
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}
Comment 18 Jarod Wilson 2009-10-16 21:34:19 EDT
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.
Comment 19 Eric Sandeen 2009-10-16 22:40:34 EDT
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
Comment 20 Kevin Fenzi 2009-10-17 16:41:31 EDT
cvs done.
Comment 21 Fedora Update System 2009-10-18 12:41:53 EDT
ncid-0.74-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/ncid-0.74-3.fc11
Comment 22 Fedora Update System 2009-10-18 12:43:05 EDT
ncid-0.74-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/ncid-0.74-3.fc12
Comment 23 Fedora Update System 2009-10-20 20:52:57 EDT
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.

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