Created attachment 145526 [details]
build failure log

Not a full review, just an early bird's picks:

* %defattr missing in several sub-packages

* Obsoletes ought to specify max.versions (using "LT", or "LE"
inequations)

* main package must not own %{_libdir}/gwenhywfar since that belongs
into the "gwenhywfar" pkg already

* Excludes are not symmetric. That's dangerous since you can
lose files:

main package:
%exclude %{_libdir}/aqbanking/plugins/*/debugger
%exclude %{_libdir}/aqbanking/plugins/*/frontends/*
%exclude %{_libdir}/aqbanking/plugins/*/wizards

qbanking package:
%{_libdir}/aqbanking/plugins/*/debugger
%{_libdir}/aqbanking/plugins/*/frontends/qbanking
%{_libdir}/aqbanking/plugins/*/wizards

You exclude everything in
%{_libdir}/aqbanking/plugins/*/frontends/*
but only
%{_libdir}/aqbanking/plugins/*/frontends/qbanking
is included explicitly. Other content below
%{_libdir}/aqbanking/plugins/*/frontends/
would be skipped/excluded silently.

* the -devel packages ought to "Requires: automake", so the
%{_datadir}/aclocal/ directory is not orphaned - and because smart
applications use automake anyway - it's kinda difficult to determine
the config values without using the foo-config scripts/pkgconfig/automake

* [ "$RPM_BUILD_ROOT" != "/" ] && rm -rf $RPM_BUILD_ROOT
Useless check. Nowadays buildroot cannot be "/" anymore.

* "Requires: pkgconfig" in the frontends' -devel packages is redundant,
since they all need aqbanking-devel which in turn requires pkgconfig
for all its *-config queries (due to the applied Patch2)

Doesn't build in mock (rawhide x86_64), build.log ends with;
<<
DIE_RPATH_DIE="/usr/lib64:$DIE_RPATH_DIE" gcc -O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
-m64 -mtune=generic -g -Wall -g -o .libs/testlib testlib.o  ./.libs/libcbanking.so
/usr/bin/ld: warning: libaqbanking.so.16, needed by ./.libs/libcbanking.so, not
found (try using -rpath or -rpath-link)
testlib.o: In function `main':
/builddir/build/BUILD/aqbanking-2.1.0/src/frontends/cbanking/testlib.c:6:
undefined reference to `AB_Banking_free'
./.libs/libcbanking.so: undefined reference to `AB_Banking_SetProgressEndFn'
./.libs/libcbanking.so: undefined reference to `AB_Banking_SetHideBoxFn'
./.libs/libcbanking.so: undefined reference to `AB_Banking_new'
./.libs/libcbanking.so: undefined reference to `AB_Banking_SetProgressLogFn'
./.libs/libcbanking.so: undefined reference to `AB_Banking_InputBox'
./.libs/libcbanking.so: undefined reference to `AB_Banking_SetShowBoxFn'
./.libs/libcbanking.so: undefined reference to `AB_Banking_SetMessageBoxFn'
./.libs/libcbanking.so: undefined reference to `AB_Banking_SetGetPinFn'
./.libs/libcbanking.so: undefined reference to `AB_BANKING__INHERIT_GETLIST'
./.libs/libcbanking.so: undefined reference to `AB_BANKING__INHERIT_SETDATA'
./.libs/libcbanking.so: undefined reference to `AB_Banking_SetProgressAdvanceFn'
./.libs/libcbanking.so: undefined reference to `AB_Banking_SetInputBoxFn'
./.libs/libcbanking.so: undefined reference to `AB_Banking_MessageBox'
./.libs/libcbanking.so: undefined reference to `AB_Banking_SetProgressStartFn'
collect2: ld returned 1 exit status
make[4]: *** [testlib] Error 1
make[4]: Leaving directory
`/builddir/build/BUILD/aqbanking-2.1.0/src/frontends/cbanking'
make[3]: *** [all-recursive] Error 1
make[3]: Leaving directory `/builddir/build/BUILD/aqbanking-2.1.0/src/frontends'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/builddir/build/BUILD/aqbanking-2.1.0/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/builddir/build/BUILD/aqbanking-2.1.0'
make: *** [all] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.37465 (%build)
>>

Other minor issues;
Shouldn't Buildrequires on python be python-devel instead?
Also the python packaging guildeline mandates defining python_sitelib at the top
of your specfile, so the files under -python-%{name} subpackages would just go
into %{python_sitelib}/%{name}/

(In reply to comment #1)

> * %defattr missing in several sub-packages

Gack, fixed.

> * Obsoletes ought to specify max.versions (using "LT", or "LE"
> inequations)

They're obsolete upstream projects that this replaced - is there really
a need to specify a version for such things?

> * main package must not own %{_libdir}/gwenhywfar since that belongs
> into the "gwenhywfar" pkg already

Fixed.

> You exclude everything in
> %{_libdir}/aqbanking/plugins/*/frontends/*
> but only
> %{_libdir}/aqbanking/plugins/*/frontends/qbanking
> is included explicitly. Other content below
> %{_libdir}/aqbanking/plugins/*/frontends/
> would be skipped/excluded silently.

So, other frontends could eventually add code. If I only exclude the specific
frontend, the code will end up wrongly in the main package. If exclude all, as
it's currently done, it will end up silently dropped. Fun.

I suppose in the wrong package is better than silently missed.

> * the -devel packages ought to "Requires: automake", so the
> %{_datadir}/aclocal/ directory is not orphaned - and because smart
> applications use automake anyway - it's kinda difficult to determine
> the config values without using the foo-config scripts/pkgconfig/automake

??? Why bring automake into the buildroot if the package never calls it?
Seems better to have something else own the dir.

> * [ "$RPM_BUILD_ROOT" != "/" ] && rm -rf $RPM_BUILD_ROOT
> Useless check. Nowadays buildroot cannot be "/" anymore.

Fixed.

> * "Requires: pkgconfig" in the frontends' -devel packages is redundant,
> since they all need aqbanking-devel which in turn requires pkgconfig
> for all its *-config queries (due to the applied Patch2)

True, but since all the -config scripts *explicitly* call it, I'm more
comfortable leaving the Requires: in.

(In reply to comment #2)
> Doesn't build in mock (rawhide x86_64), build.log ends with;

Will poke at it. Yay libtool.

> Other minor issues;
> Shouldn't Buildrequires on python be python-devel instead?

It doesn't link against libpython, so, no.

> Also the python packaging guildeline mandates defining python_sitelib at the top
> of your specfile, so the files under -python-%{name} subpackages would just go
> into %{python_sitelib}/%{name}/

Is this on the wiki somewhere? I'm not finding it.

(In reply to comment #4)

> 
> > Also the python packaging guildeline mandates defining python_sitelib at the top
> > of your specfile, so the files under -python-%{name} subpackages would just go
> > into %{python_sitelib}/%{name}/
> 
> Is this on the wiki somewhere? I'm not finding it.

http://fedoraproject.org/wiki/Packaging/Python?highlight=%28python%29%7C%28packaging%29

Ah. That's not actually linked anywhere from the main guidelines page. Perhaps
it should be.

> ??? Why bring automake into the buildroot if the
> package never calls it? Seems better to have something
> else own the dir.

Wink, wink, Core dudes! ;-)

The reviewing guidelines are quite explicit in this case nowadays:

MUST: A package must own all directories that it creates. If it
does not create a directory that it uses, then it should require
a package which does create that directory. The exception to this
are directories listed explicitly in the Filesystem Hierarchy
Standard ([WWW] http://www.pathname.com/fhs/pub/fhs-2.3.html),
as it is safe to assume that those directories exist. 


But as I wrote, aqbanking-devel API users likely use automake
anyway. *g*

> versioned Obsoletes

... make it less of a hassle in case you (or your fellow packagers)
ever want to bring back packages with the obsolete names (and that
has been a real-world scenario before, not a purely theoretical one).

Admittely, it's far from a serious issue for the "aqhbci" namespace.

(In reply to comment #7)
> MUST: A package must own all directories that it creates. If it
> does not create a directory that it uses, then it should require
> a package which does create that directory. The exception to this
> are directories listed explicitly in the Filesystem Hierarchy
> Standard ([WWW] http://www.pathname.com/fhs/pub/fhs-2.3.html),
> as it is safe to assume that those directories exist. 

Messy; essentially means any automake-ed devel package will require automake
even if no one uses it. Ah well. Fixed.

> > versioned Obsoletes
> 
> ... make it less of a hassle in case you (or your fellow packagers)
> ever want to bring back packages with the obsolete names (and that
> has been a real-world scenario before, not a purely theoretical one).

Oh, I know the theory and that it happens; just not sure it will happen
in this case.

(Of course, if it happens with a new package, in the same namespace,
with a lower version, then you get to play with epochs. Yay!)

Tweaked for the last version I could find anywhere (1.0.3).

New stuff uploaded as -12.

The changes you made in the spec here,

mkdir -p $RPM_BUILD_ROOT/%{_datadir}/doc/%{name}-%{version}
mv $RPM_BUILD_ROOT/%{_datadir}/doc/{aqbanking,aqhbci}
$RPM_BUILD_ROOT/%{_datadir}/doc/%{name}-%{version}

are thrown out by the %doc macro in the files section. The docs you move there
are lost; I suggest you remove the %doc macro from aqbanking file section and
manually move {AUTHORS README COPYING ChangeLog} to that directory you created.

Also (not sure if this matters),somewhere in the build log there is
make[4]: Entering directory `/builddir/build/BUILD/aqbanking-2.1.0/bindings/python'
make[4]: Nothing to be done for `install-exec-am'.
./aqcodegen types.xml ../../src/libs/aqbanking/types > _aqtypes.py.tmp && \
	  mv _aqtypes.py.tmp _aqtypes.py || \
	  rm -f _aqtypes.py.tmp
Traceback (most recent call last):
  File "./aqcodegen", line 5, in <module>
    import xml.dom.ext.reader.Sax2
ImportError: No module named ext.reader.Sax2
test -z "/usr/lib/python2.5/site-packages/aqbanking" || mkdir -p --
"/var/tmp/aqbanking-2.1.0-12-root-mockbuild/usr/lib/python2.5/site-packages/aqbanking"

Fixing - the latter seems like a missing buildreq (PyXML)... are you still
getting a valid python package?

Fixes uploaded as -13.

You forgot to add the fixed doc directory to the file list. Adding it, and
running rpmlint on the resulting packages produce;

[deji@agape reviews]$ rpmlint aqbanking-2.1.0-13.src.rpm
W: aqbanking unversioned-explicit-obsoletes aqhbci-devel
- You forgot to version to this one.
[deji@agape reviews]$ rpmlint aqbanking-2.1.0-13.x86_64.rpm
E: aqbanking obsolete-not-provided aqhbci
- aqbanking needs to also 'Provides: aqhbci'
E: aqbanking zero-length /usr/share/aqbanking/bankinfo/us/bic.idx
[deji@agape reviews]$ rpmlint aqbanking-devel-2.1.0-13.x86_64.rpm
E: aqbanking-devel obsolete-not-provided aqhbci-devel
- Also needs to provided for.
E: aqbanking-devel zero-length usr/share/doc/aqbanking-devel-2.1.0/01-OVERVIEW
- Can be left out of the doc list
[deji@agape reviews]$ rpmlint qbanking-2.1.0-13.x86_64.rpm
E: qbanking obsolete-not-provided aqhbci-qt-tools
- I'm not sure you ever ship this, you can drop the obsolete or also provide it.
W: qbanking no-documentation
- I believe this and the rest like it can be ignored.
[deji@agape reviews]$ rpmlint qbanking-devel-2.1.0-13.x86_64.rpm
W: qbanking-devel no-documentation
[deji@agape reviews]$ rpmlint kbanking-2.1.0-13.x86_64.rpm
W: kbanking no-documentation
[deji@agape reviews]$ rpmlint kbanking-devel-2.1.0-13.x86_64.rpm
W: kbanking-devel no-documentation
[deji@agape reviews]$ rpmlint python-aqbanking-2.1.0-13.x86_64.rpm
W: python-aqbanking no-documentation
[deji@agape reviews]$ rpmlint g2banking-devel-2.1.0-13.x86_64.rpm
W: g2banking-devel no-documentation
[deji@agape reviews]$ rpmlint g2banking-2.1.0-13.x86_64.rpm
W: g2banking no-documentation

And you really haven't fixed
> * [ "$RPM_BUILD_ROOT" != "/" ] && rm -rf $RPM_BUILD_ROOT
> Useless check. Nowadays buildroot cannot be "/" anymore.

 (In reply to comment #12)

> [deji@agape reviews]$ rpmlint aqbanking-2.1.0-13.x86_64.rpm
> E: aqbanking obsolete-not-provided aqhbci
> - aqbanking needs to also 'Provides: aqhbci'

How does this help? Anything that actually used the old aqhbci will
not work even if this did provide it - different ABI, different interface,
different paths to the binaries used. I believe this falls under the '[not] a
compatible enough replacement' category of the provides/obsoletes draft.


> [deji@agape reviews]$ rpmlint qbanking-2.1.0-13.x86_64.rpm
> E: qbanking obsolete-not-provided aqhbci-qt-tools
> - I'm not sure you ever ship this, you can drop the obsolete or also provide it.

It's in FE-4. But see above.

> And you really haven't fixed
> > * [ "$RPM_BUILD_ROOT" != "/" ] && rm -rf $RPM_BUILD_ROOT
> > Useless check. Nowadays buildroot cannot be "/" anymore.

Fixed %install before, fixed %clean now.

Uploaded as -14.

(In reply to comment #13)
>  (In reply to comment #12)
> 
> > [deji@agape reviews]$ rpmlint aqbanking-2.1.0-13.x86_64.rpm
> > E: aqbanking obsolete-not-provided aqhbci
> > - aqbanking needs to also 'Provides: aqhbci'
> 
> How does this help? Anything that actually used the old aqhbci will
> not work even if this did provide it - different ABI, different interface,
> different paths to the binaries used. I believe this falls under the '[not] a
> compatible enough replacement' category of the provides/obsoletes draft.
> 
The way I always read Obsolete/Provides is that its meant for people installing
the software, not programs using it. Thus if someone doesn't know aqhbci had
been obsoleted type 'yum install aqhbci', he'll get aqbanking because of Provides.
> 
> Uploaded as -14.
Look like you inadvertently use the %doc macro under aqbanking files section
again, that will clean out the directory before packaging it ;). Just listing
the directory without the %doc will do.

I believe the package is OK after doing that.

Re: %doc - I've tested it, the doc build as it's there now seems to work fine
for me.

I'd be happy to review this package. Here's a review: 

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
See below - License (GPL)
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
712b21f0354d4f890a02da4f8763768b  aqbanking-2.1.0.tar.gz
712b21f0354d4f890a02da4f8763768b  aqbanking-2.1.0.tar.gz.1
See below - BuildRequires correct
OK - Spec handles locales/find_lang
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK - Headers/static libs in -devel subpackage.
OK - Spec has needed ldconfig in post and postun
OK - .pc files in -devel subpackage/requires pkgconfig
OK - .so files in -devel subpackage.
OK - -devel package Requires: %{name} = %{version}-%{release}
OK - .la files are removed.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have subpackages require base package with fully versioned depend.
See below - Should have dist tag
See below - Should package latest version
3 outstanding bugs - check for outstanding bugs on package.

Issues:

1. Minor: Could include COPYING file? Also, possibly:
AUTHORS Changelog NEWS README TODO

2. Possibly a missing BuildRequires:

checking for AccountNumberCheck_new in -lktoblzcheck... no
checking ktoblzcheck.h usability... no
checking ktoblzcheck.h presence... no
checking for ktoblzcheck.h... no

3. According to the COPYING file:
"The banking backend "AqYellowNet" is currently only available binary-only
because of a nondisclosure agreement."
So, should this code just be removed from the source package entirely?
I don't think it's being shipped/linked, but the .so is still in the source.

4. rpmlint says:

a)
E: aqbanking obsolete-not-provided aqhbci
E: aqbanking-devel obsolete-not-provided aqhbci-devel
E: qbanking obsolete-not-provided aqhbci-qt-tools

Suggest: As mentioned earlier in this review, these can probibly be
ignored if it's unlikely that these packages will ever come back at
a later time.

b)
E: aqbanking zero-length /usr/share/aqbanking/bankinfo/us/bic.idx
E: aqbanking-devel zero-length /usr/share/doc/aqbanking-devel-2.1.0/01-OVERVIEW

Suggest: Could possibly remove these? Or ping upstream about it.

c)
W: g2banking no-documentation
W: g2banking-devel no-documentation
W: kbanking no-documentation
W: kbanking-devel no-documentation
W: python-aqbanking no-documentation
W: qbanking no-documentation
W: qbanking-devel no-documentation

Suggest: ignore.

5. Minor: use dist tag?

6. This is an old version... upstream is at 2.2.8.
Any reason not to upgrade to that version?

7. 3 outstanding bugs, might look at the multilib conflicts and see if they
are solveable at this time?

(In reply to comment #16)
> Issues:
> 
> 1. Minor: Could include COPYING file? Also, possibly:
> AUTHORS Changelog NEWS README TODO

Should be in there - see the shenanigans in %install.

> 2. Possibly a missing BuildRequires:
> 
> checking for AccountNumberCheck_new in -lktoblzcheck... no
> checking ktoblzcheck.h usability... no
> checking ktoblzcheck.h presence... no
> checking for ktoblzcheck.h... no

Not shipped in Core/Extras. If someone wants to maintain it, I can
add a buildreq, but I'm not really interested.

> 3. According to the COPYING file:
> "The banking backend "AqYellowNet" is currently only available binary-only
> because of a nondisclosure agreement."
> So, should this code just be removed from the source package entirely?
> I don't think it's being shipped/linked, but the .so is still in the source.

*shrug* We could. It's not like MP3 or something where we remove it so we're not
violating any license.

> 4. rpmlint says:
..
> b)
> E: aqbanking zero-length /usr/share/aqbanking/bankinfo/us/bic.idx
> E: aqbanking-devel zero-length /usr/share/doc/aqbanking-devel-2.1.0/01-OVERVIEW
> 
> Suggest: Could possibly remove these? Or ping upstream about it.

Upstream poked.

> 5. Minor: use dist tag?

It changes ABI, so it's unlikely to be rebased between releases. But it could
be added if needed later.

> 6. This is an old version... upstream is at 2.2.8.
> Any reason not to upgrade to that version?

Want to get stack reviewed, then upgrade stack.

> 7. 3 outstanding bugs, might look at the multilib conflicts and see if they
> are solveable at this time?

205589 and 228321 are both solved in this package with the split into separate
packages. 212518 will be solved with an upgrade.

1. Ah, yeah, I see all of those now except for NEWS. Not sure how useful that
file is really, so it's a pretty minor item. 

2. ok. 

3. True, as long as nothing links to that binary library, it shouldn't matter. 
Can you confirm that the 'yellownet.so*' isn't linking to that binary module?

4. ok, thanks. Note that this might be fixed in an updated version already. 

5. ok. 

6. ok. Fair enough. 

7. Excellent. Sounds good. 

So, the only outstanding issues are including the NEWS file if you want and to
doublecheck and make sure the yellownet.so* files are never linking against the
binary only yellow .so thats shipped with the package. 

I'll go ahead and APPROVE this package now. If you could check the library and
address the NEWS file before importing that would be great. 

Looked at yellownet.so; it does not reference any symbols from that library.

NEWS file added in CVS.

This is built now.

Package Change Request
======================
Package Name: aqbanking
New Branches: EL-4 EL-5

CVS done.

BTW, there's no need to reopen bugs to make CVS requests; we only query for the
flag state.