Bug 652623
Summary: | Review Request: erlang-bitcask - Eric Brewer-inspired key/value store | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Peter Lemenkov <lemenkov> |
Component: | Package Review | Assignee: | Ville-Pekka Vainio <vpvainio> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, notting, vpvainio |
Target Milestone: | --- | Flags: | vpvainio:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2012-07-17 16:46:23 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | 639263, 652616 | ||
Bug Blocks: | 652629, 652682 |
Description
Peter Lemenkov
2010-11-12 11:12:04 UTC
Koji scratchbuild for Rawhide (erlang-ebloom was packaged): http://koji.fedoraproject.org/koji/taskinfo?taskID=2717015 I'll take this. 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" (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 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 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? Ping? (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). (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. (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. (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. 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 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: 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. (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 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. (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: Git done (by process-git-requests). 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 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 erlang-bitcask-1.5.1-2.fc17 has been pushed to the Fedora 17 testing repository. erlang-bitcask-1.5.1-2.fc17 has been pushed to the Fedora 17 stable repository. erlang-bitcask-1.5.1-2.el6 has been pushed to the Fedora EPEL 6 stable repository. |