Bug 214830 - Review Request: Limph - PHP network host monitor
Review Request: Limph - PHP network host monitor
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orion Poplawski
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-11-09 12:26 EST by Jon Ciesla
Modified: 2007-11-30 17:11 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-01-12 07:41:21 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)
patch to spec file (1.57 KB, patch)
2006-11-22 18:36 EST, Orion Poplawski
no flags Details | Diff

  None (edit)
Description Jon Ciesla 2006-11-09 12:26:57 EST
Spec URL: http://gryffindor.jcomserv.net/extras/limph/limph.spec
SRPM URL: http://gryffindor.jcomserv.net/extras/limph/limph-1.9.3-1.src.rpm
Description: Limph is a PHP5 compatible network host monitor with host agent support.  This is my second submitted package, and I am still unsponsored.
Comment 1 Orion Poplawski 2006-11-16 16:21:11 EST
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?

Comment 2 Jon Ciesla 2006-11-17 09:21:53 EST
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.
Comment 3 Orion Poplawski 2006-11-22 18:36:02 EST
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.
Comment 4 Jon Ciesla 2006-11-27 14:00:36 EST
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.
> 

Comment 5 Jon Ciesla 2006-11-27 14:05:56 EST
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.
> > 
> 
> 

Comment 6 Jon Ciesla 2006-11-27 14:21:45 EST
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.
> > > 
> > 
> > 
> 
> 

Comment 7 Jon Ciesla 2006-12-12 09:18:51 EST
Is there anything further I need to do here, or were my most recent changes
sufficient?
Comment 8 Jon Ciesla 2007-01-09 14:07:49 EST
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.
Comment 9 Orion Poplawski 2007-01-09 14:27:41 EST
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.
Comment 10 Jon Ciesla 2007-01-09 14:43:15 EST
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
Comment 11 Orion Poplawski 2007-01-09 15:23:41 EST
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.
Comment 12 Jon Ciesla 2007-01-09 15:41:32 EST
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
Comment 13 Orion Poplawski 2007-01-09 15:54:15 EST
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.
Comment 14 Jon Ciesla 2007-01-09 16:08:18 EST
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
Comment 15 Orion Poplawski 2007-01-09 16:13:59 EST
In main %files section, should be:

%{limphdir}/*

so it doesn't own the directory.
Comment 17 Orion Poplawski 2007-01-09 16:35:46 EST
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
Comment 18 Michael Schwendt 2007-01-09 17:17:31 EST
* %{_sysconfdir}/limph/ is not included
Comment 19 Orion Poplawski 2007-01-09 17:19:39 EST
(In reply to comment #18)
> * %{_sysconfdir}/limph/ is not included

Gah, thanks Michael!
Comment 21 Jon Ciesla 2007-01-10 12:23:10 EST
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@jcomserv.net.  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?
Comment 22 Jon Ciesla 2007-01-11 14:01:26 EST
Imported, owners.list updated, branches requested.
Comment 23 Jon Ciesla 2007-01-12 07:41:21 EST
Build in devel.  Will build in FC-6/5 when branches are complete. Closing.
Comment 24 Jon Ciesla 2007-07-12 11:22:55 EDT
Package Change Request
======================
Package Name: limph
New Branches: EL-4 EL-5
Comment 25 Kevin Fenzi 2007-07-12 12:51:41 EDT
cvs done.

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