Bug 652623 - Review Request: erlang-bitcask - Eric Brewer-inspired key/value store
Review Request: erlang-bitcask - Eric Brewer-inspired key/value store
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville-Pekka Vainio
Fedora Extras Quality Assurance
:
Depends On: 639263 652616
Blocks: 652629 652682
  Show dependency treegraph
 
Reported: 2010-11-12 06:12 EST by Peter Lemenkov
Modified: 2012-07-27 18:07 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-17 12:46:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
vpvainio: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Peter Lemenkov 2010-11-12 06:12:04 EST
Spec URL: http://peter.fedorapeople.org/erlang-bitcask.spec
SRPM URL: http://peter.fedorapeople.org/erlang-bitcask-1.1.4-1.fc12.src.rpm
Description: Eric Brewer-inspired key/value store


This is one of the requirements for Riak.
Comment 1 Peter Lemenkov 2011-01-12 10:29:23 EST
Koji scratchbuild for Rawhide (erlang-ebloom was packaged):

http://koji.fedoraproject.org/koji/taskinfo?taskID=2717015
Comment 2 Ville-Pekka Vainio 2011-01-12 10:56:41 EST
I'll take this.
Comment 3 Ville-Pekka Vainio 2011-01-13 09:29:24 EST
I'd like to discuss a couple of things before going through a "checklist" for the review. (I know very little about erlang, but a few observations anyway.)

- I'd like the build system to be even more verbose. For erl compiles it only outputs "Compiled src/bitcask_merge_worker.erl" or so. Could it also output the ecc call? (I'm assuming it's using ecc.) For C compiles it outputs something like "$CC -c $CFLAGS $DRV_CFLAGS c_src/bitcask_nifs.c -o c_src/bitcask_nifs.o" and then you need to figure out yourself from the rest of the output what those environment variables expand to. I'd like to see the exact commands it's running. However, if this is a shortcoming of rebar, then I won't consider this as a blocker for the review.

- Another thing about the build. When building the C code, I'm fairly certain your package isn't passing the Fedora build options to cc. I suggest using the following line in %build, or something similar: "CFLAGS=$RPM_OPT_FLAGS rebar compile -v"
Comment 4 Peter Lemenkov 2011-01-14 09:37:41 EST
(In reply to comment #3)
> - I'd like the build system to be even more verbose. For erl compiles it only
> outputs "Compiled src/bitcask_merge_worker.erl" or so. Could it also output the
> ecc call? (I'm assuming it's using ecc.) For C compiles it outputs something
> like "$CC -c $CFLAGS $DRV_CFLAGS c_src/bitcask_nifs.c -o c_src/bitcask_nifs.o"
> and then you need to figure out yourself from the rest of the output what those
> environment variables expand to. I'd like to see the exact commands it's
> running. However, if this is a shortcoming of rebar, then I won't consider this
> as a blocker for the review.

Unfortunately, this is a shortcoming of rebar itself. I'll try to do something with it in the future, but right now it should be considered as a "feature".

Just for the record - the only parameter which rebar passed to erlang bytecode compile is +debug_info, which ensures that we'll add a small description to the final bytecode, which greatly helps debugging.

This debugging section is simply discarded by Erlang VM while loading in the standard operational mode (w/o debugger's invocation), so it doesn't affect end-users. That's intended behavior and will be added as a "MUST" to final Erlang Packaging Guidelines.

> - Another thing about the build. When building the C code, I'm fairly certain
> your package isn't passing the Fedora build options to cc. I suggest using the
> following line in %build, or something similar: "CFLAGS=$RPM_OPT_FLAGS rebar
> compile -v"

Good catch! Thanks. Here is a new package (updated to latest 1.1.5 version): 

http://peter.fedorapeople.org/erlang-bitcask.spec
http://peter.fedorapeople.org/erlang-bitcask-1.1.5-1.fc12.src.rpm

Koji scratchbuild for Rawhide:

http://koji.fedoraproject.org/koji/taskinfo?taskID=2721264
Comment 5 Ville-Pekka Vainio 2011-01-14 12:55:27 EST
Since the package does not have a COPYING file or a mention of the license in the README file, I'm somewhat worried about the licensing of the two header files without a license header and of the whole doc/ directory. I have asked about this on the legal mailing list: http://lists.fedoraproject.org/pipermail/legal/2011-January/001495.html
Comment 6 Ville-Pekka Vainio 2011-01-14 13:39:15 EST
Tom Callaway answered my questions: http://lists.fedoraproject.org/pipermail/legal/2011-January/001497.html . We need licenses for these files (include/bitcask.hrl and c_src/erl_nif_compat.h and the contents of the doc/ directory) from the copyright holders. Peter, could you be in contact with upstream about this?
Comment 7 Ville-Pekka Vainio 2011-04-14 15:02:11 EDT
Ping?
Comment 8 Peter Lemenkov 2011-04-14 15:41:24 EDT
(In reply to comment #7)
> Ping?

Hello.

Unfortunately upstream still not added licensing information to files in question. I'll try tofind whether could I just rewrite these problematic files (this doesn't looks like a big deal).
Comment 9 Peter Lemenkov 2011-04-18 07:47:04 EDT
(In reply to comment #7)
> Ping?

The upstream added a licensing info to one of two files in question

https://github.com/basho/bitcask/commit/6e4fc07

I don't think that we need an explicit licensing information for include/bitcask.hrl as it might be considered as a derivative work from files in src/ directory (thus inheriting all their legal information). Unfortunately (or fortunately) IANAL.
Comment 10 Ville-Pekka Vainio 2011-07-19 13:00:55 EDT
(In reply to comment #9)
> I don't think that we need an explicit licensing information for
> include/bitcask.hrl as it might be considered as a derivative work from files
> in src/ directory (thus inheriting all their legal information). Unfortunately
> (or fortunately) IANAL.

Ping?

I could accept this file without the license headers, but maybe we could get a confirmation about the license from the upstream developers first. If the files in the doc/ directory still have no licenses, they would need to be dropped from the package.
Comment 11 Peter Lemenkov 2011-07-25 02:43:32 EDT
(In reply to comment #10)
> (In reply to comment #9)
> > I don't think that we need an explicit licensing information for
> > include/bitcask.hrl as it might be considered as a derivative work from files
> > in src/ directory (thus inheriting all their legal information). Unfortunately
> > (or fortunately) IANAL.
> 
> Ping?
> 
> I could accept this file without the license headers, but maybe we could get a
> confirmation about the license from the upstream developers first. If the files
> in the doc/ directory still have no licenses, they would need to be dropped
> from the package.

Still here. I plan to return to the Riak quite soon - in a week or two. I'll rewrite these files and will drop additional content.
Comment 12 Peter Lemenkov 2011-09-16 05:42:46 EDT
Heads up!

Upstream just released ver. 1.2.0 with almost all legal issues fixed. The remaining one is the absence of the explicit licensing in the include/bitcask.hrl header file (which is so simple that I believe it could be considered as a derivative work under the same licensing terms as the rest of the project, e.g. ASL 2.0). However just to be sure I've added confirmation letter from one of the upstream's authors (with some unrelated conversation removed):

http://peter.fedorapeople.org/bitcask.licensing

Here is the latest package:

http://peter.fedorapeople.org/erlang-bitcask.spec
http://peter.fedorapeople.org/erlang-bitcask-1.2.0-1.fc16.src.rpm
Comment 13 Peter Lemenkov 2012-05-17 08:44:10 EDT
Ver. 1.5.1:

http://peter.fedorapeople.org/erlang-bitcask.spec
http://peter.fedorapeople.org/erlang-bitcask-1.5.1-1.fc18.src.rpm

Koji scratchbuild for F-18:
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=4083524

* Dropped dependency on erlang-ebloom.
* include/bitcask.hrl still doesn't contain explicit legal info but I've got a email from  the upstream (Source1: bitcask.licensing, included in the package as %doc).

http://peter.fedorapeople.org/bitcask.licensing

rpmlint:

sulaco ~/rpmbuild/SPECS: rpmlint ../RPMS/ppc/erlang-bitcask-* ../SRPMS/erlang-bitcask-1.5.1-1.fc18.src.rpm 
erlang-bitcask.src: W: invalid-url Source0: basho-bitcask-1.5.1-0-g4c6d6df.tar.gz

^^^ this may be omitted - that's an issue with github's tarball naming scheme.

3 packages and 0 specfiles checked; 0 errors, 1 warnings.
sulaco ~/rpmbuild/SPECS:
Comment 14 Ville-Pekka Vainio 2012-07-09 08:40:25 EDT
The package seems fine otherwise, but there is one thing I'd like to see either fixed or explained.

https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires says: "When explicit library Requires are necessary, explicit library dependencies should typically be arch-specific (unless the packages involved are noarch) and there should be a spec file comment justifying it."

Unless I'm mistaken (I know very little about erlang), you should mark all Requires arch-specific like this: "Requires: libfubar%{?_isa}".

I also want to point out that I would still like to see a more verbose build output and a COPYING file, but it seems upstream is not that responsive and I consider the licensing issues fixed now that you have included the bitcask.licensing file.
Comment 15 Peter Lemenkov 2012-07-09 09:04:11 EDT
(In reply to comment #14)
> Unless I'm mistaken (I know very little about erlang), you should mark all
> Requires arch-specific like this: "Requires: libfubar%{?_isa}".

Good catch! Fixed.
 
> I also want to point out that I would still like to see a more verbose build
> output and a COPYING file, but it seems upstream is not that responsive and
> I consider the licensing issues fixed now that you have included the
> bitcask.licensing file.

The first suggestion will require patching rebar (an Erlang build tool) which is frankly speaking is not a high priority task. I plan to improve the situation but I wouldn't hold my breath here since nothing changed here since the beginning of 2011. As for the second - I believe it's ok now since they explicitly covered all files with a licensing info except the only one (include/bitcask.hrl), which is really simple and contains only constants and typedefs.

* http://peter.fedorapeople.org/erlang-bitcask.spec
* http://peter.fedorapeople.org/erlang-bitcask-1.5.1-2.fc18.src.rpm
Comment 16 Ville-Pekka Vainio 2012-07-09 15:12:20 EDT
Here's the checklist from fedora-review. My comments start with "vpv:".

Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== C/C++ ====
[x]: MUST Header files in -devel subpackage, if present.
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Package is not relocatable.
[!]: MUST Development (unversioned) .so files in -devel subpackage, if
     present.
     Note: erlang-bitcask-1.5.1-2.fc18.x86_64.rpm :
     /usr/lib64/erlang/lib/bitcask-1.5.1/priv/bitcask.so
     vpv: Does not apply in a package like this because the unversioned .so is all there is.


==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[!]: MUST Buildroot is not present
     Note: Buildroot is not needed unless packager plans to package for EPEL5
     vpv: This is ok, if Peter wants an EPEL5 branch, he should then take this into consideration. 
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[!]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean is needed only if supporting EPEL
     vpv: This is ok, if Peter wants an EPEL5 branch, he should then take this into consideration.
[x]: MUST Sources contain only permissible code or content.
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files section. This is OK if packaging
     for EPEL5. Otherwise not needed
     vpv: In my opinion there is nothing wrong in having a sensible deffatr like in this spec even though it is not required.
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
     vpv: This is ok, if Peter wants an EPEL5 branch, he should then take this into consideration.
[x]: MUST Large documentation files are in a -doc subpackage, if required.
     vpv: N/A
[x]: MUST 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 is included in %doc.
     vpv: N/A
[x]: MUST License field in the package spec file matches the actual license.
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
     vpv: Peter can fix this if he wants to, but I will not consider this as a blocker.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint output is silent.

rpmlint erlang-bitcask-1.5.1-2.fc18.src.rpm

erlang-bitcask.src: W: invalid-url Source0: basho-bitcask-1.5.1-0-g4c6d6df.tar.gz
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint erlang-bitcask-debuginfo-1.5.1-2.fc18.x86_64.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint erlang-bitcask-1.5.1-2.fc18.x86_64.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.

     vpv: invalid-url caused by github, not a blocker
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: MUST Package contains a SysV-style init script if in need of one.
     vpv: N/A
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: 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.
     vpv: Upstream has been asked but they have not yet added such a file.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[?]: SHOULD Package functions as described.
     vpv: Did not test as I have almost no erlang knowledge. I am going to trust Peter here.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX is a working URL.
[x]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
     vpv: N/A
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
     vpv: According to Peter's scratch build they do.
[x]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Buildroot is not present
     Note: Buildroot is not needed unless packager plans to package for EPEL5
See: http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
[!]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean is needed only if supporting EPEL
See: http://fedoraproject.org/wiki/Packaging/Guidelines#.25clean
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files section. This is OK if packaging
     for EPEL5. Otherwise not needed
See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
See: None
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
See: http://fedoraproject.org/wiki/Packaging/Guidelines#macros
[!]: MUST Rpmlint output is silent.

rpmlint erlang-bitcask-1.5.1-2.fc18.src.rpm

erlang-bitcask.src: W: invalid-url Source0: basho-bitcask-1.5.1-0-g4c6d6df.tar.gz
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint erlang-bitcask-debuginfo-1.5.1-2.fc18.x86_64.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint erlang-bitcask-1.5.1-2.fc18.x86_64.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
[!]: MUST Development (unversioned) .so files in -devel subpackage, if
     present.
     Note: erlang-bitcask-1.5.1-2.fc18.x86_64.rpm :
     /usr/lib64/erlang/lib/bitcask-1.5.1/priv/bitcask.so
See: http://fedoraproject.org/wiki/Packaging/Guidelines#DevelPackages


Generated by fedora-review 0.1.3
External plugins:

--

Peter: The only real issue I found is that you should use either %{buildroot} or $RPM_BUILD_ROOT, not both. But this is not a review blocker and you can fix before doing the first Fedora build. 

The review is passed.
Comment 17 Peter Lemenkov 2012-07-10 06:15:17 EDT
(In reply to comment #16)

> Peter: The only real issue I found is that you should use either
> %{buildroot} or $RPM_BUILD_ROOT, not both. But this is not a review blocker
> and you can fix before doing the first Fedora build. 

I'll do.

> The review is passed.

Thanks for reviewing this! This is a very important piece of a much more bigger picture so your efforts are very much appreciated.

New Package SCM Request
=======================
Package Name: erlang-bitcask
Short Description: Eric Brewer-inspired key/value store
Owners: peter
Branches: f17 el6
InitialCC:
Comment 18 Kevin Fenzi 2012-07-10 18:16:04 EDT
Git done (by process-git-requests).
Comment 19 Fedora Update System 2012-07-11 01:50:31 EDT
erlang-bitcask-1.5.1-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/erlang-bitcask-1.5.1-2.fc17
Comment 20 Fedora Update System 2012-07-11 13:52:34 EDT
erlang-bitcask-1.5.1-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/erlang-bitcask-1.5.1-2.el6
Comment 21 Fedora Update System 2012-07-11 19:59:55 EDT
erlang-bitcask-1.5.1-2.fc17 has been pushed to the Fedora 17 testing repository.
Comment 22 Fedora Update System 2012-07-19 21:52:30 EDT
erlang-bitcask-1.5.1-2.fc17 has been pushed to the Fedora 17 stable repository.
Comment 23 Fedora Update System 2012-07-27 18:07:21 EDT
erlang-bitcask-1.5.1-2.el6 has been pushed to the Fedora EPEL 6 stable repository.

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