Bug 221944 - Review Request: libofx - A library for supporting Open Financial Exchange (OFX)
Review Request: libofx - A library for supporting Open Financial Exchange (OFX)
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Package Reviews List
: Reopened
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2007-01-08 23:06 EST by Bill Nottingham
Modified: 2014-03-16 23:04 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-09 00:07:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Bill Nottingham 2007-01-08 23:06:08 EST
Spec URL: http://people.redhat.com/notting/libofx/libofx.spec
SRPM URL: http://people.redhat.com/notting/libofx/libofx-0.8.2-2.src.rpm
Description: libofx package for OFX support.

Starting reviews of the gnucash stack in preparation for the grand merge. Can't actually move until everything is reviewed.
Comment 1 Ralf Corsepius 2007-01-09 00:17:45 EST
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.
Comment 2 Bill Nottingham 2007-01-09 00:44:04 EST
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}.
Comment 3 Ralf Corsepius 2007-01-09 01:27:25 EST
(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?
Comment 4 Bill Nottingham 2007-01-09 01:28:35 EST
(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.
Comment 5 manuel wolfshant 2007-01-09 04:37:30 EST
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
Comment 6 manuel wolfshant 2007-01-09 04:43:40 EST
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 ?
Comment 7 Ralf Corsepius 2007-01-09 06:07:56 EST
(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.

Comment 8 Bill Nottingham 2007-01-09 11:49:57 EST
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.
Comment 9 Bill Nottingham 2007-01-09 12:43:15 EST
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.)
Comment 10 manuel wolfshant 2007-01-09 15:05:22 EST
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
Comment 11 Bill Nottingham 2007-01-09 15:14:48 EST
Hm, maybe. Might want to see what others do.

libtool added in CVS. Will import at some point once the whole stack gets through.
Comment 12 Michael Schwendt 2007-01-09 16:39:52 EST
* 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
Comment 13 Bill Nottingham 2007-01-11 17:39:31 EST
0.8.3-3 uploaded - INSTALL, opensp-devel fixed. Tools split off into a 'ofx'
package.
Comment 14 manuel wolfshant 2007-03-16 22:52:39 EDT
Bill, could you please close this bug, since the packages seem to have been
successfully built and pushed ?
Comment 15 Bill Nottingham 2007-03-19 15:31:00 EDT
This is built now.
Comment 16 Bill Nottingham 2007-06-08 21:17:33 EDT
Package Change Request
======================
Package Name: libofx
New Branches: EL-4 EL-5

Comment 17 Jason Tibbitts 2007-06-09 00:07:28 EDT
CVS done.

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