Bug 995974 - Review Request: libbson - BSON library for C
Review Request: libbson - BSON library for C
Status: CLOSED DUPLICATE of bug 1208101
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-12 02:01 EDT by Mike Manilone
Modified: 2015-10-07 07:19 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-03-13 20:22:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Mike Manilone 2013-08-12 02:01:19 EDT
Spec URL: http://people.midymidy.com/~ekd123/RPM/libbson.spec
SRPM URL: http://people.midymidy.com/~ekd123/RPM/libbson-0.1.10-1.fc19.src.rpm
Description: 

libbson is a library providing useful routines related to 
building, parsing, and iterating BSON documents. It is a 
useful base for those wanting to write high-performance 
C extensions to higher level languages such as Python, 
Ruby, or Perl.

Fedora Account System Username: unixekd123

Additional information: There's no stable release of libbson yet. And the compilation failed in 32bit mock. Perhaps I need to disable the -Werror option.
Comment 1 Christopher Meng 2013-08-12 02:07:11 EDT
Hi,

Fedora doesn't welcome unstable things, and if you cannot build it on i686, you don't need to go ahead ;)
Comment 2 Mike Manilone 2013-08-12 02:11:07 EDT
Yeah, but this review request could last until the library finally releases (that's what I expect).

I'm trying to figure out why the compilation failed and patch the package soon.
Comment 3 Joshua Small 2013-08-12 02:31:08 EDT
-Werror is usually there for a reason - I'd be concerned about simply disabling it in order to get a package built.

There may be an upstream error I could potentially help with - can you run a koji build and link it? As someone without a 32-bit system handy, this is the easiest way to let us see the specific error.
Comment 4 Ralf Corsepius 2013-08-12 03:00:58 EDT
(In reply to Joshua Small from comment #3)
> -Werror is usually there for a reason - I'd be concerned about simply
> disabling it in order to get a package built.
-Werror raises warnings to errors - Even neglible ones.

That said, using -Werror has its justifications during development, but is very tedious in production release SW.

It's even more tedious when taking longer time frames into account, because the set of warnings GCC warns about changes over time and because other factors (e.g. CFLAGS, other packages, etc.) influence the warnings.

In other words, IMO, using -Werror in production SW is naive and non-helpful.
Comment 5 Mike Manilone 2013-08-12 03:43:33 EDT
This happens in static assertions, so after compiling this library, these assertions are useless. We can safely disable -Werror.
Comment 6 Mike Manilone 2013-08-12 04:23:36 EDT
OK... I've found out the reason. A patch has been sent to the upstream. The following src.rpm also includes it.

Spec URL: http://people.midymidy.com/~ekd123/RPM/libbson.spec
SRPM URL: http://people.midymidy.com/~ekd123/RPM/libbson-0.1.10-2.fc19.src.rpm
Patch: http://people.midymidy.com/~ekd123/RPM/libbson.noerror.patch
Comment 7 Joshua Small 2013-08-12 06:51:27 EDT
To clarify my point..

The upstream has created a product which, by default, ships with -Werror. Whether this is unhelpful is an issue for upstream.

If the product fails to even compile in a basic environment, this is an issue far better addressed upstream (and one I'd imagine they'd be keen to see fixed) than patched out at packaging time by disabling the error.

As far as review goes:

Obviously, the blocker here is waiting until it's stable.

I'm assuming that once a "stable" version is declared, you'll match the version number and source0 accordingly.

The patch0 address in the .spec file just 404's on me - although I'm expecting this to be redundant once this patch is incorporated.

This line:
rm -rf $RPM_BUILD_ROOT
Is obsolete and should no longer be used. See: http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

rpmlint hits up a few errors worth reviewing:
./libbson.spec:65: W: libdir-macro-in-noarch-package devel %{_libdir}/*.so
Several of the above errors. They make sense - if a file is noarch, would it really belong in /usr/lib64, as per /usr/lib64/libbson-1.0.so?

libbson.x86_64: E: zero-length /usr/share/doc/libbson-0.1.10/NEWS
Get rid of this file if it doesn't ever have "news".

libbson-python.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/cbson.so.0.0.0 cbson.so.0()(64bit)

There's some documentation on this error here:
http://fedoraproject.org/wiki/Common_Rpmlint_issues#private-shared-object-provides

However, I'm unsure if your intention should involve filtering the library out, or deliberately providing it.

I've run a koji build so others can review your build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5807541

You can review there full output for this error:
 FAILED: BuildError: mismatch when analyzing libbson-devel-0.1.10-2.fc19.noarch.rpm

The errors shown may be related to the no arch issue above.
Comment 8 Joshua Small 2013-08-12 07:23:07 EDT
Additional:
Your script runs autogen.sh, followed by ./configure. 
The second line of autogen.sh is "./configure", so something is redundant here.

Reading the documentation, once a tarball (stable) is developed, autogen.sh won't be required anyway.

The application has a "make test" that should probably be used in a %check process.

The README states:
In your source code:
#include <bson.h>

However, every header file seems to suggest you should instead #include <bson/bson.h>. Your current -devel package places these files in <libbson-1.0/bson.h>

On a note unrelated to packaging, new applications should not be using MD5.
Wikipedia states: http://en.wikipedia.org/wiki/MD5 "should be considered cryptographically broken and unsuitable for further use"
Comment 9 Ralf Corsepius 2013-08-12 07:37:31 EDT
(In reply to Joshua Small from comment #7)
> To clarify my point..
> 
> The upstream has created a product which, by default, ships with -Werror.
> Whether this is unhelpful is an issue for upstream.
You're in error: Being a fedora packager, this will be your issue.

What will happen is: With many changes in Fedora (e.g GCC, new targets, new libs) your package will break building for cosmetical, negibile and stylish issues - It will be you to handle these issues.

That said, in many cases it's not much of a problem to provoke arbitrary numbers of such issues ... e.g. by adding further cflags - That said, I can only repeat what I said in different wording: It's evidence of lack upstream developent experience to ship production SW with -Werror. Packaging history is full of packages having gone this road.
Comment 10 Michael Schwendt 2013-08-12 08:01:15 EDT
Drive-by comments...


> %build
> ./autogen.sh

Check out the contents of that tiny script. Prefer using autoreconf directly, so you can skip the duplicate and wrong configure invocation.


> %package        devel
> BuildArch:      noarch

Making it "noarch" is a fundamental packaging mistake, since it's a compiled arch-specific library, and %_libdir is not /usr/lib for some target platforms.


> %package        python
> Requires:       %{name}%{?_isa} = %{version}-%{release} python >= 2.7

Easy to miss when multiple deps are on one line. 

There's an automatic dependency for python(abi) already. The explicit one on "python >= 2.7" is wrong.  http://fedoraproject.org/wiki/Packaging:Python


> %{_libdir}/pkgconfig/%{name}-1.0.pc

It contains a hardcoded  libdir=${exec_prefix}/lib  which is wrong for build targets where %_libdir is not /usr/lib but /usr/lib64.


> $ ls tests
> binary		  test-bson-clock.c  test-bson-oid.c	 test-bson-writer.c
> bson-tests.h	  test-bson-error.c  test-bson-reader.c  test_cbson.py
> Makefile.include  test-bson-iter.c   test-bson-string.c
> test-bson.c	  test-bson-json.c   test-bson-utf8.c

Is this suitable for a %check section?
Comment 11 Mike Manilone 2013-08-12 08:59:36 EDT
Ah... So many problems :-)

One more thing, "/usr/lib64/python2.7/site-packages/cbson.so.0.0.0 cbson.so.0" are the core extension and should not be considered "private".

Spec URL: http://people.midymidy.com/~ekd123/RPM/libbson.spec
SRPM URL: http://people.midymidy.com/~ekd123/RPM/libbson-0.1.10-3.fc19.src.rpm
Patches: 
http://people.midymidy.com/~ekd123/RPM/libbson.noerror.patch
http://people.midymidy.com/~ekd123/RPM/libbson.AvoidHardcodedLibdir.patch
http://people.midymidy.com/~ekd123/RPM/libbson.autogen.patch
Comment 12 Mike Manilone 2013-08-12 09:30:02 EDT
(In reply to Joshua Small from comment #7)
> However, I'm unsure if your intention should involve filtering the library
> out, or deliberately providing it.

This libraray works well in *my* daily work so I package it... Just no official releases, though the version has grown to 0.1.10. 
According to the Fedora guidelines, perhaps I need to wait until an official release comes out.
Comment 13 Mike Manilone 2013-08-12 10:34:34 EDT
The upstream has accepted my patches. Now my package contains no patches =)

Spec URL: http://people.midymidy.com/~ekd123/RPM/libbson.spec
SRPM URL: http://people.midymidy.com/~ekd123/RPM/libbson-0.1.10-4.fc19.src.rpm
Comment 14 Michael Schwendt 2013-08-12 10:43:27 EDT
> "/usr/lib64/python2.7/site-packages/cbson.so.0.0.0 cbson.so.0"
> are the core extension and should not be considered "private".

It _is_ a "private" shared library, because it is not stored in run-time linker's search path, and the automatic Provides/Requires for the lib SONAME will not be used by external packages. See bug 994474 comment 8 for a longer explanation.
Comment 15 Mike Manilone 2013-08-12 11:05:15 EDT
OK. They are now filtered out.

Spec URL: http://people.midymidy.com/~ekd123/RPM/libbson.spec
SRPM URL: http://people.midymidy.com/~ekd123/RPM/libbson-0.1.10-5.fc19.src.rpm
Comment 16 Michael Schwendt 2013-08-12 11:59:01 EDT
It's getting better.

Any response to the bottom of comment 10?

$ make test
/bson/new                                  : PASS : 0.000046
/bson/init                                 : PASS : 0.000033
/bson/init_static                          : PASS : 0.000001
/bson/basic                                : PASS : 0.000005
...
...
...
/bson/writer/shared_buffer                 : PASS : 0.253255
/bson/writer/empty_sequence                : PASS : 0.000011
Traceback (most recent call last):
  File "./tests/test_cbson.py", line 21, in <module>
    import bson
ImportError: No module named bson
make: *** [test] Error 1
Comment 17 Michael Schwendt 2013-08-12 12:38:16 EDT
The remainder of the test-suite passes after installing "python-bson" as BuildRequires.
Comment 18 Mike Manilone 2013-08-12 13:13:03 EDT
Should I add a %check section?
Comment 19 Christopher Meng 2013-08-12 20:06:50 EDT
If it has, you should add, otherwise you don't need to do that.
Comment 21 Mike Manilone 2013-08-13 01:22:10 EDT
Updated spec file as what the upstream suggested.

Spec URL: http://people.midymidy.com/~ekd123/RPM/libbson.spec
SRPM URL: http://people.midymidy.com/~ekd123/RPM/libbson-0.1.10.7.fc19.src.rpm
Comment 22 Michael Schwendt 2013-08-13 05:36:44 EDT
Make that:

Spec URL: http://people.midymidy.com/~ekd123/RPM/libbson.spec
SRPM URL: http://people.midymidy.com/~ekd123/RPM/libbson-0.1.10-7.fc19.src.rpm

Have you tried a scratch-build in the Fedora build system (koji) before?

  https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Install_the_client_tools_.28Koji.29_and_set_up_your_certificate

You could also give the "fedora-review -b 995974" command a try.
Comment 23 Mike Manilone 2013-08-13 06:12:43 EDT
(In reply to Michael Schwendt from comment #22)
> Have you tried a scratch-build in the Fedora build system (koji) before?
> 
> You could also give the "fedora-review -b 995974" command a try.

Both work great! =-)
Comment 24 Michael Schwendt 2013-08-13 06:44:24 EDT
> Both work great! =-)

Not yet. Could you verify that the spec file at "Spec URL" is exactly the same as in the src.rpm at "SRPM URL"? Currently, the differ, and the fedora-review tool fails because of that.
Comment 25 Mike Manilone 2013-08-13 06:59:42 EDT
Oops! I forgot to regenerate a src.rpm. (fedora-review worked here, I don't know why.)

Now they are really the same.
Comment 26 Michael Schwendt 2013-08-13 07:11:55 EDT
> fedora-review worked here

What did it tell about the package?
Comment 27 Mike Manilone 2013-08-13 08:29:34 EDT
Sorry, I gave the wrong result (or excuse). It did work but didn't succeed. It stopped at "make test" and complained about "no package named bson", but this is not expected - I don't have python-bson installed. And the current src.rpm failed at the same step. I didn't really see complaints about that. This is probably checked in the end?

On the other hand, Koji build succeeded. Link: http://koji.fedoraproject.org/koji/taskinfo?taskID=5811072
Comment 28 Michael Schwendt 2013-08-13 09:37:13 EDT
See comment 16 and comment 17 and the bottom of file "./tests/Makefile.include". The Python related tests are conditional and are only executed when HAVE_PYTHON is defined. In the clean koji buildroot, "python" is not installed unless your BuildRequires add it.
Comment 29 Mike Manilone 2013-08-27 22:50:08 EDT
Finally libbson has the first release 0.2.0 [1]. This is a good start for me. :-)

Spec URL: http://people.midymidy.com/~ekd123/RPM/libbson.spec
SRPM URL: http://people.midymidy.com/~ekd123/RPM/libbson-0.2.0-1.fc19.src.rpm

[1] https://github.com/mongodb/libbson/releases/tag/0.2.0
Comment 30 Ralf Corsepius 2013-08-27 23:56:51 EDT
MUSTFIX: Package fails to build on rawhide:

RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/share/doc/libbson-0.2.0/COPYING
   /usr/share/doc/libbson-0.2.0/NEWS
   /usr/share/doc/libbson-0.2.0/README
Child return code was: 1

The cause for this seems to be 2 issues interacting:
* the package is installing these files to this location by itselfs. This collides with rpm's docdir handling in fedora > 20
One workaround would be to pass --docdir=%{_pkgdocdir} to %configure and to use %{_pkgdocdir} in %files.

* However, this packages does not honor --docdir correctly.



MUSTFIX: Makefile is silent:
...
  CC       libbson_1_0_la-bson.lo
  CC       libbson_1_0_la-bson-context.lo
  CC       libbson_1_0_la-bson-clock.lo 
  CC       libbson_1_0_la-bson-error.lo
...
Build.logs don't provide sufficient information to verify if the package compiles correctly.

=> replace --enable-silent-rules with --disable-silent-rules
Comment 31 Mike Manilone 2013-08-30 01:19:14 EDT
Hi Ralf,

Sorry for the late reply. Here's the updated package. The --docdir problem has been reported to the upstream.

Spec URL: http://people.midymidy.com/~ekd123/RPM/libbson.spec
SRPM URL: http://people.midymidy.com/~ekd123/RPM/libbson-0.2.0-2.fc19.src.rpm
Patch0 URL: http://people.midymidy.com/~ekd123/RPM/libbson.RespectDocdir.patch

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5872453
Comment 32 Michael Schwendt 2013-09-09 10:47:22 EDT
%{_pkgdocdir} is not defined for Fedora 19 and older [yet]:
http://fedoraproject.org/wiki/Changes/UnversionedDocdirs

You can add

  %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}

at the top of the spec file to define it, if you want builds for Fedora 19 and older to continue using a versioned docdir. It would be okay to make them use an unversioned docdir, too.


The added patch is fine, but not really needed. I would wait until it gets merged. So long it would be more convenient to simply _move_ the installed doc files.


> %doc COPYING README NEWS

This line is superfluous, because the same files installed by "make install". Instead of using this %doc magic to install these local files, add entries to the %files section for the already installed files:

  %install
  ...
  %if 0%{?fedora} > 19
  mv %{buildroot}%{_docdir}/%{name}-%{version} %{buildroot}%{_pkgdocdir}
  %fi

  %files
  %{_pkgdocdir}/

That also avoids the conflict between including local files via %doc and the installed files in %{_docdir}/%{name}-%{version}: https://fedorahosted.org/fpc/ticket/338


Alternatively, the old-school _tmp_docs hack would work as soon as --docdir=... works:

  %configure --docdir=$(pwd)/__tmp_docs

  %install
  ...
  rm -rf __tmp_docs ; mkdir __tmp_docs
  make install ...

  %files
  %doc __tmp_docs/*
Comment 33 Robin Lee 2013-11-13 01:09:30 EST
Version 0.2.4 is available:
https://github.com/mongodb/libbson/releases
Comment 34 Christopher Meng 2015-10-07 07:19:59 EDT

*** This bug has been marked as a duplicate of bug 1208101 ***

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