Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 214830 - Review Request: Limph - PHP network host monitor
Summary: Review Request: Limph - PHP network host monitor
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Orion Poplawski
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-11-09 17:26 UTC by Gwyn Ciesla
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-01-12 12:41:21 UTC
Type: ---
Embargoed:
kevin: fedora-cvs+


Attachments (Terms of Use)
patch to spec file (deleted)
2006-11-22 23:36 UTC, Orion Poplawski
no flags Details | Diff

Description Gwyn Ciesla 2006-11-09 17:26:57 UTC
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 21:21:11 UTC
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 Gwyn Ciesla 2006-11-17 14:21:53 UTC
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 23:36:02 UTC
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 Gwyn Ciesla 2006-11-27 19:00:36 UTC
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 Gwyn Ciesla 2006-11-27 19:05:56 UTC
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 Gwyn Ciesla 2006-11-27 19:21:45 UTC
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 Gwyn Ciesla 2006-12-12 14:18:51 UTC
Is there anything further I need to do here, or were my most recent changes
sufficient?

Comment 8 Gwyn Ciesla 2007-01-09 19:07:49 UTC
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 19:27:41 UTC
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 Gwyn Ciesla 2007-01-09 19:43:15 UTC
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 20:23:41 UTC
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 Gwyn Ciesla 2007-01-09 20:41:32 UTC
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 20:54:15 UTC
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 Gwyn Ciesla 2007-01-09 21:08:18 UTC
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 21:13:59 UTC
In main %files section, should be:

%{limphdir}/*

so it doesn't own the directory.

Comment 17 Orion Poplawski 2007-01-09 21:35:46 UTC
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 22:17:31 UTC
* %{_sysconfdir}/limph/ is not included

Comment 19 Orion Poplawski 2007-01-09 22:19:39 UTC
(In reply to comment #18)
> * %{_sysconfdir}/limph/ is not included

Gah, thanks Michael!

Comment 21 Gwyn Ciesla 2007-01-10 17:23:10 UTC
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?

Comment 22 Gwyn Ciesla 2007-01-11 19:01:26 UTC
Imported, owners.list updated, branches requested.

Comment 23 Gwyn Ciesla 2007-01-12 12:41:21 UTC
Build in devel.  Will build in FC-6/5 when branches are complete. Closing.

Comment 24 Gwyn Ciesla 2007-07-12 15:22:55 UTC
Package Change Request
======================
Package Name: limph
New Branches: EL-4 EL-5

Comment 25 Kevin Fenzi 2007-07-12 16:51:41 UTC
cvs done.


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