Bug 434727 - Review Request: libpst - utilities to convert Outlook .pst files to other formats
Review Request: libpst - utilities to convert Outlook .pst files to other for...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-24 20:59 EST by Carl Byington
Modified: 2009-05-22 12:54 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-07-02 17:54:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tcallawa: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Carl Byington 2008-02-24 20:59:19 EST
Spec URL: <http://www.five-ten-sg.com/libpst/packages/libpst.spec>
SRPM URL: <http://www.five-ten-sg.com/libpst/packages/libpst-0.6.7-1.src.rpm>
Description: The Libpst utilities include readpst which can convert email messages to both mbox and MH mailbox formats, and pst2ldif which can convert the
contacts to .ldif format for import into ldap databases.

This is my first Fedora package, and I am seeking a sponsor.
Comment 1 Tom "spot" Callaway 2008-02-26 17:04:12 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. :)
Comment 2 Carl Byington 2008-02-26 19:56:43 EST
Thanks! 

What is the policy on the change log in the .spec file versus the gnu
ChangeLog/NEWS files? Are we supposed to duplicate every entry from ChangeLog
into the .spec file, or is the changelog in the spec file only for packaging
related changes?

Yes, the name is strange, but that is the original name of the project, and I
did not want to arbitrarily change it. At some point we may be able to
standardize the interface enough to create a libpst.so used by the actual
utilities, and also usable by other projects. That will further add to the
naming confusion.
Comment 3 Tom "spot" Callaway 2008-02-28 16:38:11 EST
The changelog in the .spec file is for changes to the spec file itself, not for
copying the packaged source "ChangeLog". :)
Comment 4 Carl Byington 2008-03-05 22:25:54 EST
Delay caused by work on 0.6.8 with another conversion utility. This now requires
ImageMagick and gd.

The libpst.spec <http://www.five-ten-sg.com/libpst/packages/libpst.spec> is
built via ./configure (which we need to run to get 'make distcheck' anyway) from
libpst.spec.in <http://www.five-ten-sg.com/libpst/packages/libpst.spec.in>. 

The packaging guidelines say "Every time you make changes, that is, whenever you
increment the E-V-R of a package, add a changelog entry.". The version is
contained in the autoconf configure.in, so when we bump that and rebuild, we
automatically get a new libpst.spec with a new version number. But since we
don't generally update the libpst.spec.in, that results in no change log entry
in libpst.spec. Do you want us to manually add changelog entries in
libpst.spec.in for every version bump in configure.in, even though the packaging
is not really changing? I can see advantages to both yes and no answers.

%files
%doc AUTHORS COPYING ChangeLog NEWS README

That caused problems, since the %doc seems to remove the documentation
directory, which has already been populated by autoconf with html help files. My
solution is to have autoconf also handle those GNU standard files, and then just
reference 

%files
%docdir %{_datadir}/doc/%{name}-%{version}
%{_datadir}/doc/%{name}-%{version}

with the documentation content fully populated by autoconf make install.
Comment 5 Tom "spot" Callaway 2008-03-24 14:16:30 EDT
Sorry for the delay in responding. I'm not an advocate of .spec.in files for the
reasons that you describe. In Fedora, your spec file is maintained independently
of your source tree, so you do need to add changelog entries in that file.

As to the %docdir, as long as those files are in that directory, it is fine.
Comment 6 Carl Byington 2008-03-31 11:46:15 EDT
Ok, I will change the workflow scripts here to ensure that the changelog entries
in .spec.in (and therefore in .spec) are consistent with the ones in the gnu
NEWS and ChangeLog files.
Comment 7 Carl Byington 2008-05-16 14:43:03 EDT
Now that F9 is out, what is the next step here? Updated tarballs and spec file
at <http://www.five-ten-sg.com/libpst/packages/>.
Comment 8 Tom "spot" Callaway 2008-05-28 11:43:07 EDT
Good:

- rpmlint checks return:
libpst.src: W: strange-permission libpst.spec 0600
libpst.x86_64: W: incoherent-version-in-changelog 0.6.9 0.6.9-1.fc10

I'm ok with ignoring the permission on the spec file, but you should really fix
the changelog versioning. Just remember to put the version-release at the end
(right now, you're just putting version).

- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2+) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream (6321e94601697a20b9850a3c4eec718b77869ec8)
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 

This is approved. Thanks for your patience! :)

You should be able to pick up the process around here:
https://fedoraproject.org/wiki/PackageMaintainers/Join#Watch_for_Feedback

I will sponsor you.
Comment 9 Carl Byington 2008-05-29 12:13:51 EDT
Since release is 1.fc10, what do you want for changelog entries that are
distribution independent? Or do we need to maintain separate .spec files for
fc10 and rhel5 (for example).

Looking at the above URL, does that procedure require a fedora client system? I
only have RHEL/Centos systems here.
Comment 10 Tom "spot" Callaway 2008-05-29 12:43:42 EDT
Shouldn't require a fedora client system.

As to the release in the changelog, just use "1" without the disttag. This will
be correct for all distributions.
Comment 11 Tom "spot" Callaway 2008-06-03 14:48:36 EDT
Just checking in, I haven't see you apply for cvsextras in the Fedora Account
System...
Comment 12 Carl Byington 2008-06-05 09:35:48 EDT
Sorry, I got way to busy this week. This is on my list for this weekend.
Comment 13 Carl Byington 2008-06-08 18:17:54 EDT
New Package CVS Request
=======================
Package Name: libpst
Short Description: Utilities to convert Outlook .pst files to other formats
Owners: carllibpst
Branches: F-10
InitialCC:
Cvsextras Commits: yes
Comment 14 Kevin Fenzi 2008-06-08 23:50:15 EDT
cvs done.
Comment 15 Carl Byington 2008-06-19 17:03:13 EDT
./common/cvs-import.sh /tmp/libpst-0.6.14-1.src.rpm
cd devel; make build

That worked, but trying to import into the F-10 branch does not:

./common/cvs-import.sh -b F-10 /tmp/libpst-0.6.14-1.src.rpm
Checking out module: 'libpst'
ERROR: "libpst/F-10" does not exist!

Is that supposed to work, or is there some other requirement?
Comment 16 Tom "spot" Callaway 2008-06-19 17:13:44 EDT
There is no F-10 branch because there is no F-10 release. devel is a "magic"
branch because it is always there, but right now, it is what will become F-10.

F-8 and F-9 are the other active branches right now.
Comment 17 Tom "spot" Callaway 2008-07-02 17:54:03 EDT
Built in rawhide, closing this review ticket out.

Note You need to log in before you can comment on or make changes to this bug.