Bug 202901
Summary: | Review Request: pgFouine - PostgreSQL log analyzer | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Devrim GUNDUZ <devrim> |
Component: | Package Review | Assignee: | Toshio Kuratomi <toshio> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | Flags: | petersen:
fedora-cvs+
|
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-11-30 23:54:02 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Devrim GUNDUZ
2006-08-16 23:29:31 UTC
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 |