Bug 204954 - Review Request: libofa - Open Fingerprint Architecture library
Review Request: libofa - Open Fingerprint Architecture library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-09-01 13:30 EDT by Rex Dieter
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-12 13:00:25 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Mock build log of libofa with debian gcc patch applied (52.93 KB, text/plain)
2006-09-11 12:40 EDT, Mamoru TASAKA
no flags Details
diff of libofa.spec (946 bytes, patch)
2006-09-11 12:43 EDT, Mamoru TASAKA
no flags Details | Diff
Mock build log of libofa-0.9.3-6 (failed) (51.45 KB, text/plain)
2006-09-12 12:21 EDT, Mamoru TASAKA
no flags Details

  None (edit)
Description Rex Dieter 2006-09-01 13:30:34 EDT
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/libofa.spec
SRPM URL: http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.stable/libofa-0.9.3-1.src.rpm
Description:
Currently, MusicDNS and the Open Fingerprint Architecture are being used to:
* identify duplicate tracks, even when the metadata is different, MusicIP
  identifies the master recording.
* fix metadata
* find out more about tracks by connecting to MusicBrainz
Comment 1 Rex Dieter 2006-09-01 13:44:18 EDT
This is a (new) dependency for libtunepimp-0.5.x
Comment 2 Parag AN(पराग) 2006-09-05 06:23:26 EDT
{Not Official Reviewer}
packaging looks ok in SPEC.
- Mockbuild is FAILED for i386 FC6
JAMA/tnt_math_utils.h:33: error: call of overloaded 'abs(const float&)' is ambiguous
/usr/include/stdlib.h:786: note: candidates are: int abs(int)
/usr/lib/gcc/i386-redhat-linux/4.1.1/../../../../include/c++/4.1.1/cstdlib:172:
note:                 long long int __gnu_cxx::abs(long long int)
/usr/lib/gcc/i386-redhat-linux/4.1.1/../../../../include/c++/4.1.1/cstdlib:142:
note:                 long int std::abs(long int)
make[3]: *** [mainprint.lo] Error 1
make[3]: Leaving directory `/builddir/build/BUILD/libofa-0.9.3/lib'
make[2]: *** [all-recursive] Error 1

+ dist tag is present
+ Buildroot is correct
- source URL is NOT working
+ License used is GPL
+ License file COPYING is included
+ Devel package is handled correclty in SPEC
- No upstream tarball available to verify its MD5

Comment 3 Rex Dieter 2006-09-06 10:46:09 EDT
Crud, I had only previously tested this on rhel4.  Must be a gcc4 thing.  I'll
report it upstream.
Comment 4 Mamoru TASAKA 2006-09-11 08:06:36 EDT
Perhaps debian's patch for lib/JAMA/tnt_math_utils.h works
( I have not yet checked debian's patch)

http://ftp.debian.org/debian/pool/main/libo/libofa/libofa_0.9.3-1.diff.gz
Comment 5 Rex Dieter 2006-09-11 10:00:22 EDT
Thanks, hidden within all the debian-isms in in there, is what appears to be a
gcc41 patch.
Comment 6 Rex Dieter 2006-09-11 12:08:12 EDT
Turns out the patch doesn't seem to help any, build dies similarly as before: 
...
 g++ -DHAVE_CONFIG_H -I. -I. -I.. -I../include -O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
-m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -Wall -g -MT
mainprint.lo -MD -MP -MF .deps/mainprint.Tpo -c mainprint.cpp  -fPIC -DPIC -o
.libs/mainprint.o
JAMA/tnt_math_utils.h: In function 'Real TNT::hypot(const Real&, const Real&)
[with Real = float]':
JAMA/jama_svd.h:73:   instantiated from 'JAMA::SVD<Real>::SVD(const
TNT::Array2D<T>&) [with Real = float]'
mainprint.cpp:151:   instantiated from here
JAMA/tnt_math_utils.h:33: error: call of overloaded 'abs(const float&)' is ambiguous
/usr/include/stdlib.h:786: note: candidates are: int abs(int)
/usr/lib/gcc/i386-redhat-linux/4.1.1/../../../../include/c++/4.1.1/cstdlib:172:
note:                 long long int __gnu_cxx::abs(long long int)
/usr/lib/gcc/i386-redhat-linux/4.1.1/../../../../include/c++/4.1.1/cstdlib:142:
note:                 long int std::abs(long int)
Comment 7 Rex Dieter 2006-09-11 12:18:35 EDT
build failure reported upstream:
http://forums.predixis.com/index.php?showtopic=2196
Comment 8 Mamoru TASAKA 2006-09-11 12:40:26 EDT
Created attachment 136017 [details]
Mock build log of libofa with debian gcc patch applied

Umm?

For me (gcc-4.1.1-20.i386), mock build succeeded.
Comment 9 Mamoru TASAKA 2006-09-11 12:43:38 EDT
Created attachment 136018 [details]
diff of libofa.spec

Diff between original 0.9.3-1 spec file and the one I used.
Comment 11 Mamoru TASAKA 2006-09-11 22:18:49 EDT
(In reply to comment #10)
> Here's what I used that still doesn't build (for me):
> Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/libofa.spec

Yes, surely by your spec file, mock build fails.

When you apply debian's patch by %patch1 -p1, it creates debian/ directory
and debian-original files are made under the directory. You can see 
the "real patch" is created under debian/patches directory.

So next time you have to apply the "real patch" under debian/patches
created by %patch1 -p1. The spec file I used includes:

-----------------------------------
%patch0 -p1 -b .deb
patch -p1 -b --suffix .gcc41 < debian/patches/01_gcc41.diff
-----------------------------------

Comment 12 Rex Dieter 2006-09-11 22:21:22 EDT
Arg, didn't realize that the patch simply created another patch!
Comment 13 Rex Dieter 2006-09-11 22:37:36 EDT
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/libofa.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.testing/libofa-0.9.3-3.src.rpm

%changelog
* Mon Sep 11 2006 Rex Dieter <rexdieter[AT]users.sf.net> 0.9.3-3
- use gcc41 patch extracted from debian's patchset

Comment 14 Mamoru TASAKA 2006-09-12 02:36:43 EDT
Okay. I will review this package.

First review:

1. From http://fedoraproject.org/wiki/Packaging/Guidelines :
* rpmlint
  - is not silent.
    W: libofa wrong-file-end-of-line-encoding /usr/share/doc/libofa-0.9.3/README
    - The following files have wrong (Windows-like) end-of-line
      encoding.

libofa-0.9.3-3.fc6/libofa-0.9.3-gcc41.patch
libofa-0.9.3-3.fc6/usr/share/doc/libofa-0.9.3/README
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/AFLIB/aflibConverter.cpp
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/AFLIB/aflibConverter.h
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/AFLIB/aflibConverterLargeFilter.h
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/AFLIB/aflibConverterSmallFilter.h
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/JAMA/jama_svd.h
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/JAMA/tnt_array1d.h
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/JAMA/tnt_array2d.h
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/JAMA/tnt_math_utils.h
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/fft_op.cpp
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/fftlibw3_op.cpp
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/frametracker_op.cpp
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/frametracker_op.h
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/trackdata_op.cpp
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/trackdata_op.h
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/trackframe_op.cpp
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/trackframe_op.h
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/tracklist_op.cpp
libofa-debuginfo-0.9.3-3.fc6/usr/src/debug/libofa-0.9.3/lib/tracklist_op.h
libofa-devel-0.9.3-3.fc6/usr/include/ofa1/ofa.h

    W: libofa mixed-use-of-spaces-and-tabs (this is srpm)
    W: libofa-devel no-documentation (ignored)

* Requires:
  - Requires for -devel package cannot be supplied automatically, which must be
found by
  manual check.
  libofa-devel-0.9.3-3.fc6/usr/lib/pkgconfig/libofa.pc says:

Requires: fftw3
Libs: -L${libdir} -lofa -lexpat -lm
Cflags: -I${includedir}

  This means that libofa-devel requires fftw3-devel, expat-devel (and
  libofa).

 2. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :
* MUST: The sources used to build the package....
  Umm? I cannot find Source0. I can find .tar.gz.

3. Other things I have noticed:
   = Nothing.
Comment 15 Rex Dieter 2006-09-12 09:06:49 EDT
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/libofa.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.testing/libofa-0.9.3-4.src.rpm

%changelog
* Tue Sep 12 2006 Rex Dieter <rexdieter[AT]users.sf.net> 0.9.3-4
- remove extrenous entries from libofa.pc
- dos2unix README
- fix url in Source0
- -devel: %doc examples/

NOTE: can't change encoding of gcc41.patch, since the file it patches is a DOS
text file.
Comment 16 Paul Howarth 2006-09-12 09:58:17 EDT
(In reply to comment #15)
> %changelog
> * Tue Sep 12 2006 Rex Dieter <rexdieter[AT]users.sf.net> 0.9.3-4
> - remove extrenous entries from libofa.pc
> - dos2unix README
> - fix url in Source0
> - -devel: %doc examples/
> 
> NOTE: can't change encoding of gcc41.patch, since the file it patches is a DOS
> text file.

Actually you could if you did a dos2unix on the file to be patched before
applying the patch.
Comment 17 Rex Dieter 2006-09-12 10:00:19 EDT
> Actually you could if you did a dos2unix on the file to be patched before
> applying the patch.

For no other purpose than to make rpmlint happy?  No thanks.
Comment 18 Mamoru TASAKA 2006-09-12 10:22:59 EDT
Second review:

* rpmlint
  - is not yet silent:

W: libofa macro-in-%changelog doc
W: libofa mixed-use-of-spaces-and-tabs (this is for src.rpm)
W: libofa-devel hidden-file-or-dir /usr/share/doc/libofa-devel-0.9.3/examples/.deps
W: libofa-devel hidden-file-or-dir /usr/share/doc/libofa-devel-0.9.3/examples/.deps

    - Use %%doc, not %doc in %changelog entry.
    - Remove spaces or tabs in spec file and unify spacing.
    - examples/.deps is needed?
  - Well, this wiki page discourages to use unix2dos and actually, unix2dos
    is not needed. Also, changing the end-of-file encodings of souce files
    (i.e. .h or .cpp files) is done by:

    for f in `find . -name README -or -name \*.cpp -or -name \*.h` ; do \
        sed -i -e 's|\r||' $f ; done

    in %prep stage. Then removing DOS-like encoding of patch file (Patch1) can be
    done. Changing the end-of file encoding of \*.cpp, \*.h files is needed
    because they are finally included in debuginfo rpm.

* Well, removing external dependency from .pc file is correct?
Comment 19 Rex Dieter 2006-09-12 11:30:54 EDT
> * Well, removing external dependency from .pc file is correct?

Yeah, I'm pretty sure.  If problems arise, we can always add them back.
Comment 20 Rex Dieter 2006-09-12 11:33:04 EDT
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/libofa.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.testing/libofa-0.9.3-5.src.rpm

%changelog
* Tue Sep 12 2006 Rex Dieter <rexdieter[AT]users.sf.net> 0.9.3-5
- use sed instead of dos2unix
- omit examples/.deps
Comment 21 Rex Dieter 2006-09-12 11:45:16 EDT
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/libofa.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.testing/libofa-0.9.3-6.src.rpm

%changelog
* Tue Sep 12 2006 Rex Dieter <rexdieter[AT]users.sf.net> 0.9.3-6
- de-DOS'ify .cpp, .h files too
Comment 22 Mamoru TASAKA 2006-09-12 12:21:28 EDT
Created attachment 136085 [details]
Mock build log of libofa-0.9.3-6 (failed)

I cannot rebuild -6 src.rpm

+ rm -rf rpmdocs
+ mkdir -p rpmdocs
+ cp -a examples rpmdocs/
+ make -C rpmdocs/examples clean
make: Entering directory `/builddir/build/BUILD/libofa-0.9.3/rpmdocs/examples'
cd .. && make  am--refresh
make[1]: Entering directory `/builddir/build/BUILD/libofa-0.9.3/rpmdocs'
make[1]: *** No rule to make target `am--refresh'.  Stop.
make[1]: Leaving directory `/builddir/build/BUILD/libofa-0.9.3/rpmdocs'
make: *** [../configure] Error 2
make: Leaving directory `/builddir/build/BUILD/libofa-0.9.3/rpmdocs/examples'
error: Bad exit status from /var/tmp/rpm-tmp.25351 (%install)
Comment 23 Rex Dieter 2006-09-12 12:24:51 EDT
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/libofa.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.testing/libofa-0.9.3-6.src.rpm

%changelog
* Tue Sep 12 2006 Rex Dieter <rexdieter[AT]users.sf.net> 0.9.3-7
- fix rpmdoc handling

Serves me right for not actually testing the build first. (:
Comment 25 Mamoru TASAKA 2006-09-12 12:46:12 EDT
(In reply to comment #24)
> Make that SRPM URL:
> http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.testing/libofa-0.9.3-7.src.rpm

Okay.

Well, rpmlint complaint for src.rpm still exists.
W: libofa mixed-use-of-spaces-and-tabs
This can be fixed by using only spaces, not tabs. Consider
to use only spaces or tabs.

Aside for it, no problems is left.

--------------------------------------------------------------
       This package (libofa) is APPROVED by me.
Comment 26 Rex Dieter 2006-09-12 13:00:25 EDT
Thanks for the excellent review, importing.

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