Bug 619237

Spec:
http://github.com/silas/rpms/raw/master/redis/redis.spec

Woops, broken link above.

Simon,

I'm not an official package maintainer so this is an informal review.

Key:
- = N/A
x = Check
! = Problem
? = Not evaluated

MUST
----

[X] rpmlint must be run on every package

$ rpmlint redis-1.2.6-1.fc14.src.rpm
redis.src: W: spelling-error %description -l en_US memcached -> cached, mimicked, coached
redis.src: W: invalid-url Source0: http://redis.googlecode.com/files/redis-1.2.6.tar.gz HTTP Error 404: Not Found
1 packages and 0 specfiles checked; 0 errors, 2 warnings.
$ rpmlint redis.spec 
redis.spec: W: invalid-url Source0: http://redis.googlecode.com/files/redis-1.2.6.tar.gz HTTP Error 404: Not Found
0 packages and 1 specfiles checked; 0 errors, 1 warnings.
$ 

[X] The package must be named according to the Package Naming Guidelines .
[X] The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. 
[X] The package must meet the Packaging Guidelines
[?] The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .

hiredis.c, hiredis.h don't have licensing or copyright information in them. I'm not sure if this poses a problem or not.

[X] The License field in the package spec file must match the actual license.
[X] 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.
[X] The spec file must be written in American English.
[X] The spec file for the package MUST be legible.
[X] The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. 

They're both 0c5355e57606523f9e8ce816db5e542f.

[X] The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[-] If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
[X] All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines 
[-] The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
[-] Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. 
[X] Packages must NOT bundle copies of system libraries.
[-] If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. 
[X] A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 
[X] A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations)
[X] Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. 
[X] Each package must consistently use macros. 
[X] The package must contain code, or permissable content. 
[-] Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). 
[-] If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. 
[-] Header files must be in a -devel package. 
[-] Static libraries must be in a -static package. 
[-] If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. 
[-] In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} 
[X] Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
[-] Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. 
[X] Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. 
[X] All filenames in rpm packages must be valid UTF-8. 

SHOULD
------
[-] If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[-] The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. 
[X] The reviewer should test that the package builds in mock. 
[?] The package should compile and build into binary rpms on all supported architectures. 
[X] The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.

I built and tested redis on FC13. I did some of the first examples from http://code.google.com/p/redis/wiki/TwitterAlikeExample and it works fine.

[-] If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. 
[-] Usually, subpackages other than devel should require the base package using a fully versioned dependency. 
[-] The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. 
[-] If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. 
[X] your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.

Would be nice if upstream included these too.


COMMENTS
--------
1. You could use the tarball's redis.conf and use sed to get it to the desired state. The following will do it for you: sed -i -e 's|daemonize no|daemonize yes|' -e 's|pidfile /var/run/redis\.pid|pidfile /var/run/redis/redis.pid|' -e 's|# bind 127.0.0.1|bind 127.0.0.1|' -e 's|loglevel debug|loglevel notice|' -e 's|logfile stdout|logfile /var/log/redis/redis.log|' -e 's|dir \./|dir /var/lib/redis/|' ./redis.conf

2. You could replace 'redis' with the %{name} macro in a lot of places in your spec if you want.

Thanks for the review Mark.

SRPM:
http://github.com/downloads/silas/rpms/redis-1.2.6-2.fc13.src.rpm

Diff:
http://github.com/silas/rpms/commit/16625b7e23b26e4b69563a8e65fda542ef4891e7

Response to Comments:
 1. I used a patch instead of sed (just for readability)
 2. Updated

Hi Silas,

here are some more things to consider:

- add a comment telling what the patch does

- append CFLAGS='%{optflags} -std=c99' to the "make" statement in order to 
  apply Fedora's default build flags
  (I also suggest to replace %{__make} with "make")

- add "sed -i '/^DEBUG/d' Makefile" before "make" (or patch the Makefile) 
  to remove the additional debug flags

- add the following files with %doc:
  00-RELEASENOTES, BUGS, Changelog, TODO
  and drop README as it doesn't contain any useful information

- if you plan to maintain this package for Fedora only, replace 
  %{_initrddir} with %{_initddir} (%{_initrddir} is deprecated but still
  required for EPEL <= 5)

- in the scriptlets, ensure that stderr is also redirected to /dev/null, e.g. 
  by adding "2>&1"

- I recommend to add a check for the config file to function start() in the
  initscript: 
  [ -f $REDIS_CONFIG ] || exit 6

I highly recommend to rename %{_initddir}/redis to %{_initddir}/redis-server since the daemon excutable name is redis-server. Actually, most daemon are end with d or -server.

Installing %{name}-server to %{_sbindir} may also not be appropriate as the redis-server is very useful for normal users.

From upstream config file:

# By default Redis does not run as a daemon. Use 'yes' if you need it.# Note that Redis will write a pid file in /var/run/redis.pid when daemonized.

Maybe you can install redis.conf to some non-standard place(e.g. /etc/redis/redis.conf), then you can pass the place of configfile to redis-server in initscript, so that redis.conf can also run in non-daemon mode.

See http://packages.debian.org/experimental/redis-server

SRPM:
http://github.com/downloads/silas/rpms/redis-1.2.6-3.fc15.src.rpm

Diff:
http://github.com/silas/rpms/commit/8adc996ca389e52686216a5faead0218f10dab9a

* Update to redis 2.0.0
* Remove man pages in favor of waiting for upstream (http://code.google.com/p/redis/issues/detail?id=202)
* Add %check section

SRPM:
http://github.com/downloads/silas/rpms/redis-2.0.0-1.fc13.src.rpm

Diff:
http://github.com/silas/rpms/commit/2669a446bc9b1ef2d5bf8081686a59947b727676

rpmlint

redis.src: W: spelling-error %description -l en_US memcached -> cached, mimicked, coached
redis.src: W: invalid-url Source0: http://redis.googlecode.com/files/redis-2.0.0.tar.gz HTTP Error 404: Not Found
redis.x86_64: W: spelling-error %description -l en_US memcached -> cached, mimicked, coached
redis.x86_64: W: non-standard-uid /var/lib/redis redis
redis.x86_64: W: non-standard-uid /var/log/redis redis
redis.x86_64: W: non-standard-uid /var/run/redis redis
redis.x86_64: W: no-manual-page-for-binary redis-check-aof
redis.x86_64: W: no-manual-page-for-binary redis-server
redis.x86_64: W: no-manual-page-for-binary redis-benchmark
redis.x86_64: W: no-manual-page-for-binary redis-check-dump
redis.x86_64: W: no-manual-page-for-binary redis-cli
3 packages and 0 specfiles checked; 0 errors, 11 warnings.

* Update to 2.0.1

SRPM:
http://github.com/downloads/silas/rpms/redis-2.0.1-1.fc13.src.rpm

I'll review it

Koji scratch build for F-14 (wip currently):

http://koji.fedoraproject.org/koji/taskinfo?taskID=2519746

REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

+ rpmlint is NOT silent but all its messages should be omitted:

work ~: rpmlint Desktop/redis-*
redis.i686: W: spelling-error %description -l en_US memcached -> mem cached, mem-cached, mustached
redis.i686: W: non-standard-uid /var/lib/redis redis
redis.i686: W: non-standard-uid /var/log/redis redis
redis.i686: W: non-standard-uid /var/run/redis redis
redis.i686: W: no-manual-page-for-binary redis-check-aof
redis.i686: W: no-manual-page-for-binary redis-server
redis.i686: W: no-manual-page-for-binary redis-benchmark
redis.i686: W: no-manual-page-for-binary redis-check-dump
redis.i686: W: no-manual-page-for-binary redis-cli
2 packages and 0 specfiles checked; 0 errors, 9 warnings.
work ~: 

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license (BSD).
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

Sulaco ~/rpmbuild/SOURCES: sha256sum redis-2.0.1.tar.gz*
4a20e667fe4267e1eb743d9b929a3662f7d5e211ef036c8dd4a7280f51a3b169  redis-2.0.1.tar.gz
4a20e667fe4267e1eb743d9b929a3662f7d5e211ef036c8dd4a7280f51a3b169  redis-2.0.1.tar.gz.1
Sulaco ~/rpmbuild/SOURCES:

+ The package successfully compiles and builds into binary rpms on at least one primary architecture.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files.
+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No header files.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain library files with a suffix (e.g. libfoo.so.1.1).
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ All filenames in rpm packages are valid UTF-8

Ok, I didn't find any issues so this package is

APPROVED.

- Update to 2.0.2
- Add guard around checks for el5

SRPM: http://github.com/downloads/silas/rpms/redis-2.0.2-1.fc13.src.rpm
Diff: http://github.com/silas/rpms/commit/c08037e875e3edbebc4ca9a976516ef5eb421a3a

New Package CVS Request
=======================
Package Name: redis
Short Description: A persistent key-value database
Owners: silas
Branches: F-13 F-14 EL-5 EL-6
InitialCC:

Git done (by process-git-requests).

Ping! Still no builds for redis in Koji.

Sorry about the delay, will be pushing to testing shortly.

redis-2.0.3-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/redis-2.0.3-1.fc14

redis-2.0.3-1.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/redis-2.0.3-1.fc13

redis-2.0.3-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/redis-2.0.3-1.el5

redis-2.0.3-1.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update redis'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/redis-2.0.3-1.fc13

redis-2.0.3-1.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

redis-2.0.3-2.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/redis-2.0.3-2.el5

redis-2.0.3-1.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

redis-2.0.3-2.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: redis
New Branches: epel7
Owners: silas cicku

Git done (by process-git-requests).