Bug 1070702

Christopher, one month has past since you assigned this review request to you, but nothing has happened to it.

Are you still interested in reviewing this submission?

Some remarks on this package:

- Building doesn't honor RPM_OPT_FLAGS.
One work-around to this seems to be passing them to make through XCFLAGS, i.e.
to use 
make %{?_smp_mflags} XCFLAGS="${RPM_OPT_FLAGS}"

- The package contains testsuite, which should be excercised, IMO.
AFAIS, it can be invoked this way:

%check
rm -rf testdb
make test LD_LIBRARY_PATH=$(pwd)

- COPYRIGHT and LICENSE need to be added to %doc

- Shipping Doxyfile in %doc doesn't make sense. It should be removed from %doc.

- The package seems to support doxygen-generated docs. I'd recommend to build and package them (but I haven checked whether these actually are useable).


Finally, the naming of the package is not clear to me. I see a mix of lmdb, mdb and liblmdb, which seems inconsistent to me. Unfortunately I don't have a clear vision/imagination about what would be the appropriate name for this package, rsp. under which name its users would expect to find it.

Also, noticing openldap seems to be the origin, I am not sure if prefixing this package with "openldap" would be adequate (i.e. openldap-lmdb or openldap-liblmdb).

Updated package:
Spec URL: http://jstanek.fedorapeople.org/lmdb/liblmdb.spec
SRPM URL: http://jstanek.fedorapeople.org/lmdb/liblmdb-0.9.11-1.fc21.src.rpm

Changes:
 - Renamed package to liblmdb
 - Added %{optflags} to make and %check section for testsuite.
 - Removed Doxyfile, instead shipping the html Doxygen documentation in -devel %doc

Comments:

Ad COPYRIGHT and LICENSE - They are already in the main package %doc section. Or am I missing something?

Ad naming - The project was originally called MDB, but then got renamed to LMDB, so I took the newer name as package name. However, the embedded databases are mostly libraries, and since upstream uses "liblmdb" as the source directory's name, I renamed it to liblmdb. That name should IMHO be the least confusing.

As for the openldap- prefix, I'd prefer not to add it - LMDB is (according it's webpage) separate project (although it is developed in OpenLDAP) and the goal of this package is provide the database independent from OpenLDAP as possible replacement for BerkeleyDB v6+.

Fyi, the project name is LMDB. "liblmdb" is just POSIX convention since all libraries begin with a "lib" prefix - the prefix is insignificant as far as naming goes and surely I'm not telling you something you didn't already know.

From my point of view the package should be named "lmdb" as it is upstream
name. Only libraries should be liblmdb. The "l" is for "lightning", right?

Right. The main significance is to prevent it getting confused with mdbtools, which is used to manipulate Microsoft Access DB files.

As for the "test suite" - it's really only there for my use, as a developer of LMDB. It's not necessary or intended for anyone else to ever touch the test programs. It's main usefulness is in verifying basic functionality when porting to new CPU architectures, not as a regression test suite.

Patches to add a proper regression test with e.g. gcov would certainly be welcome.

(In reply to Robert Scheck from comment #5)
> From my point of view the package should be named "lmdb" as it is upstream
> name. Only libraries should be liblmdb. The "l" is for "lightning", right?

The point of the package name "liblmdb" is that the whole project is a library - the test programs really does not count. I feel that this area is a bit hazy, but in this case I used our naming schema of libdb/BerkeleyDB - the project name is AFAIK "Oracle BerkeleyDB" and the package is libdb (since the whole project is embedded DB/library).

However as I stated, the naming scheme is not clear, and since I am not aware of any official guideline, it could be renamed back to "lmdb".

Personally, I would stick with liblmdb, but if Howard have any feeling about the naming, I will adapt to them.

(In reply to Jan Staněk from comment #7)
> However as I stated, the naming scheme is not clear, and since I am not
> aware of any official guideline, it could be renamed back to "lmdb".

Just expressing my bigger sympathy for lmdb, which better aligns with other *dbm packages like qdbm, gdbm, ..

Feel free to align with Debian :)

http://packages.qa.debian.org/l/lmdb.html

Also I would suggest that we standardize on library SOVERSION.

http://anonscm.debian.org/gitweb/?p=pkg-db/lmdb.git;a=tree;f=debian/patches

(In reply to Ondřej Surý from comment #9)
> Also I would suggest that we standardize on library SOVERSION.
> 
> http://anonscm.debian.org/gitweb/?p=pkg-db/lmdb.git;a=tree;f=debian/patches

Good idea, thanks. openSUSE also uses liblmdb.so.0.0.0 as a SONAME and their srpm is available here:
http://software.opensuse.org/package/lmdb

The same srpm with only some minor changes to build fine on Fedora 20 available here (may be used for the Fedora package or not):
http://hhorak.fedorapeople.org/lmdb-0.9.11-7.1.src.rpm

OpenSUSE uses own downstream CMakeLists.txt, but I haven't looked much into details if it has some advantages over the Makefile.

>Also I would suggest that we standardize on library SOVERSION.

Howard Chu has mentioned to me in 1-to-1 conversation that "There will be no incompatible API changes.", which is why there is no SOVERSION assigned.

>OpenSUSE uses own downstream CMakeLists.txt, but I haven't looked much into details if it has some advantages over the Makefile.

Not really, especially not with its filesize which is more than 4x the size needed for the plain Makefile or an automake-enhanced lmdb.

Assigning myself as a reviewer, so we can move forward.

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

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


Issues:
=======
- As already mentioned above, package name should correspond with the
  project name, which is lmdb, not liblmdb.
- As mentioned in comment #9, syncing with Debian/OpenSUSE library
  versioning seems like a good idea to me. It seems the other distros
  use liblmdb.so.0.0.0, which is what we should use as well then.
- Binaries should be detached from the library file, since for proper library
  dependency only the library is necessary and the binaries are not.
  This may be also significant on multilib systems, in case there is some
  non-ELF file in the /usr/bin in the future.
  So I guess we should be prepared for that.
  This can be solved by introducing lmdb-libs subpackage, that would include
  only the library (and necessary doc -- license, ...)
- Generated documentation can introduce file conflicts on multilib systems,
  which means 32bit and 64bit -devel packages could not be co-installable.
  Therefore the generated doc may be moved to a separate package and made
  noarch.
  That would also solve the next issue:
- Large data in /usr/share should live in a noarch subpackage if package is arched.
  Note: Arch-ed rpms have a total of 2181120 bytes in /usr/share
- The macro %{version} should be changed to %%{version} in the comment in the spec file
- The following lines seem to be not necessary to me in the %install section,
  since they only remove files from the build directory:
    rm -f Doxyfile
    rm -rf man # Doxygen generated manpages

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "ISC", "Unknown or generated". 13 files have unknown license. Detailed
     output of licensecheck in /home/hhorak/tmp/lmdb/liblmdb/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.

- However, as already mentioned above, imho package name should correspond with the
  project name, which is lmdb, not liblmdb.

[x]: Package does not generate any conflict.

- However, generated documentation can introduce file conflicts on
  multilib systems, which means 32bit and 64bit -devel packages could
  not be co-installable. Therefore the generated doc may be moved to
  a separate package and made noarch.

[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[!]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

- The macro %{version} should be changed to %%{version} in the comment.
- Issue with Source0 is justified

[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 is included in %doc.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: Patches link to upstream bugs/comments/lists or are otherwise justified.

- All changes to Makefile done by patches seem to be generic enough to be
  accepted upstream, so it makes sense to report bugs for them and mention
  the links in the patches.

[x]: Scriptlets must be sane, if used.
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[!]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
     Note: Arch-ed rpms have a total of 2181120 bytes in /usr/share

- As mentioned above, moving the docs to a separate package is also good idea to make the package multilib clean.

[!]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.

- The macro %{version} should be changed to %%{version} in the comment.
- Issue with Source0 is justified

Rpmlint
-------
Checking: liblmdb-0.9.11-1.fc20.x86_64.rpm
          liblmdb-devel-0.9.11-1.fc20.x86_64.rpm
          liblmdb-0.9.11-1.fc20.src.rpm
liblmdb.src:9: W: macro-in-comment %{version}
liblmdb.src: W: invalid-url Source0: liblmdb-0.9.11.tar.gz
3 packages and 0 specfiles checked; 0 errors, 2 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint liblmdb liblmdb-devel
2 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'



Requires
--------
liblmdb (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libc.so.6()(64bit)
    liblmdb.so.0.1()(64bit)
    libpthread.so.0()(64bit)
    rtld(GNU_HASH)

liblmdb-devel (rpmlib, GLIBC filtered):
    liblmdb(x86-64)
    liblmdb.so.0.1()(64bit)



Provides
--------
liblmdb:
    liblmdb
    liblmdb(x86-64)
    liblmdb.so.0.1()(64bit)

liblmdb-devel:
    liblmdb-devel
    liblmdb-devel(x86-64)

(In reply to Honza Horak from comment #13)
>   This can be solved by introducing lmdb-libs subpackage, that would include
>   only the library (and necessary doc -- license, ...)

Maybe lmdb for binaries and liblmdb for libraries? That is like acl package
is doing. That might also make above liblmdb folks happy? Just a suggestion.
Personally I am also fine with lmdb-libs or similar :)

Updated package:
 Spec URL: http://jstanek.fedorapeople.org/lmdb/lmdb.spec
 SRPM URL: http://jstanek.fedorapeople.org/lmdb/lmdb-0.9.11-1.fc20.src.rpm

> - As already mentioned above, package name should correspond with the
>  project name, which is lmdb, not liblmdb.
Package renamed back to lmdb.

> - As mentioned in comment #9, syncing with Debian/OpenSUSE library
>   versioning seems like a good idea to me. It seems the other distros
>   use liblmdb.so.0.0.0, which is what we should use as well then.
Went with that idea -- the full name (filename and soname flag) is now liblmdb.so.0.0.0

> - Binaries should be detached from the library file, since for proper library
>   dependency only the library is necessary and the binaries are not.
>   This may be also significant on multilib systems, in case there is some
>   non-ELF file in the /usr/bin in the future.
>   So I guess we should be prepared for that.
>   This can be solved by introducing lmdb-libs subpackage, that would include
>   only the library (and necessary doc -- license, ...)
Created subpackage -libs, which now contains the shared library and the %doc files from main package (COPYRIGHT, CHANGES and LICENSE). I figured that since this subpackage does not need the main package, but main package is dependent on it, the %doc files should be in the subpackage. This way they are allways installed when the library is. However rpmlint does not likes that and issues a warning about no documentation in the main package.

> - Generated documentation can introduce file conflicts on multilib systems,
>   which means 32bit and 64bit -devel packages could not be co-installable.
>   Therefore the generated doc may be moved to a separate package and made
>   noarch.
>   That would also solve the next issue:
> - Large data in /usr/share should live in a noarch subpackage if package is arched.
>   Note: Arch-ed rpms have a total of 2181120 bytes in /usr/share
Moved the doxygen generated documentation to separate -doc subpackage. I was not able to find any guidelines about Requires: for this kind of package, so right now it is standalone - it requires no other lmdb* package and it is not required by any of them. Is that OK?

> - The macro %{version} should be changed to %%{version} in the comment in the spec file
Corrected.

> - The following lines seem to be not necessary to me in the %install section,
>   since they only remove files from the build directory:
>     rm -f Doxyfile
>     rm -rf man # Doxygen generated manpages
Moved the mentioned lines at the end of the %build section. Their role is to silence the rpm warnings regarding unpackaged files.

(In reply to Jan Staněk from comment #15)
> Moved the doxygen generated documentation to separate -doc subpackage. I was
> not able to find any guidelines about Requires: for this kind of package, so
> right now it is standalone - it requires no other lmdb* package and it is
> not required by any of them. Is that OK?

I think it is. Only the files COPYRIGHT, CHANGES and LICENSE may not be installed then. So I'd rather add them also for the -doc subpackage.

Otherwise it looks fine, so giving ACK, but, please, add the files to the -doc before commiting to git. Thanks.

New Package SCM Request
=======================
Package Name: lmdb
Short Description: Memory-mapped key-value database
Upstream URL: http://symas.com/mdb/
Owners: jstanek
Branches: f20
InitialCC: 

(In reply to Honza Horak from comment #16)
... snap ...
> 
> I think it is. Only the files COPYRIGHT, CHANGES and LICENSE may not be
> installed then. So I'd rather add them also for the -doc subpackage.
> 
> Otherwise it looks fine, so giving ACK, but, please, add the files to the
> -doc before commiting to git. Thanks.

OK, added the files to the -doc subpackage, now they are contained as %doc in -libs AND -doc subpackages.

Git done (by process-git-requests).

Package imported and successfully built for rawhide and Fedora 20.

lmdb-0.9.11-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/lmdb-0.9.11-1.fc20

lmdb-0.9.11-1.fc20 has been pushed to the Fedora 20 stable repository.

Package Change Request
======================
Package Name: lmdb
New Branches: el6 epel7
Owners: jvcelak

Comments from the primary maintainers?

We discussed this with jvcelak previously, I'm OK with this request.

Jan, thank you for an "official" approval.

Git done (by process-git-requests).