[Bug 434727] Review Request: libpst - utilities to convert Outlook .pst files to other formats

Tue Feb 26 22:04:13 UTC 2008

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 at redhat.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-review?

------- Additional Comments From tcallawa at redhat.com  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.