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.
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
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.
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
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
Can someone take a look at this and either approve it or tell me why it shouldn't be approved.
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,-)
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
Retaking ownership of this review, since the prior taking is lost w/the bugzilla db crash.
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).
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. :)
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.
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.
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?
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.
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.
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.
Ben, can we by chance also get the icebox4.dev file thrown into the next upstream release?