Per Toshio's review; here is the new spec and srpm:
Spec URL: http://developer.postgresql.org/~devrim/rpms/other/pgfouine/pgfouine.spec

SRPM URL:
http://developer.postgresql.org/~devrim/rpms/other/pgfouine/pgfouine-0.7-3.src.rpm

MD5Sums
249021f97fc8f90836205db8d5c2cd5a  pgfouine-0.7-3.src.rpm
c6b09d495fd11e0b8e9b4b86e4252449  pgfouine-0.7-include_path.patch
536a23564b21eb28d98485a3746b90a5  pgfouine-0.7.tar.gz
13783dd119055e07a1f4bb5822b5e768  pgfouine.spec

Blockers:
* Source does not match upstream tarball.  It looks like upstream has taken
  some changes from you and you have repackaged the 0.7 release with these
  changes included.  This is not okay.  Instead you can either:
  1) Use the vanilla 0.7 tarball with any necessary patches applied afterwards.
  2) Use a snapshot from upstream's SCM and in a comment show how to recreate
     the snapshot package.
  Since a diff of the files doesn't show any changes that will affect building
  or what is installed on the system, I would suggest using upstream's 0.7
  tarball in this case.
* Macros are used everywhere except in the patch file.  So if %{_datadir} is
  ever redefined, the include_path will still be set to /usr/share/....
  I would suggest using a patch file that does something like:
    + ini_set('include_path', '@INCLUDEPATH@');
    +
  And then in the spec using:
    sed -i 's!@INCLUDEPATH@!%{_datadir}!' pgfouine_vacuum.php
    sed -i 's!@INCLUDEPATH@!%{_datadir}!' pgfouine.php

  If the upstream package had a build script it would do something like that
  to allow the paths to be redefined.
* INSTALL is not needed in the package as it doesn't give any information that
  would be relevant to someone who installed via the rpm.  ChangeLog could go
  in but that depends on how useful you think it will be to users of the
  package.
* Why are the tests installed into %{_datadir}/pgfouine?  Are they necessary
  for the package to run?  If not, they should be installed to %doc (if they
  are useful for the user to know how to run the programs) or left out.

Cosmetic:
* There's no need to check that the buildroot is not "/" before rm'ing it
  because you are already setting the buildroot in the spec file.  So:
    [ "%{buildroot}" != "/" ] && rm -rf %{buildroot}
  can be reduced to:
    rm -rf %{buildroot}
* I favor more verbose Changelog entries.  Since the previous reviewing
  occurred on IRC rather than bugzilla, it would be especially nice
  (When in bugzilla, you can reference the bugzilla number; when on IRC, things
  can get lost.)

Good and Already Fixed:
* Name conforms to the naming guidelines.
* Spec file name matches the package name.
* Package is licensed under the GPL and this is reflected in the license tag.
* License is included as the COPYING file.
* Spec file is written in English and is legible.
* Builds to noarch on x86_64.
* No language files, no need for %find_lang.
* No shared libraries.
* Not relocatable.
* Owns all directories that are created.
* No duplicate files listed in %files.
* permissions are set correctly on files.
* %clean section that removes the buildroot is present.
* Code not content.
* Nothing in %doc affects the program's operation.
* Not a library package so no headers, static or dynamic libs, pkgconfig files,
  or .la files.
* Not a GUI application so no .desktop needed.
* Does not own files or directories owned by another package.
* No scriptlets included or needed.
* No subpackages
* Removed AutoReq: no and Requires: php.  This change let rpm's dependency
  resolver pick up the dependence on /usr/bin/php on its own.
* Changed patch to remove the "." path from being included in the scripts.
  This was a security hole. (Invoke the script from a world writable directory
  and someone could inject a trojan php file into the script.)
* Packager, Vendor, Copyright, and PreReq tags are not used as per
  Packaging/Guidelines.
* BuildRoot in prefered form.
* Builds in mock.
* No necessary or optional buildrequires were left out.
* rpmlint returns no issues for the package.

Based on the comments above, here are the new files:
Spec URL: http://developer.postgresql.org/~devrim/rpms/other/pgfouine/pgfouine.spec

SRPM URL:
http://developer.postgresql.org/~devrim/rpms/other/pgfouine/pgfouine-0.7-4.src.rpm

Here is the details of what we fixed and how we fixed it in the spec/src.rpm
Devrim just posted (0.7-4):

> * Source does not match upstream tarball.

It was an error of mine when I sent the first tarball to Devrim. It's now fixed
- the src.rpm contains a vanilla 0.7 release.

> * Macros are used everywhere except in the patch file.

We use a sed command in the %prep as you suggest it.

> * INSTALL is not needed in the package as it doesn't give any information that
>   would be relevant to someone who installed via the rpm.  ChangeLog could go
>   in but that depends on how useful you think it will be to users of the
>   package.

INSTALL removed, ChangeLog added - it can be useful to check if a problem has
been solved in the parser.

> * Why are the tests installed into %{_datadir}/pgfouine?  Are they necessary
>   for the package to run?  If not, they should be installed to %doc (if they
>   are useful for the user to know how to run the programs) or left out.

They are useless for the user. We removed them.

> Cosmetic:
> * There's no need to check that the buildroot is not "/" before rm'ing it
>   because you are already setting the buildroot in the spec file.  So:
>     [ "%{buildroot}" != "/" ] && rm -rf %{buildroot}
>   can be reduced to:
>     rm -rf %{buildroot}

Fixed.

> * I favor more verbose Changelog entries.  Since the previous reviewing
>   occurred on IRC rather than bugzilla, it would be especially nice
>   (When in bugzilla, you can reference the bugzilla number; when on IRC, things
>   can get lost.)

OK. We will take that into account.

Regards,

--
Guillaume

MD5Sums
09cccc6978d9e953fb9a12487d75455d  pgfouine-0.7-4.src.rpm
ad2b56340581758fbda051abdc340d71  pgfouine.spec
c6b09d495fd11e0b8e9b4b86e4252449  pgfouine-0.7-include_path.patch
4ad02f17d9da3789e548bac77fd2f2a5  pgfouine-0.7.tar.gz

Blockers:
* Macros are used everywhere except in the patch file.

  As long as the patch defines "/usr/share/..." explicitly, the sed line in
  the spec file won't accomplish anything.  If the patch instead defines it as
  @INCLUDEPATH@ then the sed substitution will change @INCLUDEPATH@ into
  whatever the datadir is.  Eventually, the upstream build script can do the
  substitution itself based on what the value of an ENVIRNMENT VARIABLE or
  define passed to make.

Fixed:
* Source matches upstream tarball now.  One thing to remember is that you
  should be slightly paranoid as a packager.  The software that you package is
  going to be installed on a lot of end-user machines.  If someone says they
  are upstream and sends you a tarball you should still check it against the
  tarball on the upstream site, compare to upstream gpg signatures or MD5Sums,
  check against tarballs in packages from other distributions, or etc.  You
  only have a reviewer checking MD5Sums while the package is being submitted.
  Once it is in the repository it is up to you to ensure that the package
  continues to contain the source from upstream.
* INSTALL has been removed and ChangeLog added.
* Tests have been removed from the binary package.
* buildroot check was removed.

Toshio,

> * Macros are used everywhere except in the patch file.

It was fixed in upstream. I think the problem occured when Devrim rebuilt the
package (it was OK in the src.rpm I have here).
The current patch in CVS contains @INCLUDEPATH@ so that the sed replacement works.

He's on vacation at the moment. I'll ask him to build a new RPM as soon as he's
back home.

Thanks for the review.

Hello,

Here are the new spec the file and srpm:
http://developer.postgresql.org/~devrim/rpms/other/pgfouine/pgfouine.spec
http://developer.postgresql.org/~devrim/rpms/other/pgfouine/pgfouine-0.7.1-1.src.rpm
 

Anything left for approval?

Regards, Devrim

79df9b088a9cd0a7741de0b5857dc3e7  pgfouine-0.7.1.tar.gz
2dd8a37d014895ef2832b1cfef9916eb  pgfouine.spec
6bbb6b68ecae95675af8725e2a400681  pgfouine-0.7-include_path.patch
d2aba441696023bf0b6c65f8bda329a9  ../pgfouine-0.7.1-1.src.rpm

All blockers have been resolved.

The only thing I still find is that there is no document that tells how to set
things up.  Maybe including this file into documentation would be good:

http://pgfouine.projects.postgresql.org/tutorial.html

Hi,

(In reply to comment #8)
> All blockers have been resolved.

Good :)
 
> The only thing I still find is that there is no document that tells how to set
> things up.  Maybe including this file into documentation would be good:
> 
> http://pgfouine.projects.postgresql.org/tutorial.html

Ok, I added a text version of that document. The URLs for the new SRPM and SPEC
file  will follow.

New spec:
http://developer.postgresql.org/~devrim/rpms/other/pgfouine/pgfouine.spec

New srpm:
http://developer.postgresql.org/~devrim/rpms/other/pgfouine/pgfouine-0.7.1-2.src.rpm

a2afd563b26c8271dc6e60290f35f3b7  pgfouine-0.7.1-2.src.rpm
6bbb6b68ecae95675af8725e2a400681  pgfouine-0.7.1-2/pgfouine-0.7-include_path.patch
79df9b088a9cd0a7741de0b5857dc3e7  pgfouine-0.7.1-2/pgfouine-0.7.1.tar.gz
def5ab84558b24388322e21bd0abdc30  pgfouine-0.7.1-2/pgfouine-tutorial.txt
1b6867a7588b678bcae31258051d9f19  pgfouine-0.7.1-2/pgfouine.spec

pgfouine.php was able to parse a small log file and generate reasonable output.

No blockers.

APPROVED

Package Change Request
======================
Package Name: pgfouine
New Branches: EL-4 EL-5

done