[Bug 202901] Review Request: pgFouine - PostgreSQL log analyzer

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: pgFouine -  PostgreSQL log analyzer


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=202901


toshio at tiki-lounge.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|nobody at fedoraproject.org    |toshio at tiki-lounge.com
OtherBugsDependingO|163776                      |163778
              nThis|                            |




------- Additional Comments From toshio at tiki-lounge.com  2006-08-18 01:48 EST -------
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.

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.