Bug 220630 - Review Request: qpidc - C++ implementation of AMQP messaging spec from Apache Qpid. Upstream for Red Hat Messaging.
Summary: Review Request: qpidc - C++ implementation of AMQP messaging spec from Apache...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ralf Corsepius
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-12-22 17:06 UTC by Alan Conway
Modified: 2007-11-30 22:11 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-04-12 20:01:06 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Alan Conway 2006-12-22 17:06:03 UTC
Spec URL: http://people.apache.org/dist/incubator/qpid/M1-incubating/cpp/qpidc.spec
SRPM URL: http://people.apache.org/dist/incubator/qpid/M1-incubating/cpp/qpidc-0.1-1.src.rpm
Description: First time submitter, need a sponsor.

Background: C++ implementation of the AMQP messaging specification (http://amqp.org/) from Apache Qpid project (http://incubator.apache.org/qpid). This is the upstream for the Red Hat Messaging project hosted at https://etp.108.redhat.com. I will submit an RPM for 108 stuff shortly.

%description 
Run-time libraries for AMQP client applications developed using Qpid
C++. Clients exchange messages with an AMQP message broker using
the AMQP protocol.

%description devel
Libraries, header files and documentation for developing AMQP clients
in C++ using Qpid.  Qpid implements the AMQP messaging specification.

%description -n qpidd
A message broker daemon that receives stores and routes messages using
the open AMQP messaging protocol.

Comment 1 Jason Tibbitts 2006-12-30 04:09:40 UTC
A couple of comments:

This package has rpath problems:
E: qpidd binary-or-shlib-defines-rpath /usr/sbin/qpidd ['/usr/lib64']

Loads of undefined-non-weak-symbol complaints from the installed package:
W: qpidc undefined-non-weak-symbol /usr/lib64/libqpidclient.so.0.1.0
_ZTIN4qpid7framing11BodyHandlerE
W: qpidc undefined-non-weak-symbol /usr/lib64/libqpidclient.so.0.1.0
_ZTIN4qpid3sys8RunnableE
W: qpidc undefined-non-weak-symbol /usr/lib64/libqpidclient.so.0.1.0
_ZNK4qpid7framing13AMQMethodBody6encodeERNS0_6BufferE
and 161 others.

Picking nits, I know, but:
BuildReqires: cppunit is redundant.
Requires: apr is also redundant; rpm finds the libapr-1.so.0 dependency
automatically.  Oddly, it doesn't find the boost dependency.

Comment 2 Jim Meyering 2007-01-05 21:14:46 UTC
Thanks for the feedback.  FYI, I've fixed the undefined-non-weak-symbol problem
with this patch:
http://mail-archives.apache.org/mod_mbox/incubator-qpid-dev/200701.mbox/%3c87ps9tz3ko.fsf@rho.meyering.net%3e

Comment 3 Alan Conway 2007-01-05 21:55:41 UTC
Applied to https://svn.apache.org/repos/asf/incubator/qpid/trunk/qpid/cpp at r493151

Comment 4 Alan Conway 2007-01-23 21:58:16 UTC
Updaed the SRPM & spec URLs with Jim's fixes for review.

Comment 5 Alan Conway 2007-01-24 04:07:11 UTC
Increased the release number and published the new SRPM at:
http://people.apache.org/dist/incubator/qpid/M1-incubating/cpp/qpidc-0.1-2.src.rpm

No changes since comment #4 except to increase the relase number and add a
changelog note.

Comment 6 Ralf Corsepius 2007-01-24 05:25:39 UTC
- spec still contains redundant "BuildRequires: cppunit"

- Unnecessarily wasting time on building static libs
=> %configure --disable-static

- rpmlint:
W: qpidc incoherent-version-in-changelog 0.1-1 0.1-2
=> Probably an oversight.

E: qpidc script-without-shebang /usr/share/doc/qpidc-0.1/LICENSE.txt
=> chmod -x LICENSE.txt in %prep.



Comment 7 Jim Meyering 2007-01-24 11:46:45 UTC
Hi Ralf,

Thanks again for the quick feedback.  I've posted a patch here:
http://www.nabble.com/qpidc-RPM-feedback-t3080445.html

Comment 8 Alan Conway 2007-01-25 17:34:47 UTC
Applied Jims patch and fixed other issues mentioned in the mail
http://www.nabble.com/qpidc-RPM-feedback-t3080445.html
removed .txt extensions, fixed svn ignores etc.

Updated spec, tarballs and SRPM at: 
http://people.apache.org/dist/incubator/qpid/M1-incubating/cpp/

Comment 9 Warren Togami 2007-02-13 20:39:25 UTC
Ralf or Jason, do you want to continue this review, or you gave up?

Comment 10 Ralf Corsepius 2007-02-14 03:38:11 UTC
(In reply to comment #9)
> Ralf or Jason, do you want to continue this review, or you gave up?
This review request simply got swept away and drowned in this tsunami of review
mails.

Technically, I recall me having wanted to investigate an issue I suspect this
package to suffer from, but I haven't gotten around to it, yet.

Comment 11 Ralf Corsepius 2007-02-14 04:59:49 UTC
OK, the issues I mentioned above don't seem to have actual side-effects:

* Diffing the log of a mock-built against a user-built:
...
-checking for java... no
+checking for java... java
 checking for javac... no
...
 checking whether compiler accepts -Werror... no
=> Looks like a bug in the configure script to me.
 
...
-checking dynamic linker characteristics... cat: ld.so.conf.d/*.conf: No such
file or directory
-GNU/Linux ld.so
+checking dynamic linker characteristics... GNU/Linux ld.so
=> Probably a buggy configure.


* Also I don't understand why libqpidbroker.so.0 exists at all.
It's exclusively used by qpidd, there is no devel package associated to it, but
... let me presume upstream has reasons for doing so.


None of them are show-stoppers.


Remains one issue:
* Source0: Should be an absolute URL.

Provided you add this, consider this package APPROVED.

Comment 12 Jim Meyering 2007-02-14 09:26:05 UTC
Hi Ralf,

Thanks for the detailed feedback.

The ld.so difference is due to the way libtool's AC_LIBTOOL_SYS_DYNAMIC_LINKER
works when /etc/ld.so.conf contains a glob pattern that matches no file.  In the
mock-build environment, I'll bet that there are no *.conf files in /etc/ld.so.conf.d

The java configure difference is probably because java was installed in your
user-built environment, but not in the mock-built one.  For qpidc, java is
optional. Used if found, but no problem (just reduced build-time functionality)
if not.

Comment 13 Jim Meyering 2007-02-20 21:23:08 UTC
I've addressed all problems and put the new specs, tarballs and RPMs here:
http://rhm.et.redhat.com/download/qpidc-0.1-4.src.rpm


Comment 14 Warren Togami 2007-02-20 22:26:53 UTC
Who will be the owners of this package?

Do you already have Fedora Account System accounts?

Have you requested cvsextras access?

(I don't know you, so I hope we can talk a short bit sometime during Wednesday
before sponsoring your membership.)

Comment 15 Alan Conway 2007-02-21 14:20:54 UTC
Main owner is myself, fedora account aconway. cvsextras requested, mailed
wtogami directly with contact info to talk today.

Comment 16 Jim Meyering 2007-02-23 14:31:49 UTC
we've "talked", and I too have requested cvsextras access.

New Package CVS Request
=======================
Package Name: qpidc
Short Description: C++ implementation of AMQP messaging spec from Apache Qpid
Owners: aconway, meyering
Branches: FC-7
InitialCC:

Comment 17 Warren Togami 2007-02-23 18:04:59 UTC
Looks like Ralf approved this back in Comment #11, but didn't assign to himself.

Comment 18 Alan Conway 2007-03-05 19:07:38 UTC
Please add Nuno Santos <nsantos> as an owner of the qpidc package.
He'll be helping to maintain it going forward.

Comment 19 Warren Togami 2007-03-05 19:22:52 UTC
done

Comment 20 Bernard Johnson 2007-04-11 22:46:25 UTC
Pardon the bugzilla spam.  This package appears to have been approved, imported,
and built.

If that is the case, please close this bug RESOLVE -> NEXTRELEASE as documented
in the package review process:
http://fedoraproject.org/wiki/PackageReviewProcess?#head-df921556b35438a4c78b4b6a790151ea568e8f9e

Comment 21 Nuno Santos 2007-10-26 15:41:19 UTC
Package Change Request
======================
Package Name: qpidc
New Branches: F-7 F-8

Please add branches to allow inclusion in F-7 updates and F-8 updates.

Comment 22 Warren Togami 2007-10-29 16:52:09 UTC
These branches already exist.  What is your actual goal?


Comment 23 Nuno Santos 2007-10-29 17:27:54 UTC
The goal is to get an updated version of qpidc in both F7-updates and F8-updates.


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