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: libpst - utilities to convert Outlook .pst files to other formats https://bugzilla.redhat.com/show_bug.cgi?id=434727 tcallawa@xxxxxxxxxx changed: What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review? ------- Additional Comments From tcallawa@xxxxxxxxxx 2008-02-26 17:04 EST ------- OK, there are a few things that you need to change in this package: * Please don't override localstatedir. This is set properly in the Fedora config. * In the Summary, please describe the package without using its name. Tell me what it does, not what it is called. :) Also, please be sure to capitalize the first word (and don't end it with a period). * Release should not be wholly macro driven, this is something that you want to increment by hand. I strongly suggest that you consider using the Dist Tag system for this, see: http://fedoraproject.org/wiki/Packaging/DistTag * We have a specific, explicit system for identifying the License tag. Your package should have "License: GPLv2+", see: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines * Please use the %{name} and %{version} macros everywhere that you have "libpst" and "0.6.7" hardcoded (except in Name: and Version:, of course). This ensures a much easier update path, with less work on your part. For example, use them in the Source: field, in your mkdir and mv commands, and in the %files entries. * Please use one of the approved BuildRoot settings, which are listed here: http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473 * Do not set Vendor or Packager in the spec file. The Fedora buildsystem will set these values automatically. * Do not use AutoReqProv. This is one of the big benefits of rpm! We want those provides! * In your %description, please don't have any line longer than 80 characters. This ensures that it displays cleanly on a terminal screen. You can have multiple lines. :) * Please use %setup -q instead of just %setup. This quiets the output from the tarball unpacking, which helps us save on logfile size in the buildsystem. * In %build, please replace your long configure invocation with %configure, which is a macro that evaluates to the same thing (and more). * In %build, please use make %{_?smp_mfiles} instead of just make. See: http://fedoraproject.org/wiki/Packaging/Guidelines#head-525c7d76890cb22df33b759c65c35c82bf434d2e * In %install, there is no reason to check the buildroot before deleting it. Since you've set it in the spec file, it will always be safe to simply delete it as the first step in the %install process. * In %install, replace your long make install invocation with simply: make DESTDIR=$RPM_BUILD_ROOT install It will achieve the same result, but will avoid embedding the buildroot into the installed files (and is also much smaller). * In %install, you do not need to make the docdir and manually copy the files into it. rpm will do this for you, for any files marked in %files as doc. To do this, just get rid of the mkdir and mv commands from %install, and delete these lines from %files: %doc %{_mandir}/* %docdir %{_datadir}/doc/libpst-0.6.7 %{_datadir}/doc/libpst-0.6.7 Then, add this line to %files: %doc AUTHORS COPYING ChangeLog NEWS README That's it! Same end result, so much less pain. :) * You do not need to have empty %pre, %post, %preun, and %postun entries. Just take them out entirely. * In your %clean entry, please add: rm -rf $RPM_BUILD_ROOT * In %files, please use %defattr(-,root,root,-) instead of %defattr(-,root,root) * In %files, please don't use %doc %{_mandir}/*. There are two problems with this line: 1. %{_mandir} is automagically marked as %doc when it is used in %files, you do not need to specify this. 2. %{_mandir}/* will cause this package to own all of the created directories under %{_mandir}, which in the case of your package are "man1/" and "man5/". You only want to own the manpages installed in those directories, so replace the line with: %{_mandir}/man1/* %{_mandir}/man5/* * In the %changelog, please ensure the following: Everytime that you make any change, no matter how small or trivial, add a changelog entry, in a supported format. The supported formats are described here: http://fedoraproject.org/wiki/Packaging/Guidelines#head-b7d622f4bb245300199c6a33128acce5fb453213 * The naming of this package "libpst" is a little strange, given that it doesn't actually have any libraries in it. A better name might be "pst-utils". Normally, I wouldn't say anything, but since I know that you are the upstream, I figured I would throw it out there. If you like the "libpst" name, that's fine, you can keep using it here. I highly recommend that you read through: http://fedoraproject.org/wiki/Packaging/Guidelines and http://fedoraproject.org/wiki/Packaging/NamingGuidelines. Please fix the spec file to reflect these changes, and I will be happy to do a review, and sponsor you upon success. :) -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug, or are watching someone who is. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review