This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours

Bug 874105

Summary: Review Request: qpid-proton - Proton is a high performance, lightweight messaging library
Product: [Fedora] Fedora Reporter: Darryl L. Pierce <dpierce>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: notting, package-review, seb, smizrahi, tross
Target Milestone: ---Flags: mtasaka: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-12-30 19:57:20 EST Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:

Description Darryl L. Pierce 2012-11-07 09:01:30 EST
Spec URL: http://mcpierce.fedorapeople.org/rpms/qpid-proton.spec
SRPM URL: http://mcpierce.fedorapeople.org/rpms/qpid-proton-0.2-1.fc17.src.rpm
Description: Proton is a high performance, lightweight messaging library. It can be used in the widest range of messaging applications including brokers, client libraries, routers, bridges, proxies, and more. Proton is based on the AMQP 1.0 messaging standard. Using Proton it is trivial to integrate with the AMQP 1.0 ecosystem from any platform, environment, or language.
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4662498
Fedora Account System Username: mcpierce
Comment 1 Sébastien Boisvert 2012-11-07 22:32:15 EST
This is not a formal review as I am not sponsored yet.

$ rpmlint  qpid-proton-*rpm
qpid-proton.x86_64: W: shared-lib-calls-exit /usr/lib64/libqpid-proton.so.1.0.0 exit@GLIBC_2.2.5
qpid-proton.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libqpid-proton.so
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

Not sure why rpmlint says that %{_libdir}/libqpid-proton.so* should to in devel.

Builds fine with mock.

mock -r  fedora-17-x86_64 rebuild qpid-proton-0.2-1.fc17.src.rpm &> mock.qp

When I install qpid-proton and do "man proton", I get this in DESCRIPTION:

       /home/seb/rpmbuild/BUILDROOT/qpid-proton-0.2-1.fc17.x86_64/usr/bin/proton:  error while loading shared libraries: libqpid-proton.so.1: can‐
       not open shared object file: No such file or directory

Same with man proton-dump.
Comment 2 Darryl L. Pierce 2012-11-30 15:34:40 EST
Updated with a static manpage to fix the problems in the previous version:

Updated SPEC: http://mcpierce.fedorapeople.org/rpms/qpid-proton.spec
Updated SRPM: http://mcpierce.fedorapeople.org/rpms/qpid-proton-0.2-2.fc17.src.rpm
Comment 3 Mamoru TASAKA 2012-12-08 00:51:16 EST
Taking.

I would appreciate it if you would review my review requst bug 770534.
Comment 4 Mamoru TASAKA 2012-12-12 00:29:06 EST
For 0.2-2:

* Summary
  - "Proton is a " on summary is redundant. It is included in Name.

* BuildRequires
  - ruby-devel:
    Currently ruby related things are not included in generated
    binary rpm. Would you check if BR: ruby-devel is really needed?
    (Or is it possible to enable ruby binding?)

* Touching %buildroot in prep
  - "mkdir -p %{buildroot}%{_mandir}" in prep does nothing because
    %buildroot is always removed at the beginning of %install.

* Possibly unneeded commands
  - Is 'echo "_mandir==%{_mandir}"' in install really needed?

! %defattr (not a blocker)
  https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#File_Permissions
  - %defattr is now not explicitly needed.

* Dependency between subpackages
  https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Requiring_Base_Package
  - Usually dependency between main <-> sub-packages should be (Epoch)-Version-
    Release.arch specific. i.e. usually Requires: %{name}%{?_isa} = %{version}-%{release}
    is needed (also in -devel subpackage).

* Files in -devel packages
  https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Devel_Packages
  - Unversioned %{_libdir}/libqpid-proton.so should usually be in -devel
    subpackage, not in main package.

  - Would you check if %_includedir/proton/cproton.i is really needed?

* Directory ownership issue
  https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#File_and_Directory_Ownership
  - Please make it sure that directories created when installing
    binary rpms are correctly owned by the appropriate rpms.
    Currently the following directories are left unowned.

    %{proton_datadir}/
    %{proton_datadir}/docs/
    %{proton_datadir}/docs/api-c/
    %{proton_datadir}/docs/api-py/
    %{_includedir}/proton/
Comment 5 Darryl L. Pierce 2012-12-13 16:17:54 EST
(In reply to comment #4)
> For 0.2-2:
> 
> * Summary
>   - "Proton is a " on summary is redundant. It is included in Name.

Removed.

> * BuildRequires
>   - ruby-devel:
>     Currently ruby related things are not included in generated
>     binary rpm. Would you check if BR: ruby-devel is really needed?
>     (Or is it possible to enable ruby binding?)

We have a Ruby gem for Proton that will be proposed as a separate package.
 
> * Touching %buildroot in prep
>   - "mkdir -p %{buildroot}%{_mandir}" in prep does nothing because
>     %buildroot is always removed at the beginning of %install.

Removed.

> * Possibly unneeded commands
>   - Is 'echo "_mandir==%{_mandir}"' in install really needed?

No, that's an artifact from when I was writing the spec. I've removed it now.

> ! %defattr (not a blocker)
>  
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#File_Permissions
>   - %defattr is now not explicitly needed.
> 
> * Dependency between subpackages
>  
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Requiring_Base_Package
>   - Usually dependency between main <-> sub-packages should be
> (Epoch)-Version-
>     Release.arch specific. i.e. usually Requires: %{name}%{?_isa} =
> %{version}-%{release}
>     is needed (also in -devel subpackage).

Fixed. Thanks for catching that.

> * Files in -devel packages
>  
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Devel_Packages
>   - Unversioned %{_libdir}/libqpid-proton.so should usually be in -devel
>     subpackage, not in main package.

Fixed.
 
>   - Would you check if %_includedir/proton/cproton.i is really needed?

It is for anybody who wants to write language bindings for Proton. 

> * Directory ownership issue
>  
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#File_and_Directory_Ownership
>   - Please make it sure that directories created when installing
>     binary rpms are correctly owned by the appropriate rpms.
>     Currently the following directories are left unowned.
> 
>     %{proton_datadir}/
>     %{proton_datadir}/docs/
>     %{proton_datadir}/docs/api-c/
>     %{proton_datadir}/docs/api-py/
>     %{_includedir}/proton/

Should be done.

Updated SPEC: http://mcpierce.fedorapeople.org/rpms/qpid-proton.spec
Updated SRPM: http://mcpierce.fedorapeople.org/rpms/qpid-proton-0.2-2.1.fc17.src.rpm
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4788015
Comment 6 Mamoru TASAKA 2012-12-16 08:01:12 EST
For -2.1:

* Versioning
  - Please don't use ".1" for release unless needed.
    Please use just integer (and %{?dist})
    c.f.
    https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning

* Dependency between subpackage.
  - As said before, usually dependency between subpackage must be
     _full_ (Epoch)-Version-"Release" and isa specific (i.e.
    Requires: %{name}%{?_isa} = %{version}-%{release} )

* Directory ownership issue
  - %{_includedir} itself is already owned by filesystem and
    should not be owned by qpid-proton-devel.

* Enabling test suite
  - I missed this before, however as the source tarball
    contains tests/ directory, please do some tests
    in %check if possible.
Comment 7 Darryl L. Pierce 2012-12-17 08:57:32 EST
(In reply to comment #6)
> For -2.1:
> 
> * Versioning
>   - Please don't use ".1" for release unless needed.
>     Please use just integer (and %{?dist})
>     c.f.
>    
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning

I'm following the pattern lower down where point releases are mentioned.

> * Dependency between subpackage.
>   - As said before, usually dependency between subpackage must be
>      _full_ (Epoch)-Version-"Release" and isa specific (i.e.
>     Requires: %{name}%{?_isa} = %{version}-%{release} )

Done.

> * Directory ownership issue
>   - %{_includedir} itself is already owned by filesystem and
>     should not be owned by qpid-proton-devel.

Oops, done.

> * Enabling test suite
>   - I missed this before, however as the source tarball
>     contains tests/ directory, please do some tests
>     in %check if possible.

The tests require, ATM, a little bit of setup in order to run them. I'll take upstream a request to make tests easier to run in the packaging environment

Update SPEC:   http://mcpierce.fedorapeople.org/rpms/qpid-proton.spec
Updated SRPM:  http://mcpierce.fedorapeople.org/rpms/qpid-proton-0.2-2.2.fc17.src.rpm
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4796610
Comment 8 Mamoru TASAKA 2012-12-18 23:11:15 EST
(In reply to comment #7)
> > * Dependency between subpackage.
> >   - As said before, usually dependency between subpackage must be
> >      _full_ (Epoch)-Version-"Release" and isa specific (i.e.
> >     Requires: %{name}%{?_isa} = %{version}-%{release} )
> 
> Done.

So would you explain why you omit "release" dependency?
(i.e. not %{name}%{?_isa} = %{version}-%{release} but
 %{name}%{?_isa} = %{version}))
Omitting %release allows user to update only -devel or some other
subpackage while not updating main package, which may result in 
inconsistent state or malfunction (especially when both main
package and subpackages are changed between release).

Another point I missed before:
* %filter_setup
 - %filter_setup is used, however no filters are actually defined,
   so this %filter_setup should not be needed.
Comment 9 Darryl L. Pierce 2012-12-19 07:50:38 EST
(In reply to comment #8)
> (In reply to comment #7)
> > > * Dependency between subpackage.
> > >   - As said before, usually dependency between subpackage must be
> > >      _full_ (Epoch)-Version-"Release" and isa specific (i.e.
> > >     Requires: %{name}%{?_isa} = %{version}-%{release} )
> > 
> > Done.
> 
> So would you explain why you omit "release" dependency?
> (i.e. not %{name}%{?_isa} = %{version}-%{release} but
>  %{name}%{?_isa} = %{version}))
> Omitting %release allows user to update only -devel or some other
> subpackage while not updating main package, which may result in 
> inconsistent state or malfunction (especially when both main
> package and subpackages are changed between release).

A typo, I thought I had included the -%{version} portion.

> 
> Another point I missed before:
> * %filter_setup
>  - %filter_setup is used, however no filters are actually defined,
>    so this %filter_setup should not be needed.

Removed.

Updated SPEC:  http://mcpierce.fedorapeople.org/rpms/qpid-proton.spec
Updated SRPM:  http://mcpierce.fedorapeople.org/rpms/qpid-proton-0.2-2.3.fc17.src.rpm
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4802573
Comment 10 Mamoru TASAKA 2012-12-24 08:20:43 EST
One remaining issue
* Directory ownership issue
  - As the directory %{proton_datadir}/docs/ is owned by the main
    package and python-qpid-proton-doc uses this directory, please
    make python-qpid-proton-doc also have 
    "Requires: %{name}%{?_isa} = %{version}-%{release}"

---------------------------------------------------------
   This package (qpid-proton) is APPROVED by mtasaka
---------------------------------------------------------
Comment 11 Darryl L. Pierce 2012-12-28 10:27:24 EST
(In reply to comment #10)
> One remaining issue
> * Directory ownership issue
>   - As the directory %{proton_datadir}/docs/ is owned by the main
>     package and python-qpid-proton-doc uses this directory, please
>     make python-qpid-proton-doc also have 
>     "Requires: %{name}%{?_isa} = %{version}-%{release}"
> 
> ---------------------------------------------------------
>    This package (qpid-proton) is APPROVED by mtasaka
> ---------------------------------------------------------

Fixed in what will be the first build.

New Package SCM Request
=======================
Package Name: qpid-proton
Short Description: A high performance, lightweight messaging library
Owners: mcpierce
Branches: f16 f17 f18
InitialCC: mcpierce
Comment 12 Kevin Fenzi 2012-12-30 15:18:02 EST
Git done (by process-git-requests).
Comment 13 Darryl L. Pierce 2012-12-30 19:57:20 EST
Thank you, all!
Comment 14 Darryl L. Pierce 2013-02-01 13:43:26 EST
Package Change Request
======================
Package Name: qpid-proton
New Branches: el6
Owners: mcpierce
Comment 15 Jon Ciesla 2013-02-01 14:29:42 EST
Git done (by process-git-requests).
Comment 16 Darryl L. Pierce 2013-03-11 16:15:23 EDT
Package Change Request
======================
Package Name: qpid-proton
New Branches: el5
Owners: mcpierce
Comment 17 Jon Ciesla 2013-03-12 14:30:46 EDT
Git done (by process-git-requests).
Comment 18 Darryl L. Pierce 2014-01-30 15:48:42 EST
Package Change Request
======================
Package Name: qpid-proton
New Branches: epel7
Owners: mcpierce
Comment 19 Jon Ciesla 2014-01-30 16:14:09 EST
Git done (by process-git-requests).