Bug 180747 - Review Request: powerman
Review Request: powerman
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jarod Wilson
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-02-09 20:30 EST by Ben Woodard
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-06-21 17:03:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Diff from Ben's latest spec to Jarod's spec (3.81 KB, patch)
2006-06-14 01:19 EDT, Jarod Wilson
no flags Details | Diff

  None (edit)
Description Ben Woodard 2006-02-09 20:30:37 EST
Spec Name or Url: http://osdn.dl.sourceforge.net/sourceforge/powerman/powerman.spec
SRPM Name or Url: http://osdn.dl.sourceforge.net/sourceforge/powerman/powerman-1.0.22-2.src.rpm

Description: 
PowerMan is a tool for manipulating remote power control (RPC) devices from a
central location. Several RPC varieties are supported natively by PowerMan and
Expect-like configurability simplifies the addition of new devices.

I will need a sponsor for this package.
Comment 1 Jochen Schmitt 2006-02-13 11:40:23 EST
I have found some issue:

- Source should cotain a full-qualified URL.
- BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
- /usr/bin should be replace by %{_bindir}
- /usr/sbin should be replace by %{_sbindir}
- /usr/man should be replace by %{_mandir}
- /etc/rc.d/init.d/ shoud be replace by %{_initrddir}
- %doc should contain a verbatin copy of the license used by this package.
- local build failed.

Error messages:

make[1]: Leaving directory `/home/pclinux/redhat/BUILD/powerman-1.0.22-1/src'
/usr/bin/make -C test
make[1]: Entering directory `/home/pclinux/redhat/BUILD/powerman-1.0.22-1/test'
cc -g -Wall -I../src -DHAVE_CONFIG_H   -c -o vpcd.o vpcd.c
vpcd.c: In function '_vpc_thread':
vpcd.c:286: warning: pointer targets in passing argument 3 of 'accept' differ in
signedness
vpcd.c: In function 'main':
vpcd.c:393: error: 'PTHREAD_THREADS_MAX' undeclared (first use in this function)
vpcd.c:393: error: (Each undeclared identifier is reported only once
vpcd.c:393: error: for each function it appears in.)
make[1]: *** [vpcd.o] Error 1
make[1]: Leaving directory `/home/pclinux/redhat/BUILD/powerman-1.0.22-1/test'
make: *** [tests] Error 2

Best Regards:

Jochen Schmitt
Comment 2 Ben Woodard 2006-02-14 23:32:46 EST
Thank you for taking the time to review powerman's rpm.
I fixed the things mentioned in your review. There is now a new src.rmp and a
new spec file up there now.

http://osdn.dl.sourceforge.net/sourceforge/powerman/powerman-1.0.22-3.src.rpm
http://osdn.dl.sourceforge.net/sourceforge/powerman/powerman.spec

I'm not yet sure that it will compile perfectly on FC4 and do not have a test
machine available to test that this evening. I'm working with the upstream
author to make sure that this will work and as soon as I can verify this. I'll
update this ticket.

In the mean time, will you please verify that the spec file is now done to your
satisfaction.
Comment 3 Ben Woodard 2006-02-16 18:47:36 EST
The upstream author fixed powerman so that it will compile on FC4 cleanly. I
rolled a new release of the packages. Could you run this through the review
process again.

http://osdn.dl.sourceforge.net/sourceforge/powerman/powerman.spec
http://osdn.dl.sourceforge.net/sourceforge/powerman/powerman-1.0.23-2.src.rpm
Comment 4 Ben Woodard 2006-02-28 11:47:08 EST
Here is a new version of powerman rpm and spec file. This fixes all the problems
that rpmlint turned up in all the rpms. Previously I had only done this for the
src rpm.

http://osdn.dl.sourceforge.net/sourceforge/powerman/powerman.spec
http://osdn.dl.sourceforge.net/sourceforge/powerman/powerman-1.0.23-3.src.rpm

Comment 5 Ben Woodard 2006-03-21 18:53:51 EST
Can someone take a look at this and either approve it or tell me why it
shouldn't be approved.
Comment 6 Christopher Stone 2006-03-21 19:59:53 EST
This should not be approved because it does not build under mock.  You need to
add tcp_wrappers to the BuildRequires in order to get it to build.

rpmlint is still giving a lot of errors and warnings...
you are hard coding /etc/powerman you should use %config and %{_sysconfdir}

Also your directory permissions dont look correct in your %files section, any
reason why you dont use: %defattr(-,root,root,-)
Comment 7 Ben Woodard 2006-04-21 21:01:08 EDT
Finally had some time to work on this.

I fixed the problem with the mock build and uploaded a new set of packages.

With regards to the other problems. I believe you must have been looking at an
older version of the spec file.

There are no errors or warnings coming from rpmlint:

[ben@flix result]$ pwd
/var/lib/mock/fedora-development-i386-core/result
[ben@flix result]$ rpmlint powerman-*
[ben@flix result]$

Defattr is set to (-,root,root):

[ben@flix result]$ grep defattr /tmp/powerman.spec
%defattr(-,root,root)
- changed defattr which was interfering with perms of included files.
[ben@flix result]$

There are no places where we have hard coded /etc and we are using _sysconfdir:

[ben@flix result]$ grep etc /tmp/powerman.spec
- change perms on files in etc/powerman so that they don't look like scripts
- changed etc to sysconfdir macro.
- Changed /etc/rc.d/init.d/ to initrddir
[ben@flix result]$ grep sysconf /tmp/powerman.spec
chmod 644 $RPM_BUILD_ROOT/%{_sysconfdir}/powerman/*
%dir %{_sysconfdir}/powerman
%config(noreplace) %{_sysconfdir}/powerman/*
- changed etc to sysconfdir macro.
[ben@flix result]$

Will you please reevaluate the CURRENT version of powerman's SRPM.

http://dl.sourceforge.net/sourceforge/powerman/powerman.spec
http://dl.sourceforge.net/sourceforge/powerman/powerman-1.0.23-4.src.rpm
Comment 8 Jarod Wilson 2006-06-14 01:03:46 EDT
Retaking ownership of this review, since the prior taking is lost w/the bugzilla
db crash.
Comment 9 Jarod Wilson 2006-06-14 01:16:53 EDT
Okay, as I think I mentioned in the comment that got lost with the bugzilla db
crash, I started work on a powerman pacakge of my own before thinking to see if
one was already pending review. The results of merging your spec and my spec
(which includes some stuff from Linux Networx, my former employer) looks pretty
good to me. First up, the issues I see with your spec:

1) New version out now (not your fault its taken so long for someone to review
though)

2) Release: tag is missing %{?dist}

3) Better to generally use %{name} and pretty much always %{version} tags
throughout a spec

4) parallel makes seem to fail intermittently on smp systems w/smp_mflags defined

5) No default powerman.conf installed, so when the user creates one, it won't be
owned by the powerman package

6) The initscript sets powermand to run by default, Fedora policy is to leave
everything off, let the user turn it on

7) Similar, on upgrades, let the user bounce the daemon unless there is a
condrestart option in the initscript

8) Looks like there's more %doc material that isn't getting installed

Hey, that's kinda a long list... But I'll attach my spec diff, and you can find
my spec (and srpm), which I believe addresses all of the above issues, here:

http://wilsonet.com/packages/powerman/

(It also adds a config file for the Linux Networx Icebox v4).
Comment 10 Jarod Wilson 2006-06-14 01:19:41 EDT
Created attachment 130799 [details]
Diff from Ben's latest spec to Jarod's spec

Some of the changes are merely cosmetic, but I believe it does address all
current concerns I've got. Please feel free to argue against any of my changes
though. :)
Comment 11 Ben Woodard 2006-06-15 19:49:27 EDT
Very cool. I'm happy to have you take this over. (The same for conman.) FYI Jim
just released a new version of powerman, you might want to pick that up before
you roll a release.
Comment 12 Jarod Wilson 2006-06-15 23:37:13 EDT
Its yours to run with if you want, but I'd be happy to take it over. First
though, on with the review...

Review details:
* package meets naming and packaging guidelines
* specfile is properly named, is cleanly written and uses macros consistently
* dist tag is present
* build root is correct
* license field matches the actual license
* license is open source-compatible.  License text included in package
* source files match upstream:
   a903511e470cb3be005075ebc739048e  powerman-1.0.24.tar.bz2
* latest version is being packaged
* BuildRequires are proper
* package builds in mock (x86_64, development)
* rpmlint has no complaints
* final provides and requires are sane:
config(powerman) = 1.0.24-1
powerman = 1.0.24-1
--
/bin/sh
config(powerman) = 1.0.24-1
libc.so.6()(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.3)(64bit)
libc.so.6(GLIBC_2.3.4)(64bit)
libnsl.so.1()(64bit)
libutil.so.1()(64bit)
libutil.so.1(GLIBC_2.2.5)(64bit)
libwrap.so.0()(64bit)
--
* no shared libraries
* package is not relocatable
* owns the directories it creates
* doesn't own any directories it shouldn't
* no duplicates in %files
* file permissions are appropriate
* %clean is present
* %check is not present; no applicable test suite upstream
* scriptlets present (chkconfig/service); all OK
* code, not content
* documentation is small, so no -docs subpackage is necessary
* %docs are not necessary for the proper functioning of the package
* no -devel subpackage to worry about
* no pkgconfig files
* no libtool .la files
* not a GUI app
* not a web app

Looks good to me, package approved.
Comment 13 Chris Ricker 2006-06-19 11:25:50 EDT
I'm a little confused - Jarod, you reviewed this, approved it, then imported it
yourself with yourself as the new package owner? Where's the independent review?
Comment 14 Jarod Wilson 2006-06-19 11:32:07 EDT
Yeah, okay, admittedly a little shady looking there... But I was originally
picking up the review of Ben's package, which has now been looked over by three
different people. Ben expressed interest in handing it off to me, so I went
ahead and imported it in the interest of getting it into the repo.
Comment 15 Jason Tibbitts 2006-06-19 17:07:27 EDT
Just to avoid the appearance of impropriety, I gave this a look.  Since the
above review resembles the template I use, it's pretty easy.  Everything checks
out, except that I cannot seem to fetch the upstream source from the location
given in the Source0: url.  The file listing at the sourceforge site doesn't
show any tarballs, just RPMs.  I downloaded the src.rpm and unpacked it; the
tarball does indeed match the checksum.

You should probably bug upstream to provide a tarball that's downloadable.

At this point, though, I'll APPROVE +1.
Comment 16 Ben Woodard 2006-06-19 17:53:16 EDT
I tossed the tar.bz2 up there. That was an oversight. Sorry.

Jim Garlick is the upstream for this. However, I'm also considered a project
admin and so you can bounce things through me if needed.
Comment 17 Jarod Wilson 2006-06-19 20:34:34 EDT
Ben, can we by chance also get the icebox4.dev file thrown into the next upstream release?

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