Bug 437397

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>

Include - version-release at the end (sans dist-tag)

(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.

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

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>        20071116

This should have been:
* Thu Mar 13 2008 Dave Jones <davej> - 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.

../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.

hmm. Passing -Iinclude in the CFLAGS still ends up with it not finding those
include files.  Puzzling.

ok, fixed.  now builds too.

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

Please use a cvs template here so we know what branches you want, etc...
see: 
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

New Package CVS Request
=======================
Package Name: pmtools
Short Description: Collection of tools for processing ACPI tables
Owners: davej
Branches: 
InitialCC: 
Cvsextras Commits: yes

cvs done.

had a request to build this for F-8 too.  Can you create a CVS branch please ? kthx.

cvs done.