Bug 874105 - Review Request: qpid-proton - Proton is a high performance, lightweight messaging library
Summary: Review Request: qpid-proton - Proton is a high performance, lightweight messa...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-11-07 14:01 UTC by Darryl L. Pierce
Modified: 2015-06-22 00:08 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-12-31 00:57:20 UTC
Type: Bug
Embargoed:
mtasaka: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Darryl L. Pierce 2012-11-07 14:01:30 UTC
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-08 03:32:15 UTC
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.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 20:34:40 UTC
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 05:51:16 UTC
Taking.

I would appreciate it if you would review my review requst bug 770534.

Comment 4 Mamoru TASAKA 2012-12-12 05:29:06 UTC
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 21:17:54 UTC
(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 13:01:12 UTC
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 13:57:32 UTC
(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-19 04:11:15 UTC
(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 12:50:38 UTC
(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 13:20:43 UTC
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 15:27:24 UTC
(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 20:18:02 UTC
Git done (by process-git-requests).

Comment 13 Darryl L. Pierce 2012-12-31 00:57:20 UTC
Thank you, all!

Comment 14 Darryl L. Pierce 2013-02-01 18:43:26 UTC
Package Change Request
======================
Package Name: qpid-proton
New Branches: el6
Owners: mcpierce

Comment 15 Gwyn Ciesla 2013-02-01 19:29:42 UTC
Git done (by process-git-requests).

Comment 16 Darryl L. Pierce 2013-03-11 20:15:23 UTC
Package Change Request
======================
Package Name: qpid-proton
New Branches: el5
Owners: mcpierce

Comment 17 Gwyn Ciesla 2013-03-12 18:30:46 UTC
Git done (by process-git-requests).

Comment 18 Darryl L. Pierce 2014-01-30 20:48:42 UTC
Package Change Request
======================
Package Name: qpid-proton
New Branches: epel7
Owners: mcpierce

Comment 19 Gwyn Ciesla 2014-01-30 21:14:09 UTC
Git done (by process-git-requests).


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