Bug 652682

Unblocking luwak - I'll add this backend later.

Ok, this is finally ready for review:

* http://peter.fedorapeople.org/riak.spec
* http://peter.fedorapeople.org/riak-1.1.4-1.fc18.src.rpm

Note - I intentionally packaged ver. 1.1.4 - I'll upgrade it to 1.2.0 this september (with luwak enabled and with several new dependencies).

I'll review this one.

Created attachment 604285 [details]
Mock build.log for rawhide-x86_64 configuration.

Hi Peter, 

For a start, it doesn't build in mock :) (Log attached)

I'm in the middle of reviewing the other portions. Can you work on making it build in the mean time please?

Thanks,
Ankur

(In reply to comment #3)
> Created attachment 604285 [details]
> Mock build.log for rawhide-x86_64 configuration.
> 
> Hi Peter, 
> 
> For a start, it doesn't build in mock :) (Log attached)
> 
> I'm in the middle of reviewing the other portions. Can you work on making it
> build in the mean time please?
> 
> Thanks,
> Ankur

Ohhh, I terribly sorry for that! 
That's my fault (I blindly uploaded version which wasn't checked in Koji and there are lots of missing BuildRequires). Stay tuned - I'll return with updated version shortly.

Done. The same version, the same location:

* http://peter.fedorapeople.org/riak.spec
* http://peter.fedorapeople.org/riak-1.1.4-1.fc18.src.rpm

Koji scratchbuild for F-18:

* http://koji.fedoraproject.org/koji/taskinfo?taskID=4388435

A bit of story behind "rebar compile generate -v" - this compiles the only binary blob for Erlang VM ("compile" part) and builds so-called "release data" ("generate" part) - a blob describing all dependent Erlang packages, the order of starting Erlang applications, some hints for live upgrade (the prominent Erlang feature I plan to introduce as a Feature for F-19 or F-20) and so on.

Okay. Here's the review. Peter, I've never done an erlang package before, so quite a few of my comments are going to be just "please check this" rather than concrete suggestions :).

[+] OK
[-] NA
[?] Issue

[+] Package meets naming and packaging guidelines
[+] Spec file matches base package name.
[-] Spec has consistant macro usage.
[+] Meets Packaging Guidelines.
[+] License
[ankur@ankur basho-riak-83ec281]$ find . -name "*" -exec licensecheck '{}' \; | sed '/UNKNOWN/ d'
./package/deb/postinst: *No copyright* GENERATED FILE
./package/deb/copyright: *No copyright* Apache (v2.0)
./doc/basic-setup.txt: *No copyright* GENERATED FILE
[ankur@ankur basho-riak-83ec281]$

[+] License field in spec matches
[+] License file included in package
[+] Spec in American English
[+] Spec is legible.
[+] Sources match upstream md5sum:
[ankur@ankur NN-feature-detect-trials(master #)]$ wget --content-disposition https://github.com/basho/riak/tarball/riak-1.1.4
--2012-08-14 23:26:32--  https://github.com/basho/riak/tarball/riak-1.1.4
Resolving github.com... 207.97.227.239
Connecting to github.com|207.97.227.239|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://nodeload.github.com/basho/riak/tarball/riak-1.1.4 [following]
--2012-08-14 23:26:33--  https://nodeload.github.com/basho/riak/tarball/riak-1.1.4
Resolving nodeload.github.com... 207.97.227.252
Connecting to nodeload.github.com|207.97.227.252|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 228239 (223K) [application/octet-stream]
Saving to: `basho-riak-riak-1.1.4-0-g95c5cb6.tar.gz'
<snip>
5bfc5b77f520adc48eff426bc8f4db95  basho-riak-riak-1.1.4-0-g95c5cb6.tar.gz
5bfc5b77f520adc48eff426bc8f4db95  /home/ankur/rpmbuild/SOURCES/basho-riak-riak-1.1.4-0-g95c5cb6.tar.gz

[-] Package needs ExcludeArch
[+] BuildRequires correct
[-] Spec handles locales/find_lang
[-] Package is relocatable and has a reason to be.
[+] Package has %defattr and permissions on files is good.
[+] Package is code or permissible content.
[-] Doc subpackage needed/used.
[+] Packages %doc files don't affect runtime.

[-] Headers/static libs in -devel subpackage.
[-] Spec has needed ldconfig in post and postun
[-] .pc files in -devel subpackage/requires pkgconfig
[-] .so files in -devel subpackage.
[-] -devel package Requires: %{name} = %{version}-%{release}
[-] .la files are removed.

[-] Package is a GUI app and has a .desktop file

[+] Package compiles and builds on at least one arch.
[+] Package has no duplicate files in %files.
[+] Package doesn't own any directories other packages own.
[+] Package owns all the directories it creates.
[-] No rpmlint output.
[ankur@ankur SRPMS]$ rpmlint riak-1.1.4-1.fc18.src.rpm /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm ../SPECS/riak.spec
riak.src: W: invalid-url URL: https://wiki.basho.com/display/RIAK/Riak HTTP Error 404: Not Found
^^ 
? Checked in browser. Actually reports a 404. Please recheck

riak.src: W: strange-permission riak.init 0775L
^^
? Please check this (although not applicable to fedora)

riak.src:8: W: macro-in-comment %global
riak.src:8: W: macro-in-comment %{eval
riak.src:8: W: macro-in-comment %{VERSION}
riak.src:9: W: macro-in-comment %global
riak.src:9: W: macro-in-comment %{VERSION}
^^
+ Harmless

riak.src:16: W: mixed-use-of-spaces-and-tabs (spaces: line 16, tab: line 11)
^^
? Tiny cosmetic change

riak.x86_64: E: no-binary
^^
? I think this is okay since it's an erlang based package?


riak.x86_64: W: only-non-binary-in-usr-lib
^^ 
? Again, this is okay for erlang packages?


riak.x86_64: W: non-standard-uid /var/lib/riak/bitcask riak
riak.x86_64: W: non-standard-gid /var/lib/riak/bitcask riak
riak.x86_64: W: non-standard-uid /var/log/riak/sasl riak
riak.x86_64: W: non-standard-gid /var/log/riak/sasl riak
riak.x86_64: W: non-standard-uid /var/log/riak riak
riak.x86_64: W: non-standard-gid /var/log/riak riak
riak.x86_64: W: non-standard-uid /var/lib/riak/mr_queue riak
riak.x86_64: W: non-standard-gid /var/lib/riak/mr_queue riak
riak.x86_64: W: non-standard-uid /var/lib/riak/ring riak
riak.x86_64: W: non-standard-gid /var/lib/riak/ring riak
riak.x86_64: W: non-standard-uid /var/lib/riak/dets riak
riak.x86_64: W: non-standard-gid /var/lib/riak/dets riak
riak.x86_64: W: non-standard-uid /var/run/riak riak
riak.x86_64: W: non-standard-gid /var/run/riak riak
riak.x86_64: W: non-standard-uid /var/lib/riak/merge_index riak
riak.x86_64: W: non-standard-gid /var/lib/riak/merge_index riak
riak.x86_64: W: non-standard-uid /var/lib/riak/leveldb riak
riak.x86_64: W: non-standard-gid /var/lib/riak/leveldb riak
riak.x86_64: W: non-standard-uid /var/lib/riak riak
riak.x86_64: W: non-standard-gid /var/lib/riak riak
^^
+ Seems okay, since you're creating a user/group for the package in the spec.

riak.x86_64: W: non-conffile-in-etc /etc/tmpfiles.d/riak.conf
riak.x86_64: W: non-conffile-in-etc /etc/riak/vm.args
riak.x86_64: W: non-conffile-in-etc /etc/riak/app.config
^^
? Shouldn't these be marked as %config?

riak.x86_64: W: dangling-symlink /usr/lib64/riak/lib /usr/lib64/erlang/lib
riak.x86_64: W: dangling-symlink /usr/lib64/riak/erts-5.9.1 /usr/lib64/erlang/erts-5.9.1
^^
? Looks okay. I'm guessing the Requires will pull in the targets. 

riak.x86_64: W: manual-page-warning /usr/share/man/man1/riak-admin.1.gz 32: a newline character is not allowed in an escape name
^^
? Please see if you can correct this.

riak.x86_64: W: log-files-without-logrotate /var/log/riak
^^
? Please refer to https://fedoraproject.org/wiki/User:Johannbg/Packaging/LogFiles 
It says "This guidelines only applies if you are going to support additional optional syslog solutions such as rsyslog or syslog-ng in your package." so I think this is OK.
Please do take a look, just to be sure..

[ankur@ankur SRPMS]$
[+] final provides and requires are sane:
(include output of for i in *rpm; do echo $i; rpm [-]qp --provides $i; echo =; rpm -qp --requires $i; echo; done
manually indented after checking each line.  I also remove the rpmlib junk and anything provided by glibc.)
[ankur@ankur result]$ review-req-check
== riak-1.1.4-1.fc19.src.rpm ==
Provides:

Requires:
erlang-rebar
erlang-cluster_info
erlang-eper
erlang-riak_control
erlang-riak_kv
erlang-riak_search

== riak-1.1.4-1.fc19.x86_64.rpm ==
Provides:
riak = 1.1.4-1.fc19
riak(x86-64) = 1.1.4-1.fc19

Requires:
/bin/bash
/bin/sh
/bin/sh
/bin/sh
/usr/bin/env
erlang-cluster_info
erlang-eper
erlang-riak_control
erlang-riak_kv
erlang-riak_search
shadow-utils
systemd-units
systemd-units
systemd-units

^^
Looks okay to me.

SHOULD Items:

[+] Should build in mock.
[+] Should build on all supported archs
[-] Should function as described.
[+] Should have sane scriptlets.
[-] Should have subpackages require base package with fully versioned depend.
[+] Should have dist tag
[-] Should package latest version
[-] check for outstanding bugs on package. (For core merge reviews)


Thanks,
Ankur

(In reply to comment #6)

> riak.src: W: invalid-url URL: https://wiki.basho.com/display/RIAK/Riak HTTP
> Error 404: Not Found
> ^^ 
> ? Checked in browser. Actually reports a 404. Please recheck

Fixed.

> riak.src: W: strange-permission riak.init 0775L
> ^^
> ? Please check this (although not applicable to fedora)

Fixed.

> riak.src:8: W: macro-in-comment %global
> riak.src:8: W: macro-in-comment %{eval
> riak.src:8: W: macro-in-comment %{VERSION}
> riak.src:9: W: macro-in-comment %global
> riak.src:9: W: macro-in-comment %{VERSION}
> ^^
> + Harmless

This is a leftover. Removed.

> riak.src:16: W: mixed-use-of-spaces-and-tabs (spaces: line 16, tab: line 11)
> ^^
> ? Tiny cosmetic change

Fixed.

> riak.x86_64: E: no-binary
> ^^
> ? I think this is okay since it's an erlang based package?

Yep. That's how Erlang packages are designed. Some of them (as this one) contains only texts or platform-independent data but since they must be installed into %{_libdir} (which is arch-dependent) the entire package becomes arch-dependent.

> riak.x86_64: W: only-non-binary-in-usr-lib
> ^^ 
> ? Again, this is okay for erlang packages?

Yes, the same reason as in the warning above.

> riak.x86_64: W: non-conffile-in-etc /etc/tmpfiles.d/riak.conf
> riak.x86_64: W: non-conffile-in-etc /etc/riak/vm.args
> riak.x86_64: W: non-conffile-in-etc /etc/riak/app.config
> ^^
> ? Shouldn't these be marked as %config?

Definitely! I overlooked it - thanks for pointing me out on this.

> riak.x86_64: W: dangling-symlink /usr/lib64/riak/lib /usr/lib64/erlang/lib
> riak.x86_64: W: dangling-symlink /usr/lib64/riak/erts-5.9.1
> /usr/lib64/erlang/erts-5.9.1
> ^^
> ? Looks okay. I'm guessing the Requires will pull in the targets. 

Yep. I plan to improve it in the future (fully autogenerated Requires/Provides). I plan to propose it as a feature for F-19 or F-20. Actually I've got a plenty of proposals for Erlang in Fedora :).

> riak.x86_64: W: manual-page-warning /usr/share/man/man1/riak-admin.1.gz 32:
> a newline character is not allowed in an escape name
> ^^
> ? Please see if you can correct this.

Fixed.

> riak.x86_64: W: log-files-without-logrotate /var/log/riak
> ^^
> ? Please refer to
> https://fedoraproject.org/wiki/User:Johannbg/Packaging/LogFiles 
> It says "This guidelines only applies if you are going to support additional
> optional syslog solutions such as rsyslog or syslog-ng in your package." so
> I think this is OK.
> Please do take a look, just to be sure..

Actually I don't know for sure what's the best way to deal with Riak log. I plan to improve it later based on a user's feedback. So far I'd rather to keep it as is.

Here is the new build:

* http://peter.fedorapeople.org/riak.spec
* http://peter.fedorapeople.org/riak-1.1.4-2.fc18.src.rpm

Hi Peter,

Looks good! Did you miss a config file though? :)

riak.x86_64: W: non-conffile-in-etc /etc/tmpfiles.d/riak.conf

Please correct it before you commit to SCM/build for rawhide.

*** APPROVED ***

(In reply to comment #8)
> Hi Peter,
> 
> Looks good! Did you miss a config file though? :)
> 
> riak.x86_64: W: non-conffile-in-etc /etc/tmpfiles.d/riak.conf
> 
> Please correct it before you commit to SCM/build for rawhide.

Yep indeed - this should go into /usr/lib/tmpfiles.d/ - again, thanks for spotting this!

> *** APPROVED ***

Thanks!

New Package SCM Request
=======================
Package Name: riak
Short Description: Dynamo-inspired key/value store
Owners: peter
Branches: f17 f18 el6
InitialCC:

Git done (by process-git-requests).

I've just build it for Rawhide/F-18. F-17 and EL6 branches will follow in a couple of weeks (as soon as all dependencies will be available in updates).

riak-1.1.4-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/riak-1.1.4-2.fc18

riak-1.1.4-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/riak-1.1.4-2.fc17

riak-1.1.4-2.fc17 has been pushed to the Fedora 17 stable repository.

riak-1.1.4-2.fc18 has been pushed to the Fedora 18 stable repository.

Package Change Request
======================
Package Name: riak
InitialCC: erlang-sig

Done.