Bug 477570 - Review Request: couchdb - A document database server, accessible via a RESTful JSON API
Review Request: couchdb - A document database server, accessible via a RESTfu...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-21 22:44 EST by Allisson Azevedo
Modified: 2013-10-23 14:19 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-02 19:55:30 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lemenkov: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Allisson Azevedo 2008-12-21 22:44:38 EST
Spec URL: http://allisson.fedorapeople.org/packages/couchdb/couchdb.spec
SRPM URL: http://allisson.fedorapeople.org/packages/couchdb/couchdb-0.8.1-1.fc9.src.rpm
Description: Apache CouchDB is a distributed, fault-tolerant and schema-free 
document-oriented database accessible via a RESTful HTTP/JSON API. 
Among other features, it provides robust, incremental replication 
with bi-directional conflict detection and resolution, and is 
queryable and indexable using a table-oriented view engine with 
JavaScript acting as the default view definition language.
Comment 1 Peter Lemenkov 2009-02-04 07:05:22 EST
I'll review it.
Comment 2 Hubert Plociniczak 2009-02-04 12:58:41 EST
I am sure Peter will follow with the proper review, but here are some bits that I found in my informal review:

- put Requires(preun) into single line
- %{_sysconfdir}/sysconfig/couchdb instead of ${_sysconfdir}/default/couchdb
- you have 
%exclude %{_sysconfdir}/rc.d/couchdb
%{_sysconfdir}/rc.d/couchdb

- use %{_initrddir} instead of %{_sysconfdir}

- init-script must not be marked as %config (http://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscript_packaging)

- if you use %{_localstatedir} then use it consistently, for example in useradd or sed

- use -D flag in 'install' instead of 'mkdir -p $RPM_BUILD_ROOT%{_initrddir}'
- remove 'exit 0' in %pre
- remove 'shadow-utils' since it is on ExceptionsList (http://fedoraproject.org/wiki/Packaging:Guidelines)
- shouldn't mochiweb be a separate sub-package?
- fix the obvious rpmlint warnings, like permissions
Comment 3 Allisson Azevedo 2009-02-10 15:29:29 EST
Update package:

Spec URL: http://allisson.fedorapeople.org/packages/couchdb/couchdb.spec

SRPM URL:
http://allisson.fedorapeople.org/packages/couchdb/couchdb-0.8.1-2.fc10.src.rpm

Rpmlint output:

[allisson@notebook RPMS]$ rpmlint couchdb-0.8.1-2.fc11.i386.rpm 
couchdb.i386: W: non-standard-uid /var/lib/couchdb couchdb
couchdb.i386: W: non-standard-uid /var/run/couchdb couchdb
couchdb.i386: W: non-standard-uid /var/log/couchdb couchdb
couchdb.i386: E: invalid-soname /usr/lib/couchdb/erlang/lib/couch-0.8.1-incubating/priv/lib/couch_erl_driver.so couch_erl_driver.so
1 packages and 0 specfiles checked; 1 errors, 3 warnings.
Comment 4 Peter Lemenkov 2009-02-16 10:34:29 EST
* I also think that the mochiweb should be excluded and packaged separately. How different version, shipped with couchdb, from the upstream one? 

* rpmlint warnings (and one error) should be safely ignored.

* "Requires: %{_bindir}/icu-config" looks ugly. First, is it really need, to have libicu-devel, to couchdb to work? Second, why you decided to require %{_bindir}/icu-config instead of libicu-devel?

Other (still remaining) issues from Hubert's notes:

* No need to explicitly require shadow-utils (as Hubert already mentioned).

* Use %{_localstatedir} in sed string and in couchdb_home var instead of /var 

* remove 'exit 0' in %pre

* use %{_initrddir} instead of %{_sysconfdir}
Comment 5 Allisson Azevedo 2009-02-16 11:53:41 EST
Hi Peter,

> * I also think that the mochiweb should be excluded and packaged separately.
> How different version, shipped with couchdb, from the upstream one?
I think is better wait for first official release of mochiweb.

> * "Requires: %{_bindir}/icu-config" looks ugly. First, is it really need, to
> have libicu-devel, to couchdb to work? Second, why you decided to require
> %{_bindir}/icu-config instead of libicu-devel?
Using "requires: libicu-devel results in two erros in rpmlint output:

[allisson@notebook RPMS]$ rpmlint couchdb-0.8.1-3.fc11.i386.rpm 
couchdb.i386: W: non-standard-uid /var/lib/couchdb couchdb
couchdb.i386: W: non-standard-uid /var/run/couchdb couchdb
couchdb.i386: W: non-standard-uid /var/log/couchdb couchdb
couchdb.i386: E: devel-dependency libicu-devel
couchdb.i386: E: explicit-lib-dependency libicu-devel
couchdb.i386: E: invalid-soname /usr/lib/couchdb/erlang/lib/couch-0.8.1-incubating/priv/lib/couch_erl_driver.so couch_erl_driver.so
1 packages and 0 specfiles checked; 3 errors, 3 warnings.

> * No need to explicitly require shadow-utils (as Hubert already mentioned).
> * remove 'exit 0' in %pre
I following http://fedoraproject.org/wiki/Packaging/UsersAndGroups

> * use %{_initrddir} instead of %{_sysconfdir}
I don't see is wrong here, i using %{_initrddir}/couchdb for sysvinit script.
Comment 7 Peter Lemenkov 2009-02-18 03:21:30 EST
(In reply to comment #5)

> > * I also think that the mochiweb should be excluded and packaged separately.
> > How different version, shipped with couchdb, from the upstream one?
> I think is better wait for first official release of mochiweb.

OK, let's wait. But keep im mind that you must remove mochiweb from couchdb as soon as mochiweb will be packaged (someday, I hope) for Fedora.

> > * "Requires: %{_bindir}/icu-config" looks ugly. First, is it really need, to
> > have libicu-devel, to couchdb to work? Second, why you decided to require
> > %{_bindir}/icu-config instead of libicu-devel?
> Using "requires: libicu-devel results in two erros in rpmlint output:

Yes, but I still think that it's more understandable for others. In any case,I personally still don't understand why couchdb needs devel-package to work.

> > * No need to explicitly require shadow-utils (as Hubert already mentioned).
> > * remove 'exit 0' in %pre
> I following http://fedoraproject.org/wiki/Packaging/UsersAndGroups

OK.

> > * use %{_initrddir} instead of %{_sysconfdir}
> I don't see is wrong here, i using %{_initrddir}/couchdb for sysvinit script.

Looks like this is my fault - I definitely confused with something. In any case - the current spec-file looks ok in terms of using  %{_initrddir} and %{_sysconfdir}.


However some things still remains unresolved.

* sed oneliner at line 67 should be changed - you must use %{_localstatedir} instead of /var
* Consider using /etc/sysconfig ( %{_sysconfdir}/sysconfig ) instead of /etc/default for storing init-script's settings.
Comment 8 Allisson Azevedo 2009-02-18 14:20:25 EST
> Yes, but I still think that it's more understandable for others. In any case,I
> personally still don't understand why couchdb needs devel-package to work.
CouchDB needs icu-config for /usr/bin/couchdb, see line 173.

I'll fix the others issues.
Comment 10 Allisson Azevedo 2009-02-27 19:43:51 EST
Ping.
Comment 11 Peter Lemenkov 2009-03-01 06:10:17 EST
REVIEW:

+/- rpmlint is not silent. However, all these messages already mentioned above and have been explained.

[petro@Sulaco ppc]$ rpmlint couchdb-*
couchdb.ppc: W: non-standard-uid /var/lib/couchdb couchdb
couchdb.ppc: W: non-standard-uid /var/run/couchdb couchdb
couchdb.ppc: W: non-standard-uid /var/log/couchdb couchdb
couchdb.ppc: E: devel-dependency libicu-devel
couchdb.ppc: E: explicit-lib-dependency libicu-devel
couchdb.ppc: E: invalid-soname /usr/lib/couchdb/erlang/lib/couch-0.8.1-incubating/priv/lib/couch_erl_driver.so couch_erl_driver.so
2 packages and 0 specfiles checked; 3 errors, 3 warnings.
[petro@Sulaco ppc]

+ 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.
+ 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 matches the upstream source, as provided in the spec URL.

[petro@Sulaco SOURCES]$ wget http://www.apache.org/dist/incubator/couchdb/0.8.1-incubating/apache-couchdb-0.8.1-incubating.tar.gz.md5 -q -O - && md5sum apache-couchdb-0.8.1-incubating.tar.gz 
89e037b370bef33be93f0f317e07615f  apache-couchdb-0.8.1-incubating.tar.gz
89e037b370bef33be93f0f317e07615f  apache-couchdb-0.8.1-incubating.tar.gz
[petro@Sulaco SOURCES]$

+ The package successfully compiles and builds into binary rpms on all primary architectures:

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

+ All build dependencies are listed in BuildRequires.
+ No need to handle locales.
+ The package calls ldconfig in %post and %postun.
+ The package owns all directories that it creates.
+ The package does not contain any duplicate files in the %files listing.
+ 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 permissable content.
+ No large documentation files.
+ Everything, the package includes as %doc, does not affect the runtime of the application.
+ No c-header files.
+ No static libraries.
+ No pkgconfig(.pc) files.
+ No library files with a suffix.
+ The package does NOT contain any .la libtool archives.
+ 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 must be valid UTF-8.

I've got only one final advise - another one fix for already mentioned above sed one-liner :). Instead of current's:

sed -i 's/\%{_localstatedir}\/run\/couchdb.pid/\%{_localstatedir}\/run\/couchdb\/couchdb.pid/g' \
$RPM_BUILD_ROOT%{_sysconfdir}/sysconfig/couchdb

It should looks like:

sed -i 's/\/var\/run\/couchdb.pid/\%{_localstatedir}\/run\/couchdb\/couchdb.pid/g' \
$RPM_BUILD_ROOT%{_sysconfdir}/default/couchdb


Note - I changed only the latter mentioning of /var.

Please, consider adding this final change, and this package is

============
============
= APPROVED =
============
============
Comment 12 Allisson Azevedo 2009-03-01 07:23:48 EST
New Package CVS Request
=======================
Package Name: couchdb
Short Description: A document database server, accessible via a RESTful JSON API
Owners: allisson
Branches: F-9 F-10 EL-5
Comment 13 Kevin Fenzi 2009-03-02 19:16:20 EST
cvs done.
Comment 14 Allisson Azevedo 2009-03-03 06:01:50 EST
Hi, i can't see EL-5 branch for CouchDB.
Comment 15 Kevin Fenzi 2009-03-03 15:48:38 EST
sorry about that. Try now?
Comment 16 Peter Lemenkov 2013-10-23 13:58:38 EDT
Package Change Request
======================
Package Name: couchdb
InitialCC: erlang-sig
Comment 17 Gwyn Ciesla 2013-10-23 14:19:22 EDT
Done.

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