Following the Package_Renaming_Process, I ack that this is a rename of the package.

Hi Nuno,

the new package introduces a lot of changes compared to what is in fedora-cvs qpidc/devel.

It would be a lot simpler if the srpm/spec file to review contains _only_ the changes related to the rename process otherwise every added change has to be re-reviewed.

The question I have for you (for eg.):

(from qpidc)
%package -n %{srv}-cluster

becomes:

(from the new one)
%package -n %{name}-server-cluster
Obsoletes: qpidd-cluster

but there is no Provides: entry.

I suggest for you to review: http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Renaming.2Freplacing_existing_packages

and if the entries in the new spec files are correct, then add a note that the new package does not fully replace the old one.

Thanks
Fabio

Fabio,

thank you for picking up the review -- much appreciated!

I've added the missing Provides: entries (the renamed package fully replaces the previous one) and uploaded the new versions of the specfile and SRPM to the same place.

Regarding the amount of changes, the rawhide version of qpidc hadn't been updated in a while because of the impending rename, but the F11 and F12 versions are identical to what is included in this rename.

Thank you,
Nuno

Fabio,

as you had requested earlier, I isolated the changes related to the package renaming and applied them to the sources and specfile currently present in F12.

The revised SRPM and specfile are available for your review at:

SRPM: http://people.redhat.com/~nsantos/qpid-cpp-0.5.829175-4.fc13.src.rpm
spec: http://people.redhat.com/~nsantos/qpid-cpp.spec

There are two patches to allow compilation under rawhide, they are being (or are already) integrated upstream.

Thanks,
Nuno

Hi Nuno,

we are getting very close but there are still some issues in the packaging.

I have compared the new spec file vs both fedora rawhide and fedora12.

It appears you have dropped a major patch from the new rpm (so_number.patch) and that also shows in the file list in the spec file (for eg.):

-%_libdir/libqmfconsole.so.3
-%_libdir/libqmfconsole.so.3.0.0
+%_libdir/libqmfconsole.so.2
+%_libdir/libqmfconsole.so.2.0.0

This creates a set of packages that are not really usable:

[root@fedora12-node2 ~]# rpm -U *.rpm
error: Failed dependencies:
        libqmfconsole.so.3()(64bit) is needed by (installed) fence-virtd-qpid-0.1.3-1.fc12.x86_64
        libqpidcommon.so.3()(64bit) is needed by (installed) rhm-cpp-server-store-0.5.829175-3.fc12.x86_64
        libqpidbroker.so.3()(64bit) is needed by (installed) rhm-cpp-server-store-0.5.829175-3.fc12.x86_64

So here is what I´d like to see:

1) get rawhide to have a version > than F-12 > F-11. this is a fundamental prerequisite for updates. So we also have one single reference point to compare.
2) fix the new spec file so that has a version > than rawhide
3) check again that the new packages can update from the old one

if rawhide needs extra patches to build, that is absolutely fine (and somewhat irrelevant for the review).

nice to see define -> global change

run an rpmlint over the newly generated srpm/rpm. There are few errors/warnings that are relevant to those changes:

qmf.x86_64: W: invalid-license ASL 2.0, LGPL 2.0

qpid-cpp.src:66: W: unversioned-explicit-obsoletes qpidc (a bunch of those)

qpid-cpp-client.x86_64: W: self-obsoletion qpidc obsoletes qpidc

qpid-cpp-client.x86_64: E: library-without-ldconfig-postin /usr/lib64/libqpidclient.so.2.0.0

ruby-qmf.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/ruby/site_ruby/1.8/x86_64-linux/qmfengine.so ['/builddir/build/BUILD/qpidc-0.5.829175/cpp/src/.libs']

most of the warnings/errors are very easy to fix.

Fabio

Fabio,

please find updated SRPM and specfile at the same URLs:

SRPM: http://people.redhat.com/~nsantos/qpid-cpp-0.5.829175-4.fc13.src.rpm
spec: http://people.redhat.com/~nsantos/qpid-cpp.spec


I put the patches back in, it's generating the correct version of .so now.

Regarding the version, this is 0.5.829175-4, which is higher than what was in rawhide before (qpidc-0.5.819819-1.fc13), and the same codebase but a revision higher than what's in F12 (qpidc-0.5.829175-3.fc12).

I fixed most of the warnings from rpmlint (except for "no-documentation", etc), but these errors are still present (explanations for each are inline):
* qpid-cpp-server.i686: E: non-readable /var/lib/qpidd/qpidd.sasldb 0600
  - this is supposed to non-readble
* qpid-cpp-server-store.i686: E: explicit-lib-dependency libaio
  - there is no explicit lib dependency, there is a requires for a package
    named libaio: "Requires: libaio" (line 281)
* qpid-cpp-server-store.i686: E: non-standard-dir-perm /var/rhm 0775
  - supposed to have those permissions
* ruby-qmf.i686: E: binary-or-shlib-defines-rpath /usr/lib/ruby/site_ruby/1.8/i386-linux/qmfengine.so ['/builddir/build/BUILD/qpidc-0.5.829175/cpp/src/.libs']
  - not sure why it's complaining, I'm using the standard ruby_sitelib and ruby_sitearch macros, not hardcoding any path

Nuno

(In reply to comment #6)
> Fabio,
> 
> please find updated SRPM and specfile at the same URLs:
> 
> SRPM: http://people.redhat.com/~nsantos/qpid-cpp-0.5.829175-4.fc13.src.rpm
> spec: http://people.redhat.com/~nsantos/qpid-cpp.spec
> 
> 
> I put the patches back in, it's generating the correct version of .so now.

Ok, the update/rename path looks good now and I was able to replace the old packages with no problems.

> 
> Regarding the version, this is 0.5.829175-4, which is higher than what was in
> rawhide before (qpidc-0.5.819819-1.fc13), and the same codebase but a revision
> higher than what's in F12 (qpidc-0.5.829175-3.fc12).

Ok.

> 
> I fixed most of the warnings from rpmlint (except for "no-documentation", etc),
> but these errors are still present (explanations for each are inline):
> * qpid-cpp-server.i686: E: non-readable /var/lib/qpidd/qpidd.sasldb 0600
>   - this is supposed to non-readble

Ok, I can see that in the spec file too and it is OK, but generally is a good idea to document in the spec file why.

> * qpid-cpp-server-store.i686: E: explicit-lib-dependency libaio
>   - there is no explicit lib dependency, there is a requires for a package
>     named libaio: "Requires: libaio" (line 281)

You have to drop the explicit Requires: libaio. rpm dependency resolver will add that automatically for you.

> * qpid-cpp-server-store.i686: E: non-standard-dir-perm /var/rhm 0775
>   - supposed to have those permissions

same as above.. document why.

> * ruby-qmf.i686: E: binary-or-shlib-defines-rpath
> /usr/lib/ruby/site_ruby/1.8/i386-linux/qmfengine.so
> ['/builddir/build/BUILD/qpidc-0.5.829175/cpp/src/.libs']
>   - not sure why it's complaining, I'm using the standard ruby_sitelib and
> ruby_sitearch macros, not hardcoding any path

I won´t make this a blocker for the package to be renamed, but please cross check with ruby packaging policy and the ruby team. It might be a bug that´s been introduced on the ruby macro.


So just to make this quick, I´ll approve the rename of the package, but please fix those bits when importing into cvs.

Fabio

(In reply to comment #7)

For the sake of completeness, I've uploaded new versions of the SRPM and the specfile to the same URLs, to address the comments above.

>> * qpid-cpp-server.i686: E: non-readable /var/lib/qpidd/qpidd.sasldb 0600
>>   - this is supposed to non-readble
> 
> Ok, I can see that in the spec file too and it is OK, but generally is a good
> idea to document in the spec file why.
>
>> * qpid-cpp-server-store.i686: E: non-standard-dir-perm /var/rhm 0775
>>   - supposed to have those permissions
> 
> same as above.. document why.

Done, added the following comments to the specfile:

# qpidd.sasldb contains sasl credentials, needs to be readable only by root
%attr(600, qpidd, qpidd) %config(noreplace) %_localstatedir/lib/qpidd/qpidd.sasldb

and

# /var/rhm needs to be group writable so that journal files can be updated properly
%attr(0775,qpidd,qpidd) %dir %_localstatedir/rhm


>> * qpid-cpp-server-store.i686: E: explicit-lib-dependency libaio
>>   - there is no explicit lib dependency, there is a requires for a package
>>     named libaio: "Requires: libaio" (line 281)
> 
> You have to drop the explicit Requires: libaio. rpm dependency resolver will
> add that automatically for you.

Done.

> So just to make this quick, I´ll approve the rename of the package, but please
> fix those bits when importing into cvs.

Thank you!

New Package CVS Request
=======================
Package Name: qpid-cpp
Short Description: AMQP messaging based on Qpid apache.org project.
Owners: nsantos
Branches: devel F-13
InitialCC:

CVS done (by process-cvs-requests.py).

Nuno, cvs has been done and all steps are completed, please follow up on this bug by closing or whatever step is required now.

Package Change Request
======================
Package Name: qpid-cpp
New Branches: el5 el6
Owners: mcpierce

Git done (by process-git-requests).

Package Change Request
======================
Package Name: qpid-cpp
New Branches: epel7
Owners: mcpierce

Git done (by process-git-requests).