Bug 612023 - Review Request: tinycdb - Utility and library for manipulating constant databases
Review Request: tinycdb - Utility and library for manipulating constant datab...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
13
All Linux
low Severity medium
: ---
: ---
Assigned To: Martin Gieseking
Fedora Extras Quality Assurance
:
: 573448 (view as bug list)
Depends On:
Blocks: 613881
  Show dependency treegraph
 
Reported: 2010-07-07 01:04 EDT by Chen Lei
Modified: 2010-07-13 21:31 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-07-13 21:31:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
martin.gieseking: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Chen Lei 2010-07-07 01:04:58 EDT
SPEC: http://dl.dropbox.com/u/1338197/1/tinycdb.spec
SRPM: http://dl.dropbox.com/u/1338197/1/tinycdb-0.77-1.fc13.src.rpm

Description:

tinycdb is a small, fast and reliable utility and subroutine library for 
creating and reading constant databases. The database structure is tuned
for fast reading.

This is a requirement for building qt-mobility.
Comment 1 Martin Gieseking 2010-07-10 14:49:46 EDT
There's already a review request for tinycdb, but Adrien still needs a sponsor:
https://bugzilla.redhat.com/show_bug.cgi?id=573448
Comment 2 Chen Lei 2010-07-10 15:33:12 EDT
(In reply to comment #1)
> There's already a review request for tinycdb, but Adrien still needs a sponsor:
> https://bugzilla.redhat.com/show_bug.cgi?id=573448    
It seems Adrien don't reply for your last comment for several months:) 

Actually, We need this package be reviewed soon, this is a blocker for packaging qt-mobility which is a core component in meego 1.1.
Comment 3 Martin Gieseking 2010-07-10 15:47:27 EDT
OK, I suggest to contact Adrien directly and ask him whether he's still interested in maintaining this package. If not, we could close it as duplicate and review yours. Otherwise, a sponsor should be convinced to take the review in a timely manner. :)
Comment 4 Chen Lei 2010-07-11 23:45:44 EDT
*** Bug 573448 has been marked as a duplicate of this bug. ***
Comment 5 Martin Gieseking 2010-07-12 11:08:12 EDT
Your package looks almost fine to me. Just a couple of minor remarks:

- Add file debian/copyright as %doc. Even if "public domain" isn't a real license, the corresponding copyright information should be added to the binary package.

- The tarball contains a pkgconfig file in folder debian/. It's probably a good idea to add it to the package. It must be slightly adapted, though.

- I suggest to replace the second sentence of the devel %description with:
This package contains library and header files for developing applications that use %{name}.
Comment 6 Chen Lei 2010-07-12 23:18:25 EDT
(In reply to comment #5)
> Your package looks almost fine to me. Just a couple of minor remarks:
> 
> - Add file debian/copyright as %doc. Even if "public domain" isn't a real
> license, the corresponding copyright information should be added to the binary
> package.
> 
> - The tarball contains a pkgconfig file in folder debian/. It's probably a good
> idea to add it to the package. It must be slightly adapted, though.
> 
> - I suggest to replace the second sentence of the devel %description with:
> This package contains library and header files for developing applications that
> use %{name}.    

All fixed except license file which is not needed for this package.

From licensing guideline:

If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc. If the source package does not include the text of the license(s), the packager should contact upstream and encourage them to correct this mistake. 

SRPM: http://dl.dropbox.com/u/1338197/1/tinycdb-0.77-2.fc13.src.rpm
Comment 7 Martin Gieseking 2010-07-13 03:08:56 EDT
(In reply to comment #6)
> If (and only if) the source package includes the text of the license(s) in its
> own file, then that file, containing the text of the license(s) for the package
> must be included in %doc. 

Yes, I know. But since "public domain" isn't a license, there's no license text that could be included. Thus, this part of the guidelines doesn't fit here. File debian/copyright is just a further %doc file like README or NEWS containing some information the user might be interested in. However, I leave it up to you to add it or not.

I'll do the formal review later today because I'm pretty busy at the moment.
Comment 8 Martin Gieseking 2010-07-13 12:02:28 EDT
The package looks OK now. Since %makeinstall usually should not be used, please add a short comment telling that it's valid here and works without unwanted side-effects.
I also recommend to be more specific in the %files section to avoid packaging of unwanted files in future versions, e.g.
%{_includedir}/*.h instead of %{_includedir}/*

$ rpmlint /var/lib/mock/fedora-13-x86_64/result/*.rpm
tinycdb.src: W: no-buildroot-tag
4 packages and 0 specfiles checked; 0 errors, 1 warnings.

If you don't plan to build this package for EPEL, the warning can be ignored.
Otherwise, add a buildroot tag and clean the buildroot in %install. Additionally, Requires: pkgconfig would be necessary.

---------------------------------
Key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: %makeinstall must not be used when make install DESTDIR=%{buildroot} works.
    - make install DESTDIR=%{buildroot} doesn't work
    - %makeinstall has no unwanted side-effects here, and simplifies the installation process

[+] MUST: The package must be licensed with a Fedora approved license.
[+] MUST: The License field in the package spec file must match the actual license.
[.] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum tinycdb_0.77.tar.gz*
    c00e5fb96c30356ac3b67b2ab5d5641b  tinycdb_0.77.tar.gz
    c00e5fb96c30356ac3b67b2ab5d5641b  tinycdb_0.77.tar.gz.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
    koji scratch builds:
    F-12: http://koji.fedoraproject.org/koji/taskinfo?taskID=2316672
    F-13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2316685

[.] MUST: If the package does not successfully compile, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[.] MUST: The spec file MUST handle locales properly.
[+] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[+] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
    - static library removed

[+] MUST: Library files with a suffix (e.g. libfoo.so.1.1) must go in a -devel package.
[+] MUST: devel packages must require the base package using a fully versioned dependency
[+] MUST: Packages must NOT contain any .la libtool archives.
[.] MUST: Packages containing GUI applications must include a %{name}.desktop file
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

[.] SHOULD: If the source package does not include license text(s) ...
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described. 
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[+] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg. 
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin ...
[+] SHOULD: your package should contain man pages for binaries/scripts.

----------------
Package APPROVED
----------------
Comment 9 Chen Lei 2010-07-13 12:15:47 EDT
(In reply to comment #8)
> The package looks OK now. Since %makeinstall usually should not be used, please
> add a short comment telling that it's valid here and works without unwanted
> side-effects.
> I also recommend to be more specific in the %files section to avoid packaging
> of unwanted files in future versions, e.g.
> %{_includedir}/*.h instead of %{_includedir}/*
> 
Actually, tinycdb only has one head file - cdb.h, will be fixed in cvs.

> $ rpmlint /var/lib/mock/fedora-13-x86_64/result/*.rpm
> tinycdb.src: W: no-buildroot-tag
> 4 packages and 0 specfiles checked; 0 errors, 1 warnings.
> 
> If you don't plan to build this package for EPEL, the warning can be ignored.
> Otherwise, add a buildroot tag and clean the buildroot in %install.
> Additionally, Requires: pkgconfig would be necessary.
> 

Requires: pkgconfig will be added by rpmbuild automatically now, thanks for the review.
Comment 10 Chen Lei 2010-07-13 12:17:40 EDT
New Package CVS Request
=======================
Package Name: tinycdb
Short Description: Utility and library for manipulating constant databases
Owners: supercyper
Branches: F-13
InitialCC:
Comment 11 Martin Gieseking 2010-07-13 12:21:54 EDT
(In reply to comment #9)
> Requires: pkgconfig will be added by rpmbuild automatically now
Yes, but not in EPEL 5. The remark about pkgconfig was related to the previous comment about EPEL.

> thanks for the review.
You're welcome.
Comment 12 Kevin Fenzi 2010-07-13 19:21:01 EDT
CVS done (by process-cvs-requests.py).

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