The value you have selected for %{version} is a bit incorrect. Please read in
http://fedoraproject.org/wiki/Packaging/NamingGuidelines the sections on
"Non-Numeric Version in Release" and most specifically "Pre-Release packages".
The  schema you have used will fail to properly upgrade to a newer, stable
(non-svn) version of the software.
Also, when using sources obtained from a cvs/svn repository, you should document
the method to be used by an independent tester in order to recreate the same
%source as you have used. In this particular case you could for instance include
the instructions from http://test.kernel.org/autotest/QuickStart and timestamp
them, so as to make sure a tester will retrieve the same version as the one you
have included.

The last line in %files is
%{_libdir}/conmux/*
This makes the package architecture-dependent (so it cannot be a -noarch). More
then that, build fails on x86_64 with the following error:
RPM build errors:Processing files: conmux-svn.484-1.fc6
error: File not found by glob:
/var/tmp/conmux-svn.484-1.fc6-root-mockbuild/usr/lib64/conmux/*

Thank you for the feedback.  I've updated the spec file and the patches with
your suggestions.

http://people.redhat.com/bpeck/conmux/conmux-0-0.1.r484.src.rpm
http://people.redhat.com/bpeck/conmux/conmux-0.1.r484.tar.gz
http://people.redhat.com/bpeck/conmux/conmux.spec

version-release is now: 0-0.1.r484

comment in the spec file on how to check out this version of conmux from svn.

and I moved the drivers/helpers from /usr/lib/conmux to /usr/share/conmux since
they are just expect/perl/python scripts.

Offenders at the first sight:

- I think it would have been better if you have used 
version=0 (or even 0.0..)
release=0.484svn <-- the leading 0 would be used as release tag and increased
for each new version of the spec (for a given snapshot release)
This way the fact that we are dealing with svn snapshots is clearly indicated,
even for those which do not examine the spec or do an actual checkout
- The changelog should contain an entry for the current version; for the moment
there is just one entry which speaks about 0-0.1.20070223svn while the submitted
package is 0-0.1.r484; if 20070223svn and 0.1.r484 are one and the same, please
edit the changelog to fit (do not forget to increase the release tag each time
you submit a new form of the spec file!)
- the license tag should be just "GPL" rather then "GPLv2"
- location of the Conmux.pm in conmux-common does not seem right, the site_perl
directory should be one level higher then the one you install to:
#ll  /usr/lib/perl5/
5.8.5/       5.8.6/       5.8.7/       5.8.8/       site_perl/   vendor_perl/
while your spec creates:
#rpm -qlp conmux-common-0-0.1.r484.fc6.noarch.rpm
/etc/conmux/conmux.conf
/usr/lib/perl5/site_perl/5.8.8/Conmux.pm
- the conmux package includes the whole /etc/conmux directory, overlapping with
conmux-common which includes /etc/conmux/conmux.conf; at install time they would
conflict
- the -client and -common packages are missing the mandatory %defattr line
- since you use service and chkconfig to add/remove the Conmux service, you must
add requirements for them (see the packaging guidelines for details on scriptlets)
- if the /etc/init.d/conmux script would contain restart/reload sections,
rpmlint would be happier (that's a nice-to-have, not a must)
- you create /var/log/conmux; it would be nice if you would also provide a means
for logrotate to handle it. However this is open for debate because you'll need
to %Require logrotate, which OTOH some people might prefer to not use (or
replace with something else).

 As a sidenote, I think that the name "console" used by the client is a bit too
generic and might clash with other console clients

Thanks for the additional feedback.  The spec and srpm have been updated.

- Renamed the versioning again 

- and fixed the changelog entry to match.  Oops

- License fixed to GPL (although I would think people will want to specify)

- I'm unclear how I can fix the Conmux.pm since I'm using the rpm macro to
install it.  
%{perl_sitelib}/Conmux.pm

- Fixed the overlapping /etc/conmux/conmux.conf in both base and common.

- Fixed %defattr for common and client

- Added the requires for chkconfig and service

- I'm currently using the maintainers init script with a small patch for
chkconfig.  I'd like to redo it but its a fairly invasive change that would
require a lot of testing

- added logrotate pieces

- I could change the name but this is the name in the upstream package.

Ugh. The package conserver-client contains a /usr/bin/console also. It was there
first, so gotta do something to prevent the namespace collision. I'd probably
make /usr/sbin/conmux into /usr/sbin/conmuxd and make /usr/bin/console into
/usr/bin/conmux.

Question on the Requires: does the base conmux server portion really *require*
the client to be installed to function? If not, I'd say drop that hard Requires.

A few of the comments should be altered slightly, you have "put in our own
initscript and logrotate", but the first file installed is the config file, not
the initscript. Note that the conf file getting installed mode 0644 there also
eliminates the need for the "adjust perms on main config file" comment and
following line.

Not really sure what to do about the .pm file, I try not to touch or even think
about perl... :)

Also, as you update the package from here out, go ahead and bump the package
version each time, along with corresponding changelog entries illustrating
what's been done -- helps make it clear what's been done when, makes it easier
for reviewers to figure out which revision of the package they've got, etc.

http://people.redhat.com/bpeck/conmux/conmux-0-2.484svn.src.rpm
http://people.redhat.com/bpeck/conmux/conmux.spec

conmux renamed to conmuxd
console renamed to conmux

removed erroneous chmod command.

The service conmux status calls console (or conmux now) so the requires on
client is valid.

http://people.redhat.com/bpeck/conmux/conmux-0-1.493svn.src.rpm
http://people.redhat.com/bpeck/conmux/conmux.spec

Updated to newer upstream, dropped socket patch.
Updated perl_sitelib to perl_vendorlib

Can anyone see anything else that I need to fix?

Quick glance:
- GOOD: package builds fine in mock/devel/x86_64
- estethic :
[wolfy@wolfy conmux]$ rpmlint conmux-0-1.493svn.fc7.src.rpm
W: conmux mixed-use-of-spaces-and-tabs (spaces: line 21, tab: line 1)
The fix is obvious...
- MUSTFIX:
rpmlint conmux-0-1.493svn.fc7.noarch.rpm
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/text-base/README.svn-base
W: conmux hidden-file-or-dir /usr/share/doc/conmux-0/examples/.svn
W: conmux hidden-file-or-dir /usr/share/doc/conmux-0/examples/.svn
E: conmux version-control-internal-file /usr/share/doc/conmux-0/examples/.svn/props
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/prop-base/direct.cf.svn-base
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/tmp/prop-base
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/text-base/command.cf.svn-base
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/prop-base
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/text-base/socket.cf.svn-base
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/prop-base/socket.cf.svn-base
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/text-base/direct.cf.svn-base
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/tmp/text-base
E: conmux version-control-internal-file /usr/share/doc/conmux-0/examples/.svn/format
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/tmp/props
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/prop-base/command.cf.svn-base
E: conmux version-control-internal-file /usr/share/doc/conmux-0/examples/.svn/tmp
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/prop-base/README.svn-base
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/text-base
E: conmux version-control-internal-file
/usr/share/doc/conmux-0/examples/.svn/entries

Since there are a couple of other useless .svn directories, I suggest using
something similar to " find /path -name .svn -type d -exec rm -fR {} \;" in
order to remove all of them.
- other messages from rpmlint:
W: conmux no-reload-entry /etc/init.d/conmux
This one can be ignored

E: conmux subsys-not-used /etc/init.d/conmux
I admit I cannot figure out why does rpmlint complain here

- rpmlint warns about no docs in -common and -client. Those can be safely ignored.

- The name of the doc folder is "/usr/share/doc/conmux-0". Is the ending "-0"
really intended? If yes, why? If not...
- Timestamps of the scripts (init.d / logrotate) are not preserved at install
time. Usually adding INSTALL="%{__install} -p" to "make install" fixes that.

http://people.redhat.com/bpeck/conmux/conmux-0.1-2.493svn.src.rpm
http://people.redhat.com/bpeck/conmux/conmux.spec

Ok, I'm finally getting it through my thick skull to run rpmlint *everytime* I
make a change no matter how small. 

- fixed all spaces to be tabs
- removed all .svn entries from the upstream tarball
- I believe I've fixed all install and cp to always use the -p option
- the doc folder is appended the version, which has been 0.  I've changed the
version to 0.1  This doesn't really matter since there has been no upstream release.

Thanks!

Actually, calling it v0.1 could be a problem if upstream does finally put out a
release, and decides to call it something like 0.0.1.

I believe the subsys-not-used bit in the rpmlint output is somehow related to
the use (or non-use) of /var/lock/subsys/ for a lockfile to help the initscript
know when the daemon has been launched.

$ rpmlint -I subsys-not-used
subsys-not-used :
While your daemon is running, you have to put a lock file in
/var/lock/subsys/. To see an example, look at this directory on your
machine and examine the corresponding init scripts.

I'm going to officially take this review now and see it through to completion.
Bill, you mentioned the upstream initscript is a mess... Since we've got some
work to do on it anyway to add proper lock support, how do you feel about going
whole hog and creating a nice, clean, sane initscript?

I was thinking the same thing.  I'm hoping to have an init script ready by
Monday.  The upstream version is far from simple since they spawn a seperate
process per config file.  If you have any suggestions on how to model it so
condreload will work I'm all ears. ;-)

(In reply to comment #13)
> I was thinking the same thing.  I'm hoping to have an init script ready by
> Monday.

Eep! How much work have you done already on the initscript? I started in on the same thing earlier 
today... 
:)


> The upstream version is far from simple since they spawn a seperate
> process per config file.  If you have any suggestions on how to model it so
> condreload will work I'm all ears. ;-)

Hrm... No suggestions on that front just yet. Only additional suggestion I've got at the moment is to 
maybe collapse the package down into just conmux instead of conmux, conmux-common and 
conmux-client, since the sub-packages are so small (and required by the main package anyhow).

http://people.redhat.com/bpeck/conmux/conmux.spec

Changed version from 0.1 to 0.0.
merged common and client subpackage.

http://people.redhat.com/bpeck/conmux/conmux.spec
http://people.redhat.com/bpeck/conmux/conmux-0.0-4.493svn.src.rpm

- Includes Jarod's new init script. But I get this warning:
[bpeck@localhost SPECS]$ rpmlint
/home/bpeck/myBuild/RPMS/noarch/conmux-0.0-4.493svn.noarch.rpm
W: conmux incoherent-subsys /etc/init.d/conmux $DAEMON

I also get this error on the noarch

[bpeck@localhost SPECS]$ rpmlint
/home/bpeck/myBuild/SRPMS/conmux-0.0-4.493svn.src.rpm
W: conmux unversioned-explicit-obsoletes conmux-common
W: conmux unversioned-explicit-provides conmux-common

Not sure if I did the obsoletes correctly.

Check out the output of rpmlint -i for further information on stuff it prints out. This one is partially that 
rpmlint isn't tracking $DAEMON back to its value conmuxd, though if it did, it would probably complain 
because the initscript name is conmux while the lockfile is conmuxd. Probably okay to ignore this one, 
but lemme give you a slightly updated initscript that'll silence that just the same. Well, that or you could 
just update it. Basically, just change 'LOCK=/var/lock/subsys/$DAEMON' to 'LOCK=/var/lock/subsys/
conmux'.

The obsoletes/provides pair should indeed be versioned, like so:
Obsoletes:      conmux-common < %{version}-%{release}
Provides:       conmux-common = %{version}-%{release}

More review fun tomorrow, time for some commerial-free, pre-recorded HDTV viewing... ;)

Just noticed something else in the spec: its got 'service conman condrestart' instead of 'service conmux 
condrestart'... :)

http://people.redhat.com/bpeck/conmux/conmux.spec
http://people.redhat.com/bpeck/conmux/conmux-0.0-5.493svn.src.rpm

Fixed obsoletes/provides
updated init script
and service conmuxd condrestart, gee wonder what spec file I was looking at. ;-)

> gee, wonder what spec file I was looking at. ;-)

I have absolutely no idea. :D

Here's the results of a full pass over the spec and resulting packages:

* source files match upstream: n/a, its an svn checkout

* package meets naming and versioning guidelines
* specfile is properly named, is cleanly written and uses macros consistently
  - only consistency issue I see is 'rm -rf $RPM_BUILD_ROOT' in one place, but
'rm -rf "$RPM_BUILD_ROOT"' in another, which is inconsequential, but you get
extra style points if they're made to match. :)

* dist tag is present
* build root is acceptable
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
* license field matches the actual license: GPL
* license is open source-compatible: GPL License text included in package.
* latest version is being packaged: no release version yet, acceptable use of
svn checkout
* BuildRequires are proper
* compiler flags are appropriate: n/a, its perl noarch stuff
* %clean is present
* package builds in mock (F7/x86_64)
* package installs properly
* debuginfo package looks complete: n/a, its noarch
* rpmlint is silent:
  - only a W: for conmux-client containing no docs. You *could* add the main
README to the conmux-client sub-package if desired (could be useful in the case
where a system only has the client), but its not a requirement.
* final provides and requires are sane:
  conmux provides:
  ----------------
  config(conmux) = 0.0-5.493svn.fc6
  conmux = 0.0-5.493svn.fc6

  conmux requires:
  ----------------
  config(conmux) = 0.0-5.493svn.fc6
  conmux-client = 0.0-5.493svn.fc6
  logrotate  
  perl  
  perl(Conmux)  
  perl(Getopt::Long)  
  perl(IO::Multiplex)  
  perl(IO::Socket)  
  perl(IPC::Open3)  
  perl(Net::Domain)  
  perl(Symbol)  
  perl(URI::Escape)  
  perl(base)  
  perl(lib)  
  perl(strict)

  conmux-client provides:
  -----------------------
  config(conmux-client) = 0.0-5.493svn.fc6
  conmux-common = 0.0-5.493svn.fc6
  perl(Conmux)  
  perl(Conmux::Registry)  
  conmux-client = 0.0-5.493svn.fc6

  conmux-client requires:
  -----------------------
  /usr/bin/perl  
  config(conmux-client) = 0.0-5.493svn.fc6
  perl(Conmux)  
  perl(Getopt::Long)  
  perl(IO::Socket)  
  perl(POSIX)  
  perl(URI::Escape)

  Looks sane enough to me.

* %check is present and all tests pass: n/a
* no shared libraries are added to the regular linker search paths
* owns the directories it creates
* doesn't own any directories it shouldn't
* no duplicates in %files
* file permissions are appropriate
* scriptlets are appropriate
* code, not content
* documentation is small, so no -docs subpackage is necessary
* %docs are not necessary for the proper functioning of the package.
* no headers
* no pkgconfig files
* no libtool .la droppings
* not a GUI app

Only the two minor (non-blocking) issues I raised above, so this package is
APPROVED, and I'll sponsor you. Next up, you need to create an account in the
Fedora Account System and jump through a few hoops to get to the point where you
can check the package in to be built. Basically, continue from here:

http://fedoraproject.org/wiki/Extras/Contributors#head-a89c07b5b8abe7748b6b39f0f89768d595234907

ref 
>* source files match upstream: n/a, its an svn checkout
As far as I have understood (please correct me if I am wrong) here "n/a" is not
correct: the reviewer is supposed to do a svn checkout of the same tag
(instructions for that should be included as comment in the spec) and compare
with the file included in the src.rpm. If not the tar.gz, at least a diff -r
should coincide...

Jarod, since you have decided to sponsor Bill, the FE-NEEDSPONSOR flag should be
removed.

(In reply to comment #21)
> ref 
> >* source files match upstream: n/a, its an svn checkout
> As far as I have understood (please correct me if I am wrong) here "n/a" is not
> correct: the reviewer is supposed to do a svn checkout of the same tag
> (instructions for that should be included as comment in the spec) and compare
> with the file included in the src.rpm. If not the tar.gz, at least a diff -r
> should coincide...

I believe you're correct. I was thinking n/a, since there's no upstream tarball
to grab, but yeah, diff'ing svn checkouts should be done in place of tarball
checksumming.

$ diff -r conmux conmux-r493
Only in conmux-r493/drivers: .svn
Only in conmux-r493/examples: .svn
Only in conmux-r493/helpers: .svn
Only in conmux-r493: .svn

Bill's tarball has the .svn dirs removed, which is fine, and everything else
appears to match.

Went ahead and dropped the FE-NEEDSPONSOR flag too.

New Package CVS Request
=======================
Package Name: conmux
Short Description: conmux is a serial multiplexor which also abstracts the hardware
Owners: bpeck
Branches: EL-5, FC-5, FC-6
InitialCC: 

Package was built for all distros a while ago, closing bug.