Bug 1495985 - Review Request: libcouchbase - Client and protocol library for the Couchbase project
Summary: Review Request: libcouchbase - Client and protocol library for the Couchbase ...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Igor Gnatenko
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-09-26 13:34 UTC by Sergey Avseyev
Modified: 2017-11-13 13:36 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-11-13 13:36:59 UTC
ignatenko: fedora-review+


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Bugzilla 1497683 None None None Never

Internal Links: 1497683

Description Sergey Avseyev 2017-09-26 13:34:16 UTC
Spec URL: https://github.com/avsej/libcouchbase.spec/raw/9e3d48045bfa29ed7560fe2b47783ee496ab8f7a/libcouchbase.spec
SRPM URL: https://github.com/avsej/libcouchbase.spec/raw/9e3d48045bfa29ed7560fe2b47783ee496ab8f7a/libcouchbase-2.8.1-1.fc26.src.rpm
Description: The client and protocol library for Couchbase project
Fedora Account System Username: avsej

Comment 1 Sergey Avseyev 2017-09-26 13:38:07 UTC
This is my first package in Fedora. I'm also upstream maintainer of libcouchbase at Couchbase company.

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22085478

Comment 5 Sergey Avseyev 2017-09-26 17:02:15 UTC
Updated SRPM:
* enabled library tests in %check
* enforced crypto policies
* fixed typos and missing _isa macro for tools package
* Suggests/Recommends tags applied only for Fedora 21+

Spec URL: https://github.com/avsej/libcouchbase.spec/raw/873d08b6c255e4acc312d4ec17e1164e038ed684/libcouchbase.spec
SRPM URL: https://github.com/avsej/libcouchbase.spec/raw/master/libcouchbase-2.8.1-1.fc28.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22087562

Comment 7 Igor Gnatenko 2017-09-26 19:33:40 UTC
Hello,

unfortunately I can't sponsor you, but I can give you some advises..

---

> %if 0%{?fedora} >= 21
> Recommends: %{name}-libevent%{_isa} = %{version}-%{release}
> Suggests: %{name}-libev%{_isa} = %{version}-%{release}
> Suggests: %{name}-tools%{_isa} = %{version}-%{release}
> Patch0: f21-enforce-system-crypto-policies.patch
> %endif
Here are 2 issues:
1. You always should include Patch0, regardless whether you will apply it or not
2. The conditional doesn't make sense because F21 is EOL for long time, so I would recommend you to remove this conditional at all

> %setup -q -n %{name}-%{version}
Note that "-n %{name}-%{version}" is default for %*setup macro, so you can safely remove this. Also I would recommend you to change whole line to "%autosetup" which also would apply patches automatically if you have them (P.S. if you will have problems with applying patches, you might want to add "-p1" after), right now patch is not applied at all

> make %{_smp_mflags} V=1
You could replace (and I recommend to) this with %make_build (I don't think that V=1 is also needed because %cmake macro automatically adds -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON to cmake invocation which makes make verbose)

> make install DESTDIR="%{buildroot}" AM_INSTALL_PROGRAM_FLAGS=""
You could replace (and I recommend to) this with "%make_install" which does absolutely same, not sure about AM_INSTALL_* thing though, but you still can pass it to %make_install

> -DLCB_BUILD_LIBUV=OFF
Why not to build libuv part as well? We have it in Fedora..

> %post -n %{name} -p /sbin/ldconfig
> %postun -n %{name} -p /sbin/ldconfig
Consider dropping "-n %{name}" since it is default.

---

You miss BuildRequires: gcc and BuildRequires: gcc-c++. You have to list them since your package needs them to build.

And you probably want to add soname for libev / libevent libs, not sure how it is supposed to work though (see https://fedoraproject.org/wiki/Packaging:Guidelines#Downstream_.so_name_versioning).

===

Hope it is useful =)

Comment 8 Sergey Avseyev 2017-09-26 20:19:07 UTC
Thank you, Igor. Your notes are very useful.

I applied almost all of them. 

> 2. The conditional doesn't make sense because F21 is EOL for long time, so I would recommend you to remove this conditional at all

I included this to reuse the same spec in future for EPEL, but I think it would be better to keep it clean for now.

>> -DLCB_BUILD_LIBUV=OFF
> Why not to build libuv part as well? We have it in Fedora..
We didn't do it previously in our builds, because in the past libuv was not common to be shipped as library, but I think it make sense. I will update spec to include libuv IO plugin too.

> And you probably want to add soname for libev / libevent libs, not sure how it is supposed to work though (see https://fedoraproject.org/wiki/Packaging:Guidelines#Downstream_.so_name_versioning).

I saw that issue in rpmlint, but currently we don't use versioning for the shared objects of the plugins, and check their versions in runtime. Ideally these plugins should not be placed in the libdir at all, and should be loaded from other place like %{_libdir}/%{name}/libevent.so. I plan to improve that in next major version in the upstream. If it is critical and blocks accepting package into Fedora, I think I can create patch to workaround it. Right now in upstream I'm doing something simple like:

  dlopen("libcouchbase_libevent.so", RTLD_NOW | RTLD_LOCAL);

Updated artifacts:

Spec URL: https://github.com/avsej/libcouchbase.spec/raw/a8c1b09ba9afeaea67c657ce85fc1cd06f5170b4/libcouchbase.spec
SRPM URL: https://github.com/avsej/libcouchbase.spec/raw/a8c1b09ba9afeaea67c657ce85fc1cd06f5170b4/libcouchbase-2.8.1-1.fc28.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22089176

In the next step I'm going to add package libuv IO plugin, and test if the plugins could be easily loaded from "%{_libdir}/%{name}/"

Comment 11 Vasiliy Glazov 2017-09-27 06:28:39 UTC
%make_install must be in %install section.

Comment 12 Vasiliy Glazov 2017-09-27 06:41:40 UTC
And %make_build must be in %build section.

Comment 13 Vasiliy Glazov 2017-09-27 06:50:19 UTC
Also check rpmlint error and warnings:
libcouchbase-tools.x86_64: E: explicit-lib-dependency libcouchbase-libevent(x86-64)

libcouchbase.x86_64: W: unstripped-binary-or-object /usr/lib64/libcouchbase.so.2.0.49
libcouchbase.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libcouchbase.so.2.0.49 SSL_CTX_set_cipher_list
libcouchbase.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libcouchbase.so.2.0.49 SSL_CTX_set_cipher_list

libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-admin.1
libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-bucket-create.1
libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-bucket-delete.1
libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-cat.1
libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-connstr.1

And so on.

Comment 14 Igor Gnatenko 2017-09-27 07:01:22 UTC
> %cmake -DLCB_NO_MOCK=1
> %make_build
Move this to %build section

> %make_install
And this to %install section

There seems to be one more bug.. In pkg-config file you have
> libdir=${prefix}//usr/lib64
Which is wrong =)

---

> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-admin.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-bucket-create.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-bucket-delete.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-cat.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-connstr.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-cp.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-create.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-dsn.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-flush.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-hash.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-lock.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-n1qlback.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-observe.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-pillowfight.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-ping.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-rm.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-role-list.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-stats.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-subdoc.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-unlock.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-user-delete.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-user-list.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-user-upsert.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-verbosity.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-version.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc-view.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man1/cbc.1
> libcouchbase-tools.x86_64: W: manpage-not-compressed gz /usr/share/man/man4/cbcrc.4

> libcouchbase-tools.x86_64: E: explicit-lib-dependency libcouchbase-libevent(x86-64)
Seems to be false-positive.. mind reporting bug to rpmlint?

> libcouchbase-tools.x86_64: W: manual-page-warning /usr/share/man/man1/cbc-pillowfight.1 266: warning: macro `..' not defined
> libcouchbase-tools.x86_64: W: manual-page-warning /usr/share/man/man1/cbc-subdoc.1 317: warning: macro `..' not defined
> libcouchbase-tools.x86_64: W: manual-page-warning /usr/share/man/man1/cbc.1 520: warning: macro `..' not defined
Not a blocker, but would be nice to fix this.

> libcouchbase-libuv.x86_64: W: unstripped-binary-or-object /usr/lib64/libcouchbase/libcouchbase_libuv.so
> libcouchbase-tools.x86_64: W: unstripped-binary-or-object /usr/bin/cbc
> libcouchbase-tools.x86_64: W: unstripped-binary-or-object /usr/bin/cbc-n1qlback
> libcouchbase-tools.x86_64: W: unstripped-binary-or-object /usr/bin/cbc-pillowfight
> libcouchbase-tools.x86_64: W: unstripped-binary-or-object /usr/bin/cbc-proxy
> libcouchbase-tools.x86_64: W: unstripped-binary-or-object /usr/bin/cbc-subdoc
> libcouchbase-libevent.x86_64: W: unstripped-binary-or-object /usr/lib64/libcouchbase/libcouchbase_libevent.so
> libcouchbase-libev.x86_64: W: unstripped-binary-or-object /usr/lib64/libcouchbase/libcouchbase_libev.so
> libcouchbase.x86_64: W: unstripped-binary-or-object /usr/lib64/libcouchbase.so.2.0.49
Seems to be an issue, although I'm not sure how to fix from first glance..

> libcouchbase.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libcouchbase.so.2.0.49 SSL_CTX_set_cipher_list
> libcouchbase.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libcouchbase.so.2.0.49 SSL_CTX_set_cipher_list
I guess you need to patch it more..

I think unstripped-binary and uncompressed manpage issues will go away as soon as you will create proper %build and %install sections..

===

Anyway, awesome work! Get this things fixed and I'm pretty sure you will get package in Fedora ;)

Comment 15 Sergey Avseyev 2017-09-27 07:42:08 UTC
Spec URL: https://github.com/avsej/libcouchbase.spec/raw/6fd9ddb87f822d5dfaf8ef12a350af8698d34df4/libcouchbase.spec
SRPM URL: https://github.com/avsej/libcouchbase.spec/raw/6fd9ddb87f822d5dfaf8ef12a350af8698d34df4/libcouchbase-2.8.1-1.fc28.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22094297


Here I restored %build and %install.

>> libcouchbase-tools.x86_64: W: manual-page-warning /usr/share/man/man1/cbc-pillowfight.1 266: warning: macro `..' not defined
>> libcouchbase-tools.x86_64: W: manual-page-warning /usr/share/man/man1/cbc-subdoc.1 317: warning: macro `..' not defined
>> libcouchbase-tools.x86_64: W: manual-page-warning /usr/share/man/man1/cbc.1 520: warning: macro `..' not defined
> Not a blocker, but would be nice to fix this.

I have fixed it upstream, next release will get dots escaped.

>> libcouchbase-tools.x86_64: E: explicit-lib-dependency libcouchbase-libevent(x86-64)
> Seems to be false-positive.. mind reporting bug to rpmlint?
I'm going to look at it more closely, and then report to rpmlint.


pkg-config and crypto policies I'm fixing right now, and update the ticket once I'll get them solved

Comment 16 Vasiliy Glazov 2017-09-27 07:44:57 UTC
I am think you just need remove Requires: %{name}-libevent%{?_isa}
Because this Requires may be processed automatically.

Comment 17 Sergey Avseyev 2017-09-27 07:47:31 UTC
> I am think you just need remove Requires: %{name}-libevent%{?_isa}
> Because this Requires may be processed automatically.

No, it will not. Because it loads the plugin in run-time using dlopen(3). This is dependency on plugin, not libevent library.

Comment 18 Sergey Avseyev 2017-09-27 08:07:49 UTC
> libcouchbase-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/libcouchbase-2.8.1/contrib/cJSON/cJSON.h

BTW this warning also fixed in upstream, we don't install this header anyway.

Comment 19 Vasiliy Glazov 2017-09-27 08:08:45 UTC
Than may be rename plugins subpackages to something not "lib..."?

Comment 20 Remi Collet 2017-09-27 08:18:00 UTC
> Than may be rename plugins subpackages to something not "lib..."?

I think package naming is ok, rpmint warning about explicit-lib-dependency is obviously a false positive

Comment 21 Remi Collet 2017-09-27 08:21:16 UTC
> The conditional doesn't make sense because F21 is EOL for long time, so I would recommend you to remove this conditional at all

The condition is still needed for EPEL, and F21 is the first Fedora version for weak dependency support. SO this condition is OK.

BTW, the Patch should always in source RPM (so outside the condition)


%{_libdir}/%{name}/ should be owned by the main packages

Comment 22 Igor Gnatenko 2017-09-27 08:23:37 UTC
(In reply to Remi Collet from comment #21)
> > The conditional doesn't make sense because F21 is EOL for long time, so I would recommend you to remove this conditional at all
> 
> The condition is still needed for EPEL, and F21 is the first Fedora version
> for weak dependency support. SO this condition is OK.
In that case, it should be `%if 0%{?rhel} && 0%{?rhel} <= 7`...
> 
> BTW, the Patch should always in source RPM (so outside the condition)
> 
> 
> %{_libdir}/%{name}/ should be owned by the main packages
Ah, missed this!

Comment 23 Sergey Avseyev 2017-09-27 08:43:44 UTC
> BTW, the Patch should always in source RPM (so outside the condition)
I put the patch into condition, becauses I was using autosetup, and https://fedoraproject.org/wiki/Packaging:CryptoPolicies says it is effective since Fedora 21. Shall I also use %autosetup -N and then apply patche with condition?

Comment 24 Remi Collet 2017-09-27 09:44:31 UTC
%autosetup is Fedora only.

Comment 25 Vasiliy Glazov 2017-09-27 09:46:43 UTC
(In reply to Remi Collet from comment #24)
> %autosetup is Fedora only.

No. %autosetup work for EPEL 6 and 7. It in epel-rpm-macros package used koji by default.

Comment 26 Remi Collet 2017-09-27 09:49:04 UTC
> No. %autosetup work for EPEL 6 and 7. It in epel-rpm-macros package used koji by default.

Sorry then.
Btw, I'm used to block this epel-rpm-macros which only work in mock/koji and break other builds, in brew (RH) or cbs (CentOS)

Comment 27 Sergey Avseyev 2017-09-27 10:04:27 UTC
Spec URL: https://github.com/avsej/libcouchbase.spec/raw/0f1c1ad17ea6730c85077f333e7fc351b05a45e2/libcouchbase.spec
SRPM URL: https://github.com/avsej/libcouchbase.spec/raw/0f1c1ad17ea6730c85077f333e7fc351b05a45e2/libcouchbase-2.8.1-1.fc28.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22096250

Here I've fixed pkgconfig and crypto policy (removed commented call).

Also I have guarded Recommends/Suggests with "0%{?fedora} >= 21" and left all patches to be applied.

Comment 28 Igor Gnatenko 2017-09-27 13:49:02 UTC
(In reply to Sergey Avseyev from comment #27)
> Also I have guarded Recommends/Suggests with "0%{?fedora} >= 21" and left
> all patches to be applied.

Please use `%if 0%{?rhel} && 0%{?rhel} <= 7` for this... Because those distros are only ones which don't support Recommends/Suggests..

Comment 29 Igor Gnatenko 2017-09-27 13:50:04 UTC
> %{_libdir}/%{name}
> %exclude %{_libdir}/%{name}/*
you can replace this with `%dir %{_libdir}/%{name}`

Comment 31 Sergey Avseyev 2017-09-27 17:42:25 UTC
Strange, rpmlint still complains on

  libcouchbase.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libcouchbase.so.2.0.49 SSL_CTX_set_cipher_list

Comment 32 Sergey Avseyev 2017-09-27 18:19:05 UTC
According to the https://fedoraproject.org/wiki/Packaging:CryptoPolicies, it should be enough to use "PROFILE=SYSTEM", but rpmlint still complains.

The library after patching has the only place, where I use SSL_CTX_set_cipher_list:

$ grep -Rni SSL_CTX_set_cipher_list rpmbuild/BUILD/libcouchbase-2.8.1
rpmbuild/BUILD/libcouchbase-2.8.1/src/ssl/ssl_common.c:280:    SSL_CTX_set_cipher_list(ret->ctx, "PROFILE=SYSTEM");

I would call it false-positive, as rpmlint does something primitive like this:

$ strings /usr/lib64/libcouchbase.so.2.0.49 | grep SSL_CTX_set_cipher_list
SSL_CTX_set_cipher_list

Which obviously does not actually check the arguments.

Comment 33 Igor Gnatenko 2017-09-29 15:39:20 UTC
(In reply to Sergey Avseyev from comment #32)
> According to the https://fedoraproject.org/wiki/Packaging:CryptoPolicies, it
> should be enough to use "PROFILE=SYSTEM", but rpmlint still complains.
> 
> The library after patching has the only place, where I use
> SSL_CTX_set_cipher_list:
> 
> $ grep -Rni SSL_CTX_set_cipher_list rpmbuild/BUILD/libcouchbase-2.8.1
> rpmbuild/BUILD/libcouchbase-2.8.1/src/ssl/ssl_common.c:280:   
> SSL_CTX_set_cipher_list(ret->ctx, "PROFILE=SYSTEM");
> 
> I would call it false-positive, as rpmlint does something primitive like
> this:
> 
> $ strings /usr/lib64/libcouchbase.so.2.0.49 | grep SSL_CTX_set_cipher_list
> SSL_CTX_set_cipher_list
> 
> Which obviously does not actually check the arguments.

Could you report this to rpmlint please?

Comment 34 Michael Schwendt 2017-10-02 11:24:16 UTC
> %package tools
> Summary: Couchbase client tools
> Requires: %{name}%{?_isa} = %{version}-%{release}
> Requires: %{name}-libevent%{?_isa}

You want to pull in the backend lib with exact %version-%release in the same way it is done with base packages.

Comment 35 Michael Schwendt 2017-10-02 11:28:12 UTC
> Review Request: libcouchbase - the driver for Couchbase Server 

> Summary: Couchbase Client & Protocol Library

> %description
> This is the client and protocol library for Couchbase project.

Please make sure package review ticket title summary and package %summary are the same. The uppercase spelling in %summary is odd, btw. Since %description is very brief, %summary could copy from that:

  Summary: Client and protocol library for the Couchbase project

Note the missing "the" in %description.

Comment 36 Sergey Avseyev 2017-10-02 13:05:34 UTC
(In reply to Igor Gnatenko from comment #33)
> Could you report this to rpmlint please?
Reported in https://bugzilla.redhat.com/show_bug.cgi?id=1497683

Comment 37 Sergey Avseyev 2017-10-02 13:28:00 UTC
(In reply to Michael Schwendt from comment #34)
> > %package tools
> > Summary: Couchbase client tools
> > Requires: %{name}%{?_isa} = %{version}-%{release}
> > Requires: %{name}-libevent%{?_isa}
> 
> You want to pull in the backend lib with exact %version-%release in the same
> way it is done with base packages.

Good catch, thank you

Spec URL: https://github.com/avsej/libcouchbase.spec/raw/76024db56cf8823f27d0de86ccfed7ef1f3bfae8/libcouchbase.spec
SRPM URL: https://github.com/avsej/libcouchbase.spec/raw/76024db56cf8823f27d0de86ccfed7ef1f3bfae8/libcouchbase-2.8.1-1.fc28.src.rpm
Koji build: https://bugzilla.redhat.com/show_bug.cgi?id=1497683

Comment 38 Sergey Avseyev 2017-10-02 19:35:33 UTC
(In reply to Remi Collet from comment #20)
> > Than may be rename plugins subpackages to something not "lib..."?
> 
> I think package naming is ok, rpmint warning about explicit-lib-dependency
> is obviously a false positive

BTW the previous update has fixed the following warning

  libcouchbase-tools.x86_64: E: explicit-lib-dependency libcouchbase-libevent(x86-64)

Now it is good

Comment 40 Gwyn Ciesla 2017-10-17 11:54:03 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/libcouchbase


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