Bug 193493
Summary: | Review Request: iksemel - An XML parser library designed for Jabber applications | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jeffrey C. Ollie <jeff> |
Component: | Package Review | Assignee: | Jason Tibbitts <j> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | panemade |
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-07-28 19:10:49 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Jeffrey C. Ollie
2006-05-29 19:26:54 UTC
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. 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> - 1.2-2 - Add texinfo BR (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. 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. 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> - 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. (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. (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> - 1.2-4 - Don't re-run autotools, fix rpath in a different way. 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 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. (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... 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. (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. 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. 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> - 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. 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? (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... The build on development/i386 went fine, however the development/x86_64 build failed for me too.... 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 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. Package Change Request ====================== Package Name: iksemel New Branches: EL-4 EL-5 Updated EPEL Owners: jeff Needed in EPEL as dependency of Zabbix. cvs done. |