Bug 193493 - Review Request: iksemel - An XML parser library designed for Jabber applications
Review Request: iksemel - An XML parser library designed for Jabber applications
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-05-29 15:26 EDT by Jeffrey C. Ollie
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-07-28 15:10:49 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jeffrey C. Ollie 2006-05-29 15:26:54 EDT
Spec URL: http://www.ocjtech.us/iksemel-1.2-1.spec
SRPM URL: http://www.ocjtech.us/iksemel-1.2-1.src.rpm
Description:

An XML parser library designed for Jabber applications. It is coded in
ANSI C for POSIX compatible environments, thus highly portable.
Comment 1 Parag AN(पराग) 2006-06-01 01:46:38 EDT
I recompiled source package on FC5 i386 system and saw some warnings. Try to
solve it. 
stream.c: In function 'iks_sasl_challenge':
stream.c:176: warning: pointer targets in passing argument 2 of 'iks_md5_hash'
differ in signedness
stream.c:177: warning: pointer targets in passing argument 2 of 'iks_md5_hash'
differ in signedness
stream.c:178: warning: pointer targets in passing argument 2 of 'iks_md5_hash'
differ in signedness
stream.c:179: warning: pointer targets in passing argument 2 of 'iks_md5_hash'
differ in signedness

Rest SPEC and RPM package looks ok.
Comment 2 Jeffrey C. Ollie 2006-06-01 08:29:33 EDT
Update Spec/SRPMS:

Spec URL: http://www.ocjtech.us/iksemel-1.2-2.spec
SRPM URL: http://www.ocjtech.us/iksemel-1.2-2.src.rpm

* Tue May 30 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2-2
- Add texinfo BR
Comment 3 Jeffrey C. Ollie 2006-06-01 08:31:33 EDT
(In reply to comment #1)
> I recompiled source package on FC5 i386 system and saw some warnings. Try to
> solve it. 

Since they are only warnings, I would think that this is something to be dealt
with upstream.
Comment 4 Jason Tibbitts 2006-06-26 00:33:21 EDT
I noticed the following build failure:

+ aclocal
aclocal:configure.ac:14: warning: macro `AM_PROG_LIBTOOL' not found in library

which is solved by adding BuildRequires: libtool.  However, I'm not sure why you
think you need to rerun the autotools.  You can disable rpath just by using
make LIBTOOL=/usr/bin/libtool %{?_smp_mflags}
and then deleting the extra .a file that shows up.

Other than that, the package looks good and rpmlint is completely silent.  There
is, however, a test suite that you should call (with "make check") in a %check
section.

Review:
* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* source files match upstream:
   82e7c8fdb6211839246b788c040a796b  iksemel-1.2.tar.gz
* latest version is being packaged.
X BuildRequires are proper (libtool, as above)
* package builds in mock (development, x86_64) (after fixing libtool)
* rpmlint is silent.
* final provides and requires are sane:
  iksemel-1.2-2.fc6.x86_64.rpm
   libiksemel.so.3()(64bit)
   iksemel = 1.2-2.fc6
  =
   /sbin/ldconfig
  libiksemel.so.3()(64bit)

  iksemel-devel-1.2-2.fc6.x86_64.rpm
   iksemel-devel = 1.2-2.fc6
  =
   /bin/sh
   /sbin/install-info
   gnutls-devel
   iksemel = 1.2-2.fc6
   libiksemel.so.3()(64bit)
   pkgconfig

  iksemel-utils-1.2-2.fc6.x86_64.rpm
   iksemel-utils = 1.2-2.fc6
  =
   iksemel = 1.2-2.fc6
   libiksemel.so.3()(64bit)
* shared libraries are present; ldconfig is called properly and the unversioned
.so is in the -devel subpackage.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* %clean is present.
X %check is not present but there is a test suite.
* scriptlets present and OK (ldconfig, install-info)
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers present, in -devel package.
* pkgconfig files present, in -devel package and pkgconfig is required.
* no libtool .la droppings.
* not a GUI app.
Comment 5 Jeffrey C. Ollie 2006-06-26 14:50:48 EDT
Updated Spec/SRPM:

Spec: http://repo.ocjtech.us/misc/fedora/5/SRPMS/iksemel-1.2-3.fc5.spec
SRPM: http://repo.ocjtech.us/misc/fedora/5/SRPMS/iksemel-1.2-3.fc5.src.rpm

%changelog
* Mon Jun 26 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2-3
- Add a %check section.
- Add BR for libtool.

The %check section is skipped on non-i386 architectures because one of the tests
fail.  I also decided to stick with re-running autotools because BR'ing libtool
would pull in autoconf and automake anyway, although setting
LIBTOOL=/usr/bin/libtool worked to remove the rpath.
Comment 6 Jason Tibbitts 2006-06-26 15:01:18 EDT
(In reply to comment #5)
> The %check section is skipped on non-i386 architectures because one of the tests
> fail.

Perhaps that indicates that this package doesn't actually work on x86_64.  Have
you consulted upstream?

> I also decided to stick with re-running autotools because BR'ing libtool
> would pull in autoconf and automake anyway, although setting
> LIBTOOL=/usr/bin/libtool worked to remove the rpath.

It's not about pulling them in; it's about running them in the first place.  You
really shouldn't re-autotool a package unless you simply have no other choice. 
In this case there is a perfectly good and well-accepted alternative.
Comment 7 Jeffrey C. Ollie 2006-06-26 17:07:34 EDT
(In reply to comment #6)
> (In reply to comment #5)
> > The %check section is skipped on non-i386 architectures because one of the tests
> > fail.
> 
> Perhaps that indicates that this package doesn't actually work on x86_64.  Have
> you consulted upstream?

The failed test is testing iksemel's own SHA1 has implementation, which
obviously has some 64-bit unclean code.  As long as you avoid using the internal
SHA1 code you're good to go.  (I actually use iksemel on a x86_64 box.)

> > I also decided to stick with re-running autotools because BR'ing libtool
> > would pull in autoconf and automake anyway, although setting
> > LIBTOOL=/usr/bin/libtool worked to remove the rpath.
> 
> It's not about pulling them in; it's about running them in the first place.  You
> really shouldn't re-autotool a package unless you simply have no other choice. 
> In this case there is a perfectly good and well-accepted alternative.

Spec: http://repo.ocjtech.us/misc/fedora/5/SRPMS/iksemel-1.2-4.fc5.spec
SRPM: http://repo.ocjtech.us/misc/fedora/5/SRPMS/iksemel-1.2-4.fc5.src.rpm

%changelog
* Mon Jun 26 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2-4
- Don't re-run autotools, fix rpath in a different way.
Comment 8 Jason Tibbitts 2006-06-27 13:22:10 EDT
Hmmm.  I dislike the idea of shipping something which is known to be (partially)
broken; you should at least add a README.fedora file which makes this clear and
describes how to make sure that you're avoiding the broken portion of the
package.  You should also add comments in the spec to indicate the reasons for
disabling the test suite on everything but i386, and at least try it on PPC by
letting the buildsys run it.  (So use %ifnarch x86_64 instead of %ifarch i386.)

Other than that, the form of the package is fine now
Comment 9 Jason Tibbitts 2006-07-08 20:07:07 EDT
Any movement here?  This package just needs a couple of things: documentation
about the partial x86_64 brokenness, and at least an attempt to build on PPC.
Comment 10 Jeffrey C. Ollie 2006-07-27 22:48:37 EDT
(In reply to comment #9)
> Any movement here?  This package just needs a couple of things: documentation
> about the partial x86_64 brokenness, and at least an attempt to build on PPC.

I have no access at all to a PPC system so I don't know how I can test building
there...  I'll get a readme typed up quick and gew a new package put together...
Comment 11 Jason Tibbitts 2006-07-27 23:52:30 EDT
Perhaps I wasn't sufficiently clear earlier.  My point is that you've prevented
building on PPC when you have no evidence that it fails to work.  Why not just
leave it enabled and let the buildsystem go ahead and build it?  If it fails
there then you can mask it out.  Few of us have PPC machines to test on but we
still let our packages build there and then investigate if the build fails or
bugs are reported.  If we enabled PPC building only when we had actual PPC
machines to test on then most packages would be disabled on PPC.

Comment 12 Jeffrey C. Ollie 2006-07-28 00:18:38 EDT
(In reply to comment #11)
> Perhaps I wasn't sufficiently clear earlier.  My point is that you've prevented
> building on PPC when you have no evidence that it fails to work.  Why not just
> leave it enabled and let the buildsystem go ahead and build it?  If it fails
> there then you can mask it out.  Few of us have PPC machines to test on but we
> still let our packages build there and then investigate if the build fails or
> bugs are reported.  If we enabled PPC building only when we had actual PPC
> machines to test on then most packages would be disabled on PPC.

Are we looking at the same spec file here?  I fail to see where I prevented
building on PPC.  The only special casing of architecture is in the %check
section.  I don't understand how using "%ifarch i386" in the %check section
would prevent building of the whole package on PPC.  If not running "make check"
on PPC is what you're objecting to here, no problem... I'll fix that in my next
build.  If it's something else, perhaps there's some aspect to the %check
section that I'm not grokking that would prevent the package as a whole from
building on PPC.
Comment 13 Jason Tibbitts 2006-07-28 00:23:13 EDT
Sorry, you're right.  In the month that this bug sat inactive the defaults faded
from my memory.

My original issue is that you should let the test suite run on PPC.
Comment 14 Jeffrey C. Ollie 2006-07-28 01:59:24 EDT
Well...  Instead of writing a README explaining why iksemel was broken on
x86_64, I just went ahead and fixed it.  Once I get some sleep I'll submit the
patch upstream.

Spec: http://repo.ocjtech.us/misc/fedora/5/SRPMS/iksemel-1.2-5.fc5.spec
SRPM: http://repo.ocjtech.us/misc/fedora/5/SRPMS/iksemel-1.2-5.fc5.src.rpm

* Thu Jul 27 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2-5
- Patch to use SHA1 hashing routines from libgcrypt rather than
  broken internal code.  This means that we need to BR autoools
  to regenerate configure script and makefiles.
Comment 15 Jason Tibbitts 2006-07-28 10:53:34 EDT
Yes, that's very smart.  Unfortunately it failed to build for me on x86_64:

make[1]: Entering directory `/builddir/build/BUILD/iksemel-1.2/test'
make  check-TESTS
make[2]: Entering directory `/builddir/build/BUILD/iksemel-1.2/test'
PASS: tst-ikstack
PASS: tst-iks
/bin/sh: line 4: 12917 Segmentation fault      ${dir}$tst
FAIL: tst-sax
PASS: tst-dom
PASS: tst-sha
PASS: tst-md5
PASS: tst-filter
PASS: tst-jid
===================
1 of 8 tests failed
===================
make[2]: *** [check-TESTS] Error 1

Any ideas?
Comment 16 Jeffrey C. Ollie 2006-07-28 11:33:09 EDT
(In reply to comment #15)
> Unfortunately it failed to build for me on x86_64:

Is that a build on development?  I've only tested FC5 (i386 & x86_64) builds so
far.  I kicked off a development/i386 build but that'll take a bit since I don't
have any of the packages cached locally.  I'll try development/x86_64 once that
completes...
Comment 17 Jeffrey C. Ollie 2006-07-28 12:12:29 EDT
The build on development/i386 went fine, however the development/x86_64 build
failed for me too....
Comment 18 Jason Tibbitts 2006-07-28 12:18:54 EDT
My build was on development.  I did a build on FC5 and it did indeed pass:

make  check-TESTS
make[2]: Entering directory `/builddir/build/BUILD/iksemel-1.2/test'
PASS: tst-ikstack
PASS: tst-iks
PASS: tst-sax
PASS: tst-dom
PASS: tst-sha
PASS: tst-md5
PASS: tst-filter
PASS: tst-jid
==================
All 8 tests passed
==================

At this point I have no remaining objections; things build on a supported
release, and looking at the patches I agree with the necessity of re-autotooling
things although I hope that it won't be necessary if your patch makes it upstream.

So I'll approve now, but do keep in mind that as is you can't build on the
development branch.

APPROVED
Comment 19 Jeffrey C. Ollie 2006-07-28 15:10:49 EDT
I've entered bug #200579 to track the problem with building on devel/x86_64. 
Otherwise, the package has been imported and branches for FC-4 and FC-5 requested.
Comment 20 Jeffrey C. Ollie 2007-07-16 10:59:34 EDT
Package Change Request
======================
Package Name: iksemel
New Branches: EL-4 EL-5
Updated EPEL Owners: jeff@ocjtech.us

Needed in EPEL as dependency of Zabbix.
Comment 21 Kevin Fenzi 2007-07-16 14:07:18 EDT
cvs done.

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