Bug 437397 - Review Request: pmtools - power management debugging tools
Review Request: pmtools - power management debugging tools
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-13 16:46 EDT by Dave Jones
Modified: 2015-01-04 17:30 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-24 10:33:06 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
lkundrak: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dave Jones 2008-03-13 16:46:14 EDT
Spec URL: http://davej.fedorapeople.org/pmtools.spec
SRPM URL: http://davej.fedorapeople.org/pmtools-20071116-1.src.rpm
Description: power management debugging tools from http://www.lesswatts.org/projects/acpi/utilities.php
Comment 1 Lubomir Kundrak 2008-03-14 23:12:45 EDT
1.) Source0:        %{name}-%{version}.tar.gz

Please replace with full URL

2.) Release:        1

Include the %{dist} tag

3.) Buildroot:      %{_tmppath}/%{name}-%{version}-root

Please use the standard BuildRoot:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473

4.) ExclusiveArch:  i386 x86_64 ia64

Consider using %{ix86} macro instead of i386

5.) %setup -q
..
%build
cd %{name}-%{version}
...
%install
...
cd %{name}-%{version}

Those cds are not nice. Take a look at %setup -n
http://www.rpm.org/max-rpm/s1-rpm-inside-macros.html

6.) Replace /usr/sbin with %{_sbindir}

install acpidump/acpidump %{buildroot}/usr/sbin/acpidump
install acpixtract/acpixtract %{buildroot}/usr/sbin/acpixtract
install madt/madt %{buildroot}/usr/sbin/madt
...
%files
%defattr(-,root,root)
/usr/sbin/acpidump
/usr/sbin/acpixtract
/usr/sbin/madt
%doc README COPYING

7.) * Thu Mar 13 2008 Dave Jones <davej@redhat.com>

Include - version-release at the end (sans dist-tag)
Comment 2 Lubomir Kundrak 2008-03-14 23:18:56 EDT
(In reply to comment #1)
> 5.) %setup -q
> ..
> %build
> cd %{name}-%{version}
> ...
> %install
> ...
> cd %{name}-%{version}
> 
> Those cds are not nice. Take a look at %setup -n
> http://www.rpm.org/max-rpm/s1-rpm-inside-macros.html

Actually this is completely not needed. RPM cds there without explicit =n and
your cds break it.

8.) Either use $RPM_BUILD_ROOT or %{buildroot}, but do not mix those

9.) You don't pass %{mflags} to make; if parallel make breaks the build at least
add a comment about that

Review will continue once the package is buildable.
Comment 3 Dave Jones 2008-03-17 15:59:18 EDT
Thanks for the review.  (These comments are really helpful btw, I'm also fixing
up some of my older packages to take your comments into account).

I updated the spec at the url above. srpm url changed due to the %dist
modification.  It's now pmtools-20071116-1.fc9.src.rpm
Comment 4 Lubomir Kundrak 2008-03-18 11:01:50 EDT
1.) %setup -n %{name}-%{version}

%{name}-%{version} is the default value. "%setup -q" would be sufficient (-q is
always mandatory).

2.) * Thu Mar 13 2008 Dave Jones <davej@redhat.com>        20071116

This should have been:
* Thu Mar 13 2008 Dave Jones <davej@redhat.com> - 20071116-1

3.) The package depends on headers from kernel source; Currently they are looked
for in ../include; this is from acpidump Makefile:

KERNEL_INCLUDE := ../include
CFLAGS += -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Os -s
-D_LINUX -DDEFINE_ALTERNATE_TYPES -I$(KERNEL_INCLUDE)

Maybe one solution to this is to depend on kernel-devel and try to guess path to
the includes with something like CPPFLAGS=-I$(ls -d /usr/src/kernels/*/include
|tail -n1). Pay extra attention for the tool not to be dependent on a specific
kernel. It's likely you better know what to do there anyways :)

Another problem in above two lines is that "-Os -s" are not allowed. Patch them
away; and probably take a look at another Makefiles for similar problems.
Comment 5 Dave Jones 2008-03-18 17:24:59 EDT
../include is in the top level of the source tarball. It's a userspace-ified
version set of the kernel headers.

spec and srpm updated for other points.
Comment 6 Dave Jones 2008-03-19 01:57:00 EDT
hmm. Passing -Iinclude in the CFLAGS still ends up with it not finding those
include files.  Puzzling.
Comment 7 Dave Jones 2008-03-19 02:13:07 EDT
ok, fixed.  now builds too.
Comment 8 Lubomir Kundrak 2008-03-19 07:37:02 EDT
When you'll be improving, you might want to get rid of the rpmlint warnings:

$ rpmlint
/home/lkundrak/rpmbuild/RPMS/x86_64/pmtools-debuginfo-20071116-1.fc8.x86_64.rpm
pmtools-debuginfo.x86_64: W: spurious-executable-perm
/usr/src/debug/pmtools-20071116/acpidump/acpidump.c
$ rpmlint /home/lkundrak/rpmbuild/RPMS/x86_64/pmtools-20071116-1.fc8.x86_64.rpm
pmtools.x86_64: W: summary-ended-with-dot Collection of tools for processing
ACPI tables.
$ rpmlint pmtools-20071116-1.fc9.src.rpm
pmtools.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 11)
pmtools.src: W: summary-ended-with-dot Collection of tools for processing ACPI
tables.
$

However they're not serious enough to block import.

APPROVED
Comment 9 Kevin Fenzi 2008-03-19 22:49:37 EDT
Please use a cvs template here so we know what branches you want, etc...
see: 
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure
Comment 10 Dave Jones 2008-03-20 18:04:41 EDT
New Package CVS Request
=======================
Package Name: pmtools
Short Description: Collection of tools for processing ACPI tables
Owners: davej
Branches: 
InitialCC: 
Cvsextras Commits: yes
Comment 11 Kevin Fenzi 2008-03-20 19:00:34 EDT
cvs done.
Comment 12 Dave Jones 2008-03-29 17:21:29 EDT
had a request to build this for F-8 too.  Can you create a CVS branch please ? kthx.
Comment 13 Kevin Fenzi 2008-03-31 12:41:49 EDT
cvs done.

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