Bug 698067 - Review Request: hiredis - A C client library for redis
Summary: Review Request: hiredis - A C client library for redis
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Volker Fröhlich
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-04-20 04:58 UTC by Shakthi Kannan
Modified: 2014-01-28 13:02 UTC (History)
4 users (show)

Fixed In Version: hiredis-0.10.0-3.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-05-25 03:04:59 UTC
volker27: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Shakthi Kannan 2011-04-20 04:58:01 UTC
Spec URL: http://shakthimaan.fedorapeople.org/SPECS/hiredis.spec
SRPM URL: http://shakthimaan.fedorapeople.org/SRPMS/hiredis-0.9.2-1.fc14.src.rpm
Description: A minimalistic C client library for redis

Comment 1 Shakthi Kannan 2011-04-20 05:02:13 UTC
$  rpmlint hiredis.spec
hiredis.spec: W: invalid-url Source0: antirez-hiredis-v0.9.2-0-g0fbfa45.zip
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

$  rpmlint ../SRPMS/hiredis-0.9.2-1.fc14.src.rpm 
hiredis.src: W: spelling-error Summary(en_US) minimalistic -> minimalist, Minimalist, minimalism
hiredis.src: W: spelling-error %description -l en_US minimalistic -> minimalist, Minimalist, minimalism
hiredis.src: W: invalid-url Source0: antirez-hiredis-v0.9.2-0-g0fbfa45.zip
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

$  rpmlint ../RPMS/i686/hiredis-0.9.2-1.fc14.i686.rpm 
hiredis.i686: W: spelling-error Summary(en_US) minimalistic -> minimalist, Minimalist, minimalism
hiredis.i686: W: spelling-error %description -l en_US minimalistic -> minimalist, Minimalist, minimalism
hiredis.i686: W: shared-lib-calls-exit /usr/lib/libhiredis.so.1.0 exit@GLIBC_2.0
hiredis.i686: W: no-manual-page-for-binary hiredis-example
hiredis.i686: W: no-manual-page-for-binary hiredis-test
1 packages and 0 specfiles checked; 0 errors, 5 warnings.

$  rpmlint ../RPMS/i686/hiredis-devel-0.9.2-1.fc14.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Will inform upstream to remove exit() calls from code. Successful Koji builds for F14, F15 and EL6:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3012740
http://koji.fedoraproject.org/koji/taskinfo?taskID=3012741
http://koji.fedoraproject.org/koji/taskinfo?taskID=3012744

Comment 2 Volker Fröhlich 2011-05-07 14:08:31 UTC
Some comments:

There is a version 0.10.0 release upstream. Please update!

You can drop your second patch and instead write:

make install PREFIX=%{buildroot}/%{_prefix} INSTALL_LIB=%{buildroot}/%{_libdir}

That's also replacing /usr with %{_prefix}. Please also place comments on your patches do in the spec file. You can use the name macro on some occasions, e. g. when installing.

I think you can make up a better description, given the description on the homepage. The description of the devel package even seems wrong to me, because the devel package certainly does not contain libraries to use a database.

redis should be a BuildRequires -- not a Requires.

I don't know how you managed the EPEL 6 build. Currently there is only a redis in testing, as far as I can see.

The compiler flags are not as requires by Fedora. See http://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags

Please install the TODO file as documentation.

Comment 3 Shakthi Kannan 2011-05-10 07:11:36 UTC
Thanks for your comments. Updated:

SPEC: http://shakthimaan.fedorapeople.org/SPECS/hiredis.spec
SRPM: http://shakthimaan.fedorapeople.org/SRPMS/hiredis-0.10.0-1.gitdf203bc328.fc14.src.rpm

* The 0.10.0 fails to build, so have updated to hiredis-0.10.0-1.gitdf203bc328.

* Updated use of:

  make install PREFIX=%{buildroot}/%{_prefix} INSTALL_LIB=%{buildroot}/%{_libdir}

For 64-bit, we need to override LIBRARY_PATH.

* redis is not required to build hiredis, but, only to use it. Hence, have used Requires.

* Added TODO file as documentation.

$  rpmlint hiredis.spec
hiredis.spec: W: invalid-url Source0: hiredis.gitdf203bc328.tar.bz2
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

$  rpmlint hiredis-0.10.0-1.gitdf203bc328.fc14.src.rpm 
hiredis.src: W: spelling-error Summary(en_US) minimalistic -> minimalist, Minimalist, minimalism
hiredis.src: W: spelling-error %description -l en_US minimalistic -> minimalist, Minimalist, minimalism
hiredis.src: W: invalid-url Source0: hiredis.gitdf203bc328.tar.bz2
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

$  rpmlint hiredis-0.10.0-1.gitdf203bc328.fc14.i686.rpm 
hiredis.i686: W: spelling-error Summary(en_US) minimalistic -> minimalist, Minimalist, minimalism
hiredis.i686: W: spelling-error %description -l en_US minimalistic -> minimalist, Minimalist, minimalism
hiredis.i686: W: no-manual-page-for-binary hiredis-example
hiredis.i686: W: no-manual-page-for-binary hiredis-test
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

$  rpmlint hiredis-devel-0.10.0-1.gitdf203bc328.fc14.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Successful Koji builds for F-14, F-15, and EL-6:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3062235
http://koji.fedoraproject.org/koji/taskinfo?taskID=3062238
http://koji.fedoraproject.org/koji/taskinfo?taskID=3062241

Comment 4 Volker Fröhlich 2011-05-10 21:13:29 UTC
Actually the upstream 0.10.0 builds just fine. Please use the .tar.gz from upstream, if it is smaller in size.

Please see the spec below and apply the changes to your file. The changes basically are a corrected devel sub-package description, use of optimization flags and a simpler install path part.

http://www.geofrogger.net/review/hiredis.spec

Comment 5 Shakthi Kannan 2011-05-11 02:41:17 UTC
Thanks for the changes. Updated:

SPEC: http://shakthimaan.fedorapeople.org/SPECS/hiredis.spec
SRPM: http://shakthimaan.fedorapeople.org/SRPMS/hiredis-0.10.0-2.fc14.src.rpm

$  rpmlint hiredis.spec
hiredis.spec: W: invalid-url Source0: antirez-hiredis-v0.10.0-3-gdf203bc.tar.gz
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

$  rpmlint hiredis-0.10.0-2.fc14.src.rpm 
hiredis.src: W: spelling-error Summary(en_US) minimalistic -> minimalist, Minimalist, minimalism
hiredis.src: W: spelling-error %description -l en_US minimalistic -> minimalist, Minimalist, minimalism
hiredis.src: W: invalid-url URL: https://github.com/antirez/hiredis <urlopen error [Errno 111] Connection refused>
hiredis.src: W: invalid-url Source0: antirez-hiredis-v0.10.0-3-gdf203bc.tar.gz
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

$  rpmlint hiredis-0.10.0-2.fc14.i686.rpm 
hiredis.i686: W: spelling-error Summary(en_US) minimalistic -> minimalist, Minimalist, minimalism
hiredis.i686: W: spelling-error %description -l en_US minimalistic -> minimalist, Minimalist, minimalism
hiredis.i686: W: invalid-url URL: https://github.com/antirez/hiredis <urlopen error [Errno 111] Connection refused>
hiredis.i686: W: no-manual-page-for-binary hiredis-example
hiredis.i686: W: no-manual-page-for-binary hiredis-test
1 packages and 0 specfiles checked; 0 errors, 5 warnings.

$  rpmlint hiredis-devel-0.10.0-2.fc14.i686.rpm 
hiredis-devel.i686: W: invalid-url URL: https://github.com/antirez/hiredis <urlopen error [Errno 111] Connection refused>
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Successful Koji builds for F-14, F-15, EL-6:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3064370
http://koji.fedoraproject.org/koji/taskinfo?taskID=3064385
http://koji.fedoraproject.org/koji/taskinfo?taskID=3064388

Comment 6 Volker Fröhlich 2011-05-14 19:38:20 UTC
It's common to leave out the full stop in the changelog, as it is more like a list of things.

By the way: INSTALL_LIB seems to be useless. Must be some leftover.

The summary says, the package was a "client". I think this should be "client library".

I encourage you to create a sub-package for the test and example file, to keep the library package small, or delete it at all, if it is not useful.

FYI: http://groups.google.com/group/linux.debian.devel.mentors/browse_thread/thread/b2f55966e1c58e69/c1f15d9e4289fe45?lnk=raot&fwc=1
----------------------------------------

Review:

[+] Good
[-] Needs work
[0] Does not apply

MUST:
=====

[+] rpmlint:
[makerpm@fedora14 adapters]$ rpmlint ~/rpmbuild/SRPMS/hiredis-0.10.0-2.fc14.src.rpm ~/rpmbuild/RPMS/x86_64/hiredis-*0.10.0-2.fc14.x86_64.rpm 
hiredis.src: W: spelling-error Summary(en_US) minimalistic -> minimalist, Minimalist, minimalism
hiredis.src: W: spelling-error %description -l en_US minimalistic -> minimalist, Minimalist, minimalism
hiredis.src: W: invalid-url Source0: antirez-hiredis-v0.10.0-3-gdf203bc.tar.gz
hiredis.x86_64: W: spelling-error Summary(en_US) minimalistic -> minimalist, Minimalist, minimalism
hiredis.x86_64: W: spelling-error %description -l en_US minimalistic -> minimalist, Minimalist, minimalism
hiredis.x86_64: W: no-manual-page-for-binary hiredis-example
hiredis.x86_64: W: no-manual-page-for-binary hiredis-test
4 packages and 0 specfiles checked; 0 errors, 7 warnings.

[+] Naming according to the Package Naming Guidelines
[+] Spec file matches base package name
[+] Packaging guidelines met
[+] License approved for Fedora
[+] License field in spec matches code
[+] License file included, if source package includes it
[+] Spec in American English
[+] Spec is legible
[+] Sources match upstream md5sum: b32b930e5e1ee007594c1056c3ff1c0e
[+] Compiles and builds into binary RPMs on at least one primary architecture
[0] ExcludeArch is specified and commented
[0] Locales are handled correctly
[+] All build dependencies listed
[+] Calls ldconfig for its shared libraries
[+] No bundled system libraries
[0] Stated as relocatable package
[+] Owns all its directories or requires package that does
[+] No file listing duplicates
[+] File permissions correct
[+] Consistent use of macros
[+] Code or permissible content
[0] Large documentation in -doc subpackage
[+] No runtime dependency of files listed as %doc
[+] Header files in -devel subpackage
[0] Static files in -static subpackage
[+] Library files without suffix in -devel subpackage
[+] Devel-package requires base package
[0] No .la libtool archives
[0] GUI application includes properly installed %{name}.desktop file
[+] No files or directories owned, that other packages own
[+] Filenames in packages are UTF-8

SHOULD:
=======

[0] Query upstream if no license text is included
[+] Package builds in mock: Tried epel-6-x86_64 and fedora-rawhide-i386
[?] Package works as described -- Haven't tried
[0] Scriptlets are sane, if used
[0] Subpackages other than -devel should require base package (versioned)
[0] pkgconfig files in -devel subpackage
[0] Dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider
requiring the package which provides the file instead of the file itself
[0] Contain man pages, where they make sense

--------
APPROVED
--------

Comment 7 Shakthi Kannan 2011-05-16 05:28:31 UTC
Thanks again for the review and comments. Updated to use 'client library' in Summary and removed INSTALL_LIB.

SPEC: http://shakthimaan.fedorapeople.org/SPECS/hiredis.spec
SRPM: http://shakthimaan.fedorapeople.org/SRPMS/hiredis-0.10.0-3.fc14.src.rpm

Comment 8 Shakthi Kannan 2011-05-16 05:30:55 UTC
New Package SCM Request
=======================
Package Name: hiredis
Short Description: A minimalistic C client library for Redis
Owners: shakthimaan
Branches: F-14 F-15 EL-6
InitialCC: shakthimaan

Comment 9 Jason Tibbitts 2011-05-18 22:19:52 UTC
Git done (by process-git-requests).

Comment 10 Fedora Update System 2011-05-19 05:01:15 UTC
hiredis-0.10.0-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/hiredis-0.10.0-3.fc14

Comment 11 Fedora Update System 2011-05-19 05:01:28 UTC
hiredis-0.10.0-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/hiredis-0.10.0-3.el6

Comment 12 Fedora Update System 2011-05-19 05:01:36 UTC
hiredis-0.10.0-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/hiredis-0.10.0-3.fc15

Comment 13 Fedora Update System 2011-05-19 21:59:17 UTC
hiredis-0.10.0-3.fc14 has been pushed to the Fedora 14 testing repository.

Comment 14 Fedora Update System 2011-05-25 03:04:54 UTC
hiredis-0.10.0-3.fc15 has been pushed to the Fedora 15 stable repository.

Comment 15 Fedora Update System 2011-05-27 20:20:27 UTC
hiredis-0.10.0-3.fc14 has been pushed to the Fedora 14 stable repository.

Comment 16 Fedora Update System 2011-06-06 21:33:13 UTC
hiredis-0.10.0-3.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 17 Christopher Meng 2014-01-28 02:55:38 UTC
Package Change Request
======================
Package Name: hiredis
New Branches: epel7
Owners: cicku shakthimaan

Comment 18 Gwyn Ciesla 2014-01-28 13:02:02 UTC
Git done (by process-git-requests).


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