Bug 251020 - Review Request: libflaim - Flaim Database Engine
Summary: Review Request: libflaim - Flaim Database Engine
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Till Maas
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-08-06 15:27 UTC by Christopher Brown
Modified: 2007-11-30 22:12 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-09-09 11:00:25 UTC
Type: ---
Embargoed:
opensource: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Christopher Brown 2007-08-06 15:27:09 UTC
Spec URL: http://snecker.fedorapeople.org/libflaim/libflaim.spec
SRPM URL: http://snecker.fedorapeople.org/libflaim/libflaim-4.9.1046-9.1.fc7.src.rpm
Description: FLAIM is an embeddable cross-platform database engine that provides a rich, powerful, easy-to-use feature set. It is the database engine used by Novell eDirectory. It has proven to be highly scalable, reliable, and robust. It is available on a wide variety of 32 bit and 64 bit platforms.

Notes: This is the first of a number of packages in order to provide iFolder[1] for Fedora. I am already in ACL and familiar with Koji, plague etc. Builds cleanly in mock.

[1] http://www.ifolder.com/index.php/Main_Page

Comment 1 Till Maas 2007-08-30 13:50:08 UTC
- static libraries should be in a -static package or not packaged at all:
%{_libdir}/libflaim.a - see:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7

- - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
(for directory ownership and usability).
-> Add Reuires: pkgconfig to %package devel

- you should use  $RPM_OPT_FLAGS instead of %optflags:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-f3d77b27a5d29dfc1f5600ef3fc836f2e317badf

Comment 2 Christopher Brown 2007-08-30 14:51:07 UTC
(In reply to comment #1)

Thanks for the review Till.

> - static libraries should be in a -static package or not packaged at all:
> %{_libdir}/libflaim.a - see:
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7

Done.

> - - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
> (for directory ownership and usability).
> -> Add Reuires: pkgconfig to %package devel

Done.

> - you should use  $RPM_OPT_FLAGS instead of %optflags:
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-f3d77b27a5d29dfc1f5600ef3fc836f2e317badf

Done.

Updated pkgs at:

http://snecker.fedorapeople.org/libflaim/

I am still working on iFolder and simias which are the other two packages that
form the reqs necessary to get iFolder working on Fedora.

Regards
Chris

Comment 3 Till Maas 2007-09-05 20:49:26 UTC
The release should be an integer, imho it is in the Naming Guidelines
(http://fedoraproject.org/wiki/Packaging/NamingGuidelines)

So change
Release:	9.2%{?dist}
to
Release:	10%{?dist}

You can later use
Release:	10%{?dist}.1
If you need to increment only in one Fedora Collection that is not Rawhide.

Source0 is not valid (anymore):
$ curl -I
http://forgeftp.novell.com/flaim/development/flaim/downloads/source/libflaim-4.9.1046.tar.gz
HTTP/1.1 404 Not Found

According to the URL in the spec file the latest tarball is: libflaim-4.9.989.tar.gz

Is this older version intentional?

The buildroot is not ok, see
http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473

You should use %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) or
some other example from the wiki.

Please use rm -rf %{buildroot} instead of rm -Rf %{buildroot}

Did you build your packages with mock? When I build it and run rpmlint on the
rpms, I get:
$ rpmlint libflaim-*
W: libflaim unstripped-binary-or-object /usr/lib/libflaim.so.5.2
E: libflaim-debuginfo empty-debuginfo-package
W: libflaim-devel no-documentation

The third warning can be ignored. The build.log shows:
ldconfig /var/tmp/libflaim-4.9.1046-build/usr/lib
Installation complete.
+ /usr/lib/rpm/find-debuginfo.sh /builddir/build/BUILD/libflaim-4.9.1046
0 blocks
find: /var/tmp/libflaim-4.9.1046-build/usr/lib/debug: No such file or directory

But I do not know, why.

Comment 4 Till Maas 2007-09-05 20:51:45 UTC
I just realized that the broken debuginfo affects your rpm packages, too.

Comment 5 Christopher Brown 2007-09-06 14:55:48 UTC
Hi Till,

Thanks for assigning yourself to this.

(In reply to comment #3)
> The release should be an integer, imho it is in the Naming Guidelines
> (http://fedoraproject.org/wiki/Packaging/NamingGuidelines)
> 
> So change
> Release:	9.2%{?dist}
> to
> Release:	10%{?dist}
> 
> You can later use
> Release:	10%{?dist}.1
> If you need to increment only in one Fedora Collection that is not Rawhide.
> 
> Source0 is not valid (anymore):
> $ curl -I
>
http://forgeftp.novell.com/flaim/development/flaim/downloads/source/libflaim-4.9.1046.tar.gz
> HTTP/1.1 404 Not Found
> 
> According to the URL in the spec file the latest tarball is:
libflaim-4.9.989.tar.gz
> 
> Is this older version intentional?

The version being built is newer, essentially a subversion checkout therefore I
have made the necessary changes to reflect this.

> The buildroot is not ok, see
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473
> 
> You should use %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) or
> some other example from the wiki.

Okay, done.

> Please use rm -rf %{buildroot} instead of rm -Rf %{buildroot}

Okay, done.

> Did you build your packages with mock? When I build it and run rpmlint on the
> rpms, I get:
> $ rpmlint libflaim-*
> W: libflaim unstripped-binary-or-object /usr/lib/libflaim.so.5.2
> E: libflaim-debuginfo empty-debuginfo-package
> W: libflaim-devel no-documentation
> 
> The third warning can be ignored. The build.log shows:
> ldconfig /var/tmp/libflaim-4.9.1046-build/usr/lib
> Installation complete.
> + /usr/lib/rpm/find-debuginfo.sh /builddir/build/BUILD/libflaim-4.9.1046
> 0 blocks
> find: /var/tmp/libflaim-4.9.1046-build/usr/lib/debug: No such file or directory
> 
> But I do not know, why.

This was because the shared object was not installed executable - I have
attached a small patch to resolve this and should hopefully get this upstream
quickly.

Updated RPMS as usual at:

http://snecker.fedorapeople.org/libflaim/

Regards
Chris

Comment 6 Till Maas 2007-09-06 15:31:43 UTC
(In reply to comment #5)

http://forgeftp.novell.com/flaim/development/flaim/downloads/source/libflaim-4.9.1046.tar.gz
> > HTTP/1.1 404 Not Found
> > 
> > According to the URL in the spec file the latest tarball is:
> libflaim-4.9.989.tar.gz
> > 
> > Is this older version intentional?
> 
> The version being built is newer, essentially a subversion checkout therefore I
> have made the necessary changes to reflect this.

This still needs some work. I do not understand where the 1046 comes from in the
Version tag. Is this the svn revision?
Did you built the svn snapshot yourself? In this case, you need to add
instructions to the spec how to rebuild the snapshot, see:
http://fedoraproject.org/wiki/Packaging/SourceURL#head-615f6271efb394ab340a93a6cf030f2d08cf0d49

The URL in Source0 does not work here:
$ curl --insecure -I 
https://forgesvn1.novell.com/svn/flaim/libflaim-4.9.1046.tar.gz
HTTP/1.1 404 Not Found
[...]



Comment 7 Christopher Brown 2007-09-06 20:30:26 UTC
(In reply to comment #6)

> This still needs some work. I do not understand where the 1046 comes from in the
> Version tag. Is this the svn revision?

It was, kinda, sorta. I had used the some RPMS from a SUSE developers repo as a
starting point and that tarball was included as source. I can't figure out how
it was generated as pulling from svn also brings down a number of other header
files and such which cause the build to fail. Therefore I've downgraded the spec
a little to an official release which builds fine using the patch. The patch was
accepted upstream today btw.

I have uploaded new builds. Rpmlint is quiet on these now.

Cheers
Chris

Comment 8 Till Maas 2007-09-06 21:22:05 UTC
- rpmlint: ok
W: libflaim-devel no-documentation

- naming: ok
- versioning: ok
- license: BAD/TODO
  License Tag ist LGPLv2 but license is GPLv2
  license text (GPLv2) is included and mentioned in source files
- rpm legible: ok
- source: ok
url points to correct file:
cbd0caf6239cffb7640391eda7551d4a  libflaim-4.9.989.tar.gz
cbd0caf6239cffb7640391eda7551d4a  libflaim-4.9.989.tar.gz.1
- builds in mock for F7, i386
- ldconfig run: ok
- directory ownage: ok
- buildroot: ok
- %files: no dupes and correct defattr: ok
- no -static needed: ok
- no libtool files: ok
- .so file without suffix in devel: ok
- %install and %clean with rm: ok
- -devel requires of mainpackage: ok

- todo before import:
* add a libflaim prefix to the patch, i.e. rename it to:
libflaim-permissions.patch

* fix license tag

Comment 9 Christopher Brown 2007-09-07 08:56:32 UTC
(In reply to comment #8)

> - todo before import:
> * add a libflaim prefix to the patch, i.e. rename it to:
> libflaim-permissions.patch

Okay, done.

> * fix license tag

Okay, done.

Cheers
Chris

Comment 10 Till Maas 2007-09-07 09:10:13 UTC
Package is APPROVED, you need to follow:
http://fedoraproject.org/wiki/PackageMaintainers/NewPackageProcess
(step 8)

Comment 11 Christopher Brown 2007-09-07 09:25:20 UTC
New Package CVS Request
=======================
Package Name: libflaim
Short Description: Flaim Database Engine
Owners: snecker
Branches: F-7
InitialCC: 
Cvsextras Commits: yes

Comment 12 Kevin Fenzi 2007-09-07 18:33:32 UTC
cvs done.

Comment 13 Mamoru TASAKA 2007-09-08 16:26:02 UTC
libflaim.pc contains the line:
----------------------------------------------------
Libs: -lpthread -lrt -lstdc++ -ldl -lncurses -lflaim -L${libdir}
----------------------------------------------------
"-lncurses" means that libflaim-devel should have "Requires: ncurses-devel".

But make check if "-lncurses" is really needed. For libflaim.so
the linkage against libncurses.so MUST be done in advance and
"-lncurses" is usually not needed.

Comment 14 Christopher Brown 2007-09-09 11:00:25 UTC
(In reply to comment #13)
> libflaim.pc contains the line:
> ----------------------------------------------------
> Libs: -lpthread -lrt -lstdc++ -ldl -lncurses -lflaim -L${libdir}
> ----------------------------------------------------
> "-lncurses" means that libflaim-devel should have "Requires: ncurses-devel".
> 
> But make check if "-lncurses" is really needed. For libflaim.so
> the linkage against libncurses.so MUST be done in advance and
> "-lncurses" is usually not needed.

Thanks for the additional info Mamoru. I'll add this on the next build. Buildsys
seems to be having problems at the moment. :(

Closing now as per package process - thanks everyone.

Regards
Chris


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