Initial comments:

- BAD: md5sum does not match upstream.

- Need full URL for source (http://prdownloads.sf.net/limph/limph-%{version}.tar.gz

- Please, BuildArch: not BuildArchitectures:

$ rpmlint ~/Desktop/limph-1.9.3-1.src.rpm
W: limph strange-permission limph-hostagent 0755

- perhaps just change the permission with "install"?

E: limph use-of-RPM_SOURCE_DIR

- Instead of "$RPM_SOURCE_DIR/limph.conf", use %SOURCE1, etc.

- should add to limph.conf:

<Directory /usr/share/limph/>
        Order Deny,Allow
        Deny from all
        Allow from 127.0.0.1
</Directory>

- More rpmlint:

# rpmlint limph
E: limph file-in-usr-marked-as-conffile /usr/share/limph/config.php
E: limph file-in-usr-marked-as-conffile /usr/share/limph/input.php

- move to /etc/limph.  Is input.php really a config file?

E: limph world-writable /usr/share/limph/tmp 0777
E: limph non-standard-dir-perm /usr/share/limph/tmp 0777

- These should only be writable by limph.

# rpmlint limph-hostagent
E: limph-hostagent file-in-usr-marked-as-conffile /usr/share/limph/host_agent.php

- same here.  Maybe put $secret into a config file in /etc/limph and include that?

Spec URL: http://gryffindor.jcomserv.net/extras/limph/limph-2.spec
SRPM URL: http://gryffindor.jcomserv.net/extras/limph/limph-1.9.3-2.src.rpm

I addressed the source BuildArch, URL, RPM_SOURCE_DIR, limph.conf issues.  I'm
trying to solve the conf file issues with symlinks and it's not working, nor is
my %attr for the tmp directory applying the desired ownership.  What am I doing
wrong?

The reason the MD5 doesn't match upstream is the folder name in the source
tarball is limph, not limph-1.9.3, so I untarred, renamed and retarred to get it
to build.  Should I re-release a 1.9.3.1 with this fixed, or is there a way
around it in the SPEC?

Thanks for the help, BTW.

Created attachment 141960 [details]
patch to spec file

Here's a patch that resolves some issues.

To fix the tar ball issue:

%setup -q -n %{name}

Then go back to %setup -q when you fix that.

Other issues:
- still got some config files in /usr.	See my earlier suggestion about maybe
having them include the password information from a common file in /etc/limph.
- tmp dir is still wrong.  Do you really need your own tmp dir?  Can't be in
/usr in any case.

Thanks, terribly helpful patch.  I've applied it, released upstream version
1.9.4 to address the config file, tmp dir and tarball issues, updated spec
accordingly, rebuilt, and posted the results:

Spec URL: http://gryffindor.jcomserv.net/extras/limph/limph-3.spec
SRPM URL: http://gryffindor.jcomserv.net/extras/limph/limph-1.9.4-1.src.rpm

(In reply to comment #3)
> Created an attachment (id=141960) [edit]
> patch to spec file
> 
> Here's a patch that resolves some issues.
> 
> To fix the tar ball issue:
> 
> %setup -q -n %{name}
> 
> Then go back to %setup -q when you fix that.
> 
> Other issues:
> - still got some config files in /usr.	See my earlier suggestion about maybe
> having them include the password information from a common file in /etc/limph.
> - tmp dir is still wrong.  Do you really need your own tmp dir?  Can't be in
> /usr in any case.
> 

Hang on, scratch those, I forgot to migrate one thing to a different file.  Give
me a few minutes. . . .

(In reply to comment #4)
> Thanks, terribly helpful patch.  I've applied it, released upstream version
> 1.9.4 to address the config file, tmp dir and tarball issues, updated spec
> accordingly, rebuilt, and posted the results:
> 
> Spec URL: http://gryffindor.jcomserv.net/extras/limph/limph-3.spec
> SRPM URL: http://gryffindor.jcomserv.net/extras/limph/limph-1.9.4-1.src.rpm
> 
> (In reply to comment #3)
> > Created an attachment (id=141960) [edit] [edit]
> > patch to spec file
> > 
> > Here's a patch that resolves some issues.
> > 
> > To fix the tar ball issue:
> > 
> > %setup -q -n %{name}
> > 
> > Then go back to %setup -q when you fix that.
> > 
> > Other issues:
> > - still got some config files in /usr.	See my earlier suggestion about maybe
> > having them include the password information from a common file in /etc/limph.
> > - tmp dir is still wrong.  Do you really need your own tmp dir?  Can't be in
> > /usr in any case.
> > 
> 
> 

Ok, the above links are again valid.  I had to re-do upstream since I forgot to
migrate a chunk of host_agent.php to config.php.  All better now, but I didn't
bump the release number, if I need to, let me know.

(In reply to comment #5)
> Hang on, scratch those, I forgot to migrate one thing to a different file.  Give
> me a few minutes. . . .
> 
> (In reply to comment #4)
> > Thanks, terribly helpful patch.  I've applied it, released upstream version
> > 1.9.4 to address the config file, tmp dir and tarball issues, updated spec
> > accordingly, rebuilt, and posted the results:
> > 
> > Spec URL: http://gryffindor.jcomserv.net/extras/limph/limph-3.spec
> > SRPM URL: http://gryffindor.jcomserv.net/extras/limph/limph-1.9.4-1.src.rpm
> > 
> > (In reply to comment #3)
> > > Created an attachment (id=141960) [edit] [edit] [edit]
> > > patch to spec file
> > > 
> > > Here's a patch that resolves some issues.
> > > 
> > > To fix the tar ball issue:
> > > 
> > > %setup -q -n %{name}
> > > 
> > > Then go back to %setup -q when you fix that.
> > > 
> > > Other issues:
> > > - still got some config files in /usr.	See my earlier suggestion about maybe
> > > having them include the password information from a common file in /etc/limph.
> > > - tmp dir is still wrong.  Do you really need your own tmp dir?  Can't be in
> > > /usr in any case.
> > > 
> > 
> > 
> 
> 

Is there anything further I need to do here, or were my most recent changes
sufficient?

It's been over a month since the last action on this review, mine or otherwise.
 I recognize that we're just coming off the holidays, but this
http://fedoraproject.org/wiki/Extras/Policy/StalledReviews would seem indicate
that this review is stalled.  

I think I've fulfilled the stated requests, and welcome correction if I have not.

Sorry for the delay.  On travel and then the holidays.

- $ rpmlint -i limph-hostagent-1.9.4-1.fc6.noarch.rpm
E: limph-hostagent executable-marked-as-config-file /etc/cron.hourly/limph-hostagent
Executables must not be marked as config files because that may
prevent upgrades from working correctly. If you need to be able to
customize an executable, make it for example read a config file in
/etc/sysconfig.

- You're still creating the tmp dir:

mkdir -p %{buildroot}%{limphdir}/tmp

- Both limph and limph-hostagent own /usr/share/limph/host_agent.php.  You need
an %exclude in the limph files section.

- Does limph-hostagent require limph to run?  Looks like it does need
config.php.  Add the appropriate requires.

Thanks for the quick response.  I've addressed the config and tmp issues, and
the exclude.  

Limph-hostagent does not require Limph to run, it includes it's own config.php.
 Is this a problem? Do they need to be in two seperate places, like /etc/limph
and /etc/limph-hostagent for example?

Links to the current versions:
 Spec URL: http://gryffindor.jcomserv.net/extras/limph/limph-4.spec
> > SRPM URL: http://gryffindor.jcomserv.net/extras/limph/limph-1.9.4-2.src.rpm

But both limph and limph-hostagent use and own the same config file.  This is
not allowed.  What you could do is create limph-common to own
/etc/limph/config.php and have both limph and limph-hostagent require that.

I see.  I've done that.  Now rpmlint -i is clean for all 3.

Links to the current versions:
Spec URL: http://gryffindor.jcomserv.net/extras/limph/limph-5.spec
SRPM URL: http://gryffindor.jcomserv.net/extras/limph/limph-1.9.4-3.src.rpm

Requires should be:

Requires: limph-common = %{version}-%{release}

And one more problem, %limphdir needs to be owned by limph-common as well and
not by limph.

Ok.  Does this do it for %limphdir?  I think I specified the files and the
directory properly.  I also re-included the config.php symlink.

Spec URL: http://gryffindor.jcomserv.net/extras/limph/limph-6.spec
SRPM URL: http://gryffindor.jcomserv.net/extras/limph/limph-1.9.4-4.src.rpm

In main %files section, should be:

%{limphdir}/*

so it doesn't own the directory.

So like this?:
Spec URL: http://gryffindor.jcomserv.net/extras/limph/limph-7.spec
SRPM URL: http://gryffindor.jcomserv.net/extras/limph/limph-1.9.4-5.src.rpm

That's the ticket.

Good:

- rpmlint checks return nothing
- package meets naming guidelines
- package meets packaging guidelines
- license (GPL) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

APPROVED

* %{_sysconfdir}/limph/ is not included

(In reply to comment #18)
> * %{_sysconfdir}/limph/ is not included

Gah, thanks Michael!

Spec URL: http://gryffindor.jcomserv.net/extras/limph/limph-8.spec
SRPM URL: http://gryffindor.jcomserv.net/extras/limph/limph-1.9.4-6.src.rpm

Done.

So I'm sponsored in cvsextras (thank you!), I've got the various certs, CVS and
plague set up.  

However:
[limb@zanoni cvs]$ cvs co common
Permission denied (publickey,keyboard-interactive).
cvs [checkout aborted]: end of file from server (consult above messages if any)

Any thoughts?  I'm using limb as the username in my cvsroot env entry in
.bash_profile, and not limb.  I've tried both, however.  I'm
guessing limb is correct due to syntax (I'm a Subversion user and emarrasingly
new to CVS).  Has my new permission status just not propagated yet?

Imported, owners.list updated, branches requested.

Build in devel.  Will build in FC-6/5 when branches are complete. Closing.

Package Change Request
======================
Package Name: limph
New Branches: EL-4 EL-5

cvs done.