Bug 221944
Summary: | Review Request: libofx - A library for supporting Open Financial Exchange (OFX) | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Bill Nottingham <notting> |
Component: | Package Review | Assignee: | manuel wolfshant <manuel.wolfshant> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | rvokal |
Target Milestone: | --- | Keywords: | Reopened |
Target Release: | --- | Flags: | j:
fedora-review+
j: fedora-cvs+ |
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-06-09 04:07:28 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
Bill Nottingham
2007-01-09 04:06:08 UTC
Some remarks: - Consider disabling building the static libs (%configure --disable-static) - Consider using "make DESTDIR=$RPM_BUILD_ROOT install" instead of %makeinstall - Could you explain the "Requires: openjade". AFAIS, this package only needs a certain version of libopensp and doesn't actually apply/require openjade. - Consider using a standard FE BuildRoot and using %{?dist}. I am not going to formally reviewing the package because of the %buildroot being used, sorry. You're going to comment but not review based on one item? And the point of that is....? --disable-static added, wee, get to save a few secs. DESTDIR added. The Requires is old historical stuff back when the libs were in openjade and it required that version to run right. Buildroot tweaked. Since upstream breaks abi even in minor point releases (0.8.0 -> 0.8.2), not sure what use pretending it can be built for multiple distros at once is, so no %{?dist}. (In reply to comment #2) > You're going to comment but not review based on one item? Yes, it's an item I have on my personal show stopper list. > And the point of that is....? Fighting guerillias? (In reply to comment #3) > (In reply to comment #2) > > You're going to comment but not review based on one item? > Yes, it's an item I have on my personal show stopper list. Ok then. Mind you, the buildroot shouldn't even be *IN* the spec file, but that's an issue for a different day. I'll try to review this one. Preliminary comments - src.rpm should be updated when spec file is updated. You forgot to include the modified spec in the -2 src.rpm - upstream has released a new version yesterday Preliminary review for release 2: * RPM name is OK * Source libofx-0.8.2.tar.gz is the same as upstream but not last version, http://sourceforge.net/project/showfiles.php?group_id=61170&package_id=57485&release_id=477013 shows that version 0.8.3 is available * Builds fine in mock * rpmlint of libofx-devel looks OK * File list of libofx looks OK * File list of libofx-devel looks OK rpmlint on libofx gives: E: libofx binary-or-shlib-defines-rpath /usr/bin/ofx2qif ['/usr/lib64'] E: libofx binary-or-shlib-defines-rpath /usr/bin/ofxdump ['/usr/lib64'] E: libofx binary-or-shlib-defines-rpath /usr/lib64/libofx.so.3.0.0 ['/usr/lib64'] Shouldn't this be fixed ? (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > You're going to comment but not review based on one item? > > Yes, it's an item I have on my personal show stopper list. > > Ok then. Mind you, the buildroot shouldn't even be *IN* the spec file, Right, but until that day, I for one will insist on all rpm.specs using the same %buildroot for the sake of "reproducability/deterministic builts" and simplicity. (In reply to comment #5) > I'll try to review this one. OK, one of the Guerilla has took over ... I quit from this review. Re: source rpm - there's a -3 there. Re: 0.8.3 - will handle once imported ; it shouldn't change much, aside from possibly the ABI. Will look at the rpath, although that's almost certainly libtool dainbramage. 0.8.3-1 uploaded in the same place; it does not change ABI (just a crash bugfix.) rpath fixed with libtool override. Added conditional support for libxml++ for ofxconnect test utility - disabled while I get with upstream about whether he wants to support it with newer libxml++. (also, it adds a boatload of reqs to the package.) MUST - rpmlint is silent on bioth src and binary packages - package meets naming guidelines - package meets packaging guidelines - license (GPL ) is OK, text in %doc, matches source - spec file legible, in am. english - source is latest version (0.8.3), matches upstream; sha1sum 0fa22f0535c1f50d4b9056f6f86d1429f1c8485f libofx-0.8.3.tar.gz - package builds on devel (x86 and i386) - missing BR: libtool - no unnecessary BR - no locales - not relocatable - owns all directories and files that it creates - no duplicate files - permissions ok - %clean ok - macro use consistent - code, not content - docs and examples are included in the -devel package - nothing in %doc affects runtime - no need for .desktop file - post/postun ldconfig ok - no scriptlets but ldconfig - devel package ok - no .la files - devel correctly requires base package and also pkgconfig (due to presence of .pc) SHOULD Items: - no translations - builds/installs on devel (i386 and x86_64; no ppc to test on) - none of the bundled binaries segfaults on i386 Add the missing BR (libtool) and the package is APPROVED. Suggestion: since you do not build all the included binaries (ofxconnect) and the main purpose is just the library anyway, how about moving the bundled binaries (ofx2qif,ofxdump) to a separate ofx-tools or smtg similar ? They do not seem of much use Hm, maybe. Might want to see what others do. libtool added in CVS. Will import at some point once the whole stack gets through. * missing "Requires: opensp-devel" in libofx-devel This is because upstream's pkgconfig template is customised for static linking. * the INSTALL file in %doc is irrelevant to RPM package users 0.8.3-3 uploaded - INSTALL, opensp-devel fixed. Tools split off into a 'ofx' package. Bill, could you please close this bug, since the packages seem to have been successfully built and pushed ? This is built now. Package Change Request ====================== Package Name: libofx New Branches: EL-4 EL-5 CVS done. |