Bug 433161

Hi Bob,

I have a few quick questions and minor suggested changes while I do a more
detailed review.

/usr/sbin should be replaced with %{_sbindir} in the file list

Adding a space between the changelog entries helps things like "make clog" work
better and makes the changelog easier to read.

Why is the devel package needed?  What is the purpose of the cwtest.c file?
(This might be irrelevant if a devel package isn't really needed, but why does
the devel package Require pkgconfig, it does not ship a .pc file?)

The README section on testing says to run test/cwtest.sh, but this location
won't exist for users of the rpm.  Is the test really needed?  If so, the
package would need to depend on nc (netcat) for it to work.  And the README
should probably be changed to refer to the location that the test script is
installed.  (Unless the test is really needed, I'd be inclined to not package
it.)

The manpage says to see @prefix@/share/cwdaemon/README for a description of the
circuitry, usage and testing of cwdaemon.  But this file isn't installed (it's
removed in the spec file).  Perhaps a symlink should be created?  Something
like:

    ln -sv %{docdir}/%{name}-%{version}/README %{_datadir}/%{name}/README

should create the link.  Then include it in the file list.

I agree on the changelog line breaks, change made.

I also fixed the sbin macro and created the link for the second README location.

We are not sure if other packages will need the test.c that is why we included
the -devel with that. We have been unable to find any software that needs it but
we have only looked at about 10% of the ham related software available.

(In reply to comment #1)
>     ln -sv %{docdir}/%{name}-%{version}/README %{_datadir}/%{name}/README
> 
> should create the link.  Then include it in the file list.

Well, current spec file
---------------------------------------------------------------------
#don't include this twice
rm -rf $RPM_BUILD_ROOT%{_datadir}/%{name}/README
#Lets make a simlink instead
ln -sv %{docdir}/%{name}-%{version}/README %{_datadir}/%{name}/README
............................
%files
%doc AUTHORS ChangeLog COPYING README TODO
............................
%{_datadir}/%{name}/README
---------------------------------------------------------------------
is wrong because of the following reason.

From
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
- MUST: If a package includes something as %doc, it must not 
         affect the runtime of the application. To summarize: 
         If it is in %doc, the program must run properly 
         if it is not present.

In other words, a package should work properly with
"rpm -ivh --excludedocs". For this package when "--excludedocs" is used
- %{docdir}/%{name}-%{version}/README is not installed
- But the symlink %{_datadir}/%{name}/README is created
  this causes this symlink broken, which must be avoided.

(In reply to comment #4)

Updated the spec again. Original srpm still, will rebuild if you need. 

Curse bugzilla.  It just ate a ton of comments I'd made. :(

Mamoru is right about the link being a bad idea.  Sorry about that.  It might be
best to just patch the man page.  Maybe I'm being pedantic, but I hate it when
man pages and other docs point me to things that don't exist.

I'm still unclear on the point of the devel package.  I don't see how any other
apps are expected to make use of this single c source file.  Without knowing how
apps use it, I don't see any reason to include it.  Can you ask upstream or find
some apps that use it as is?  I imagine that the intent is to have it compiled
to be of use.

I am also curious how the daemon is to be used, whether it's a long running
daemon, run from xinetd, or something kicked off from the command line as
needed?  Bob, you mentioned in #fedora-devel that other apps would start it on
demand.  Can you elaborate a bit on that and perhaps point to some examples? 
(Please pardon my ignorance as a non-ham. :)

Looking at the README, it directs users to see "the examples in the schematics
directory."  But that refers to a dir in the tarball.  The files it's referring
to are installed in %{_datadir}/%{name}.  It seems to me that those examples
belong in %docdir instead (and the README updated accordingly so readers can
find the files easily).

That leaves cwdaemon.png and two shell scripts in %{_datadir}/%{name}
(cwsetup.sh and cwtest.sh).  The png could also be put into %docdir I believe
(it's not used for anything, is it?).  Are either of the shell scripts needed if
the rpm does the setup correctly?  cwsetup looks mostly useless for the package.
 cwtest.sh may be of use.  It seems that these script, if needed, should be
installed somewhere in $PATH.  cwtest.sh requires nc as well.

One other small thing, it's better to use %{_mandir}/man8/%{name}.8.gz for the
manpage instead of %{_datadir}/man.

qle is a package that Requires cwdaemon we have a srpm and spec started at
http://bjensen.fedorapeople.org/pkgs/hams/#qle

(In reply to comment #6)

> I am also curious how the daemon is to be used, whether it's a long running
> daemon, run from xinetd, or something kicked off from the command line as
> needed?  Bob, you mentioned in #fedora-devel that other apps would start it on
> demand.  Can you elaborate a bit on that and perhaps point to some examples? 
> (Please pardon my ignorance as a non-ham. :)
> 
See my above link to qle.

> Looking at the README, it directs users to see "the examples in the schematics
> directory."  But that refers to a dir in the tarball.  The files it's referring
> to are installed in %{_datadir}/%{name}.  It seems to me that those examples
> belong in %docdir instead (and the README updated accordingly so readers can
> find the files easily).
> 
> That leaves cwdaemon.png and two shell scripts in %{_datadir}/%{name}
> (cwsetup.sh and cwtest.sh).  The png could also be put into %docdir I believe
> (it's not used for anything, is it?).  Are either of the shell scripts needed if
> the rpm does the setup correctly?  cwsetup looks mostly useless for the package.
>  cwtest.sh may be of use.  It seems that these script, if needed, should be
> installed somewhere in $PATH.  cwtest.sh requires nc as well.
> 

I do not know how to change the README file, I have asked my mentor for
assistance. In the mean time I updated SPEC and SRPM

http://bjensen.fedorapeople.org/pkgs/hams/SPECS/cwdaemon.spec
http://bjensen.fedorapeople.org/pkgs/hams/SRPMS/cwdaemon-0.9.4-6.fc8.src.rpm

(In reply to comment #7)
> qle is a package that Requires cwdaemon we have a srpm and spec started at
> http://bjensen.fedorapeople.org/pkgs/hams/#qle

The qle docs suggest that cwdaemon should be running as a daemon.  qle doesn't
start it for you.  The Debian cwdaemon packages setup a service in /etc/init.d.  

There is a draft guide for creating LSB compliant init scripts at:
http://fedoraproject.org/wiki/PackagingDrafts/SysVInitScript

I don't know if there are any other pages in the wiki with such examples or not.
 You can also look at many of the scripts in /etc/init.d to see how things are
done currently.

(In reply to comment #8)
> I do not know how to change the README file, I have asked my mentor for
> assistance.

The Debian package installs a README.Debian in %docdir, which tells the user a
little about how to start the service and use it in Debian.  It also points the
user to the README in %{_datadir}/%{name}.  That might be a reasonable way to go
here too.

Also worth noting is that the Debian maintainer is the upstream cwdaemon
maintainer.  So if you run into any general issues that should be fixed to make
the software more easily packaged, he may be amenable to making those changes
upstream -- assuming the changes are generic and not Fedora specific.

> In the mean time I updated SPEC and SRPM.

With the devel package removed, the %files section can be simplified a bit. 
There's no need to use separate globs for the files in %{_datadir}/%{name}. 
Instead of:

%dir %{_datadir}/%{name}/
%{_datadir}/%{name}/*.ps
%{_datadir}/%{name}/*.jpg
%{_datadir}/%{name}/*.sh
%{_datadir}/%{name}/*.png

You could just use:

%{_datadir}/%{name}

Todd,

Thank you for reviewing this package for me. I am going to close this review for
now, I just do not have the resources/skills/information needed at this time to
take this package on. I will keep the spec and srpms hosted if some other party
would like to look at it.

Hello,

I'm taking over this request.

I've moved schematics to doc directory, updated README file to specify correct location and added initial configuration file and sysvinit script to startup cwdaemon...

... and finally bumped version.
New versions:

http://lucilanga.fedorapeople.org/cwdaemon.spec
http://lucilanga.fedorapeople.org/cwdaemon-0.9.4-7.fc9.src.rpm

I would suggest opening your own review ticket and closing this one as a duplicate of it.

*** This bug has been marked as a duplicate of bug 458585 ***