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
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.
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
Taking. I would appreciate it if you would review my review requst bug 770534.
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/
(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
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.
(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
(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.
(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
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 ---------------------------------------------------------
(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
Git done (by process-git-requests).
Thank you, all!
Package Change Request ====================== Package Name: qpid-proton New Branches: el6 Owners: mcpierce
Package Change Request ====================== Package Name: qpid-proton New Branches: el5 Owners: mcpierce
Package Change Request ====================== Package Name: qpid-proton New Branches: epel7 Owners: mcpierce