Bug 833853 - (libkolab) Review Request: libkolab - Kolab Format Handler library
Review Request: libkolab - Kolab Format Handler library
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Extras Quality Assurance
:
Depends On: libkolabxml
Blocks: kde-reviews 835904
  Show dependency treegraph
 
Reported: 2012-06-20 09:19 EDT by Jeroen van Meeuwen
Modified: 2012-07-27 11:14 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-27 11:14:17 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rdieter: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jeroen van Meeuwen 2012-06-20 09:19:23 EDT
Spec URL: http://git.kolabsys.com/rpm/libkolab/plain/libkolab.spec
SRPM URL: http://mirror.kolabsys.com/pub/fedora/kolab-3.0/f17/development/SRPMS/libkolab-0.3-3.fc17.kolab_3.0.src.rpm

Description: libkolab is a Kolab Format handler library needed for routine operations against groupware data stored in Kolab XML format.

Fedora Account System Username: kanarip

The latest SRPM can routinely be found linked from this page:

http://mirror.kolabsys.com/pub/fedora/kolab-3.0/f17/development/SRPMS/repoview/libkolab.html
Comment 1 Jeroen van Meeuwen 2012-06-20 09:20:07 EDT
Add dependency on libkolabxml
Comment 2 Rex Dieter 2012-06-26 10:46:17 EDT
newer kdepim-runtime's need this, I can help cwickert with the review.
Comment 3 Rex Dieter 2012-06-26 13:25:54 EDT
Naming: ok

1. MUST sources: Not ok, not verifiable.  no upstream 0.3 tarball (yet?)
6d9a81c318193c2d3ec580b73ffa8886  libkolab-0.3.tar.gz
looks like this may be a snapshot, so providing a recipe to create the tarball would be enough.

SHOULD: I'd suggest using
URL:  http://git.kolab.org/libkolab
instead of http://kolab.org/

SHOULD: remove rpm cruft like Group:, BuildRoot, %defattr

SHOULD: remove extraneous cmake qt-devel deps, like
BuildRequires:  cmake >= 2.6
BuildRequires:  qt-devel >= 4.7
(and similar install-time deps in the -devel subpkg), these are pulled in implicitly via kdepimlibs-devel already

SHOULD: track lib soname's explicitly, use
%{_libdir}/libkolab.so.0
instead of overly broad
%{_libdir}/libkolab.so.*

SHOULD: these cmake directives are extraneous and already included in fedora's default %{cmake} macro, so can be removed:
    -DCMAKE_SKIP_RPATH=ON \
    -DCMAKE_INSTALL_PREFIX=%{_prefix} \
    -DLIB_INSTALL_DIR=%{_libdir} \

2. MUST: scriptlets NOT ok, missing:
%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig

Licensing: OK
confirmed a mixture of LGPLv2+ and LGPLv3+, though no LICENSE or COPYING file is provided (another SHOULD nice-to-have fix)

3. -devel pkg MUST have and arch'd dep on the main pkg, using
Requires: %{name}%{?_isa} = %{version}-%{release}



Address at least MUST items 1-3, and looks like we have a winner.
Comment 4 Jeroen van Meeuwen 2012-06-27 04:27:41 EDT
(In reply to comment #3)
> Naming: ok
> 
> 1. MUST sources: Not ok, not verifiable.  no upstream 0.3 tarball (yet?)
> 6d9a81c318193c2d3ec580b73ffa8886  libkolab-0.3.tar.gz
> looks like this may be a snapshot, so providing a recipe to create the
> tarball would be enough.
> 

Changed this to:

# From http://git.kolab.org/libkolab/snapshot/54d11d067951ddedd8b28aa7eb54e4f68bc81218.tar.gz
Source0:        http://git.kolab.org/%{name}/snapshot/%{name}-%{version}.tar.gz

for the time being, while this still comes straight from the master branch.

> SHOULD: I'd suggest using
> URL:  http://git.kolab.org/libkolab
> instead of http://kolab.org/
> 

I'll enlist some help to create some sort of http://kolab.org/software/libkolab page, but for now I've changed the URL to the GIT repository browse location as suggested.

> SHOULD: remove rpm cruft like Group:, BuildRoot, %defattr
> 

Done.

> SHOULD: remove extraneous cmake qt-devel deps, like
> BuildRequires:  cmake >= 2.6
> BuildRequires:  qt-devel >= 4.7
> (and similar install-time deps in the -devel subpkg), these are pulled in
> implicitly via kdepimlibs-devel already
> 

Done.

> SHOULD: track lib soname's explicitly, use
> %{_libdir}/libkolab.so.0
> instead of overly broad
> %{_libdir}/libkolab.so.*
> 

Done.

> SHOULD: these cmake directives are extraneous and already included in
> fedora's default %{cmake} macro, so can be removed:
>     -DCMAKE_SKIP_RPATH=ON \
>     -DCMAKE_INSTALL_PREFIX=%{_prefix} \
>     -DLIB_INSTALL_DIR=%{_libdir} \
> 

Removed.

> 2. MUST: scriptlets NOT ok, missing:
> %post -p /sbin/ldconfig
> %postun -p /sbin/ldconfig
> 

Included.

> Licensing: OK
> confirmed a mixture of LGPLv2+ and LGPLv3+, though no LICENSE or COPYING
> file is provided (another SHOULD nice-to-have fix)
> 

Inclusion issue logged as https://bugzilla.kolabsys.com/show_bug.cgi?id=859

> 3. -devel pkg MUST have and arch'd dep on the main pkg, using
> Requires: %{name}%{?_isa} = %{version}-%{release}
> 

Done.

> Address at least MUST items 1-3, and looks like we have a winner.

Thanks for the review Rex.

New SPEC: http://git.kolabsys.com/rpm/libkolab/plain/libkolab.spec
New SRPM: http://mirror.kolabsys.com/pub/fedora/kolab-3.0/f17/development/SRPMS/libkolab-0.3-5.fc17.kolab_3.0.src.rpm
Comment 5 Rex Dieter 2012-06-27 09:08:28 EDT
Nice, so one last thing I forgot to mention, if you *are* going forward with using snapshots, please do follow:
http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages

I'll leave that to you to address prior to building any official packages.


APPROVED
Comment 6 Kevin Kofler 2012-06-27 19:08:38 EDT
To be clear, 5.fc17.kolab_3.0 is not a valid Release tag for a prerelease snapshot for several reasons:
* The tag before the disttag must be of the form 0.n where n is an integer, e.g. "0.5" rather than just "5".
* "kolab_3.0" is not a valid snapshot tag, should be of the form "20120627git" or "20120627git54d11d06".
Comment 7 Rex Dieter 2012-06-27 21:59:30 EDT
No worries, the .spec only says 5%{?dist}, so that's just part of kolab's custom dist tag
Comment 8 Jeroen van Meeuwen 2012-06-28 08:01:32 EDT
(In reply to comment #6)
> To be clear, 5.fc17.kolab_3.0 is not a valid Release tag for a prerelease
> snapshot for several reasons:
> * The tag before the disttag must be of the form 0.n where n is an integer,
> e.g. "0.5" rather than just "5".

Agreed, this was my mistake in updating from a released 0.2 series version to the git master snapshot, and now there's little opportunity to roll back without bumping the epoch.

> * "kolab_3.0" is not a valid snapshot tag, should be of the form
> "20120627git" or "20120627git54d11d06".

It is, as Rex already mentioned, our buildsystem dist-tag that does this.
Comment 9 Jeroen van Meeuwen 2012-06-28 08:02:17 EDT
New Package SCM Request
=======================
Package Name: libkolab
Short Description: Kolab Format Handler library
Owners: kanarip
Branches: el6 f17
InitialCC:
Comment 10 Gwyn Ciesla 2012-06-28 08:12:22 EDT
Git done (by process-git-requests).
Comment 11 Rex Dieter 2012-07-27 11:14:17 EDT
imported, built awhile back, closing.

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