Bug 230071

Summary: Review Request: libkexiv2 - A library to manipulate EXIF/IPTC information
Product: [Fedora] Fedora Reporter: Rex Dieter <rdieter>
Component: Package ReviewAssignee: manuel wolfshant <manuel.wolfshant>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: mgarski
Target Milestone: ---Flags: manuel.wolfshant: fedora-review+
dennis: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-03-09 13:06:48 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: 231473    

Description Rex Dieter 2007-02-26 14:25:43 UTC
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/libkexiv2.spec
SRPM URL: http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.stable/libkexiv2-0.1.1-1.src.rpm

Description: A library to manipulate EXIF/IPTC information

Needed for newer digikam versions.

Comment 1 manuel wolfshant 2007-02-27 10:24:34 UTC
The Description is a bit terse. May I suggest "A wrapper for Exiv2 library to
manipulate picture metadata, used by kipi-plugins, digiKam and other kipi host
applications." ?

Comment 2 Rex Dieter 2007-02-27 12:02:23 UTC
It's just what is in the README. (:  Besides, only digikam uses it, for now, but
I'd rather not mention client apps, since that could likely change.

Comment 3 manuel wolfshant 2007-02-27 18:23:22 UTC
I know. But their web site / sf repository is a bit more informative. If not for
the presence of "exif" in the description, I would have had absolutely no idea
what's the purpose of the library, especially since "exiv" rang no bells. In my
opinion, the presence of "picture metadata" and/or "digikam" would be better hints.
My suggestion was based on info retrieved from upstream's web site.

Comment 4 Rex Dieter 2007-02-27 19:38:20 UTC
OK, I'm convinced, and will include it in the next pkg update (after someone
comits to doing a pkg review).

Comment 5 manuel wolfshant 2007-02-27 20:37:09 UTC
Well, just to push you a bit
1. There is a typo error in %clean:
rm -rf $FPM_BUILD_ROOT

2. mock build fails with:

checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking for kde-config... not found
configure: error: The important program kde-config was not found!
Please check whether you installed KDE correctly.

error: Bad exit status from /var/tmp/rpm-tmp.91758 (%build)

=> kdelibs is missing as BR.
Adding that, build fails again, this time with:
checking for vsnprintf... yes
checking for snprintf... yes
checking for X... configure: error: Can't find X includes. Please check your
installation and add the correct path
s!
error: Bad exit status from /var/tmp/rpm-tmp.6018 (%build)
=> kdelibs-devel is missing as BR

So, please add kdelibs-devel as BR, correct the FPM into RPM and I'll try to do
a review. It would the first one for a package which relies on pkgconfig, so
please be kind to me :)

Comment 6 Rex Dieter 2007-02-27 20:41:30 UTC
crud, I coulda sworn I did a mock build prior to package submission... oh well. (:

Comment 7 Rex Dieter 2007-02-27 20:53:47 UTC
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/libkexiv2.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.stable/libkexiv2-0.1.1-2.src.rpm

%changelog
* Tue Feb 27 2007 Rex Dieter <rdieter[AT]fedoraproject.org> 0.1.1-2
- fix %%clean
- update %%description
- BR: kdelibs-devel


Comment 8 manuel wolfshant 2007-03-01 01:42:00 UTC
First of all, it would have been nice if the URL of spec from comment #7 would
have been the modified one.
Second, I think that the Description tag in -devel should match the one from the
main package. You have modified only the main one...

As a comment prior to the review: examining the build log and then the sources,
I have noticed that %configure looks for doxygen and some doxygen related
scripts seem to exist in the admin directory. Unfortunately my experience with
doxygen is limited to installing it via yum. Just adding doxygen as BR makes
configure pick it, but nothing seems to be changed afterwards: nothing is built
in the admin directory andthe build log has some failures which _seem_ to look
like missing QT requirements. Explicitely requiring qt-devel does not help
either and unfortunately the machine where I have done all the tests just
suffered yet another kernel crash. OTOH the admin folder does not seem to
contain anything else but a bunch of scripts so I guess there is no need to
struggle with it (could be a leftover from a more comprehensive package from
which this tar has been separated). If I am wrong here, I would appreciate if
someone would correct me.

Good:

- rpmlint checks do not return anything on the source and binary rpm. the -devel
package gives:
W: libkexiv2-devel no-documentation
which I guess that can be ignored. The doc is in the main package.
- package meets naming guidelines
- package meets packaging guidelines
- license (GPL) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream, is the latest available version, sha1sum
 7218bb8b81955fc4ef376f302bda9b94bd2b68bd  libkexiv2-0.1.1.tar.bz2
- package compiles on devel (x86_64 and i386)
- no missing BR
- no unnecessary BR
- no locales 
- not relocatable
- owns all files/directories that it creates, does not take ownership of other
files/directories
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- not a GUI so no need for .desktop file 
- devel package ok
- no .la files (correctly removed in %install)
- post/postun ldconfig ok
- devel correctly requires pkgconfig
- separation of libs between main and devel packages respects versioning (once
again, I kindly ask someone more experienced to verify and correct me if I am wrong)


SHOULD
- successfully builds in mock for fc6 and devel, i386 and x86_64
warning:  I cannot verify if the libs are actually working, nothing to test them
with
- no scriptlets except usage of ldconfig which is OK
- pkgconfig(.pc) files are in -devel
- devel requires the base package using a fully versioned dependency.

What I would like to see
- including of AUTHORS, ChangeLog and maybe even the RELEASE.rev files as %doc
- the same Description in -devel as in main
Both are minor and non blockers, they can be fixed before uploading to cvs.

From my point of view the package is OK (minus the minor caveats I have
mentioned ). However I kindly ask someone more experienced to have a second
look, therefore I will leave this bug in REVIEW state for another day or two.


Comment 9 manuel wolfshant 2007-03-01 01:45:01 UTC
I take that back, description in %devel is just fine. I was looking in a wrong
place. Sorry :(


Comment 10 manuel wolfshant 2007-03-06 09:28:34 UTC
No comments in one week, switching to APPROVED.

Comment 11 Rex Dieter 2007-03-06 18:23:06 UTC
New Package CVS Request
=======================
Package Name: libkexiv2
Short Description: A library to manipulate EXIF/IPTC information
Owners: rdieter.edu
Branches: FC-5 FC-6 devel 
InitialCC: mgarski

Comment 12 Dennis Gilmore 2007-03-09 02:32:28 UTC
Branched

Comment 13 Rex Dieter 2007-03-09 13:06:48 UTC
Imported, and building, thanks.