This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 207782 - Review Request: itpp - C++ library for math, signal/speech processing, and communications
Review Request: itpp - C++ library for math, signal/speech processing, and co...
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-22 23:55 EDT by Ed Hill
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-10-13 11:27:07 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Ed Hill 2006-09-22 23:55:21 EDT
Spec URL: http://mitgcm.org/eh3/fedora_misc/itpp.spec
SRPM URL: http://mitgcm.org/eh3/fedora_misc/itpp-3.10.5-1.fc5.src.rpm
Description: 
IT++ is a C++ library of mathematical, signal processing, speech
processing, and communications classes and functions.  The kernel of
the IT++ library is built upon templated vector and matrix classes
with many lots of functions for their manipulation.  Such a kernel
makes IT++ similar to Octave.  IT++ makes extensive use of existing
open-source libraries (but not only) for increased functionality,
speed and accuracy.
Comment 1 Ralf Corsepius 2006-09-23 01:14:29 EDT
Some initial comments:

MUST FIX:
* Please explain in detail, why you want to include a static lib 
or disable it (--disable-static). Feel strongly encouraged to do the latter.

* /usr/bin/itpp-config is pretty much bugged and needs further treatment:
At minimum, please remove all traces of /usr/include, /usr/lib and (!) 
-L/usr/lib/gcc* in return values.

* Something is broken with the *-doc package:
# rpm -qlp itpp-doc-3.10.5-1.i386.rpm
/usr/share/doc/itpp-3.10.5
/usr/share/doc/itpp-3.10.5/AUTHORS
/usr/share/doc/itpp-3.10.5/ChangeLog
/usr/share/doc/itpp-3.10.5/INSTALL
/usr/share/doc/itpp-3.10.5/NEWS
/usr/share/doc/itpp-3.10.5/README
/usr/share/doc/itpp-3.10.5/TODO

I haven't tried to investigate, yet, but would guess on some issue related
_docdir and/or doxygen.

COULD FIX:
* rpmlint complains:
E: itpp-debuginfo script-without-shellbang
/usr/src/debug/itpp-3.10.5/itpp/base/itpp_version.h

[Header is executable. Not a real issue, can be ignored]


CONSIDER:
* --disable-dependency-tracking. This package is large, so it should have a
measureable impact on building ;)
Comment 2 Ralf Corsepius 2006-09-23 01:16:22 EDT
MUST FIX:
* *-devel contains a *.pc
=> According to the Packaging Guidelines, *-devel MUST Require: pkgconfig
Comment 3 Ed Hill 2006-09-24 10:43:16 EDT
Hi Ralf, heres an update where I've tried to address all of the items in 
the two above comments:

  http://mitgcm.org/eh3/fedora_misc/itpp-3.10.5-3.src.rpm

Also, I found that the --disable-dependency-tracking shaved the build
time by 5% on my otherwise inactive laptop (6.33min vs. 6.02min).  I 
didn't add it to the spec file this time but probably will soon.
Comment 4 Ralf Corsepius 2006-09-26 08:22:30 EDT
(In reply to comment #3)

Two minor issues:

1. 
# rpm -q --requires -p itpp-doc-3.10.5-3.fc5.i386.rpm | grep itpp
itpp = 3.10.5-3.fc5

AFAICT, itpp-doc contains devel-docs. Therefore, it probably should better
"require: itpp-devel" instead of itpp.

2. Something seems fishy with the configure script wrt. atlas:
On one hand it uses /usr/lib/atlas/lib* and /usr/include/atlas, on the other
hand, it reports:
...
External libs:
  - BLAS ........... : yes
    * MKL .......... : no
    * ACML ......... : no
    * ATLAS ........ : no
...

... I must be missing something ;)


and a major one:

IMO, the *.pc is still bugged. It contains this:
...
Libs: -L${libdir} -litpp -lfftw3 -llapack -latlas -lblas   -L/usr/lib/atlas
-L/usr/lib/gcc/i386-redhat-linux/4.1.1
-L/usr/lib/gcc/i386-redhat-linux/4.1.1/../../.. -lgfortranbegin -lgfortran -lm
-lgcc_s
...

The references to -L/usr/lib/gcc/i386-redhat-linux and lgcc_s definitely are
bugs. They must not be present inside of a *.pc.

I am not sufficiently familiar with gfortran (My last real encounter with
fortran dates back more than 15 years) to be able to judge on -lgfortranbegin
and -lgfortran, but I am inclined to think the same apply to them.
Comment 5 Mamoru TASAKA 2006-10-06 11:06:12 EDT
Well, what is the status of this review?
Comment 6 Ed Hill 2006-10-06 11:39:28 EDT
Hi Mamoru, the status is currently FE-NEW.

Ralf posted some helpful comments (thanks, Ralf!) and I'm working on 
them.  I intend to push an updated SRPM this weekend that fixes the 
*.pc library bits per comment #4.  And I'm hoping someone will sign on 
as a reviewer and help finish the process...  :-)
Comment 7 Mamoru TASAKA 2006-10-06 11:47:05 EDT
(In reply to comment #6)
> Hi Mamoru, the status is currently FE-NEW.
> 
> I intend to push an updated SRPM this weekend that fixes the 
> *.pc library bits per comment #4.  

Okay, I understand.
Comment 8 Ed Hill 2006-10-07 17:49:45 EDT
Hi folks, here's an update:

  http://mitgcm.org/eh3/fedora_misc/itpp-3.10.5-4.src.rpm

It addresses the following things:

 + sanitizes itpp.pc
 + --disable-dependency-tracking for ~5% faster builds (woohoo!)
 + fixes the macro-in-changelog rpmlint error

And I'm not going to make -doc depend on -devel (as per comment #4) 
since (1) there is no actual dependency (that is, its a perfectly 
viable devel package without the docs and vice-versa), (2) it causes 
rpmlint errors, and (3) I think it violates the principle of least 
surprise for end users.
Comment 9 Mamoru TASAKA 2006-10-08 12:30:41 EDT
Well, from my viewpoint:

1. From http://fedoraproject.org/wiki/Packaging/Guidelines :

* rpmlint
  - rpmlint is not silent.

E: itpp-debuginfo script-without-shebang
/usr/src/debug/itpp-3.10.5/itpp/base/itpp_version.h
    - This is because the permission of this file is incorrect.

W: itpp-devel no-documentation
    - I think this can be ignored.

W: itpp undefined-non-weak-symbol /usr/lib/libitpp.so.2.2.0
_ZTVN10__cxxabiv117__class_type_infoE
W: itpp undefined-non-weak-symbol /usr/lib/libitpp.so.2.2.0
_ZTISt13basic_fstreamIcSt11char_traitsIcEE
W: itpp undefined-non-weak-symbol /usr/lib/libitpp.so.2.2.0
_ZNSt13basic_fstreamIcSt11char_traitsIcEED1Ev
W: itpp undefined-non-weak-symbol /usr/lib/libitpp.so.2.2.0
_ZNSt13basic_fstreamIcSt11char_traitsIcEED0Ev
W: itpp undefined-non-weak-symbol /usr/lib/libitpp.so.2.2.0
_ZThn8_NSt13basic_fstreamIcSt11char_traitsIcEED1Ev
W: itpp undefined-non-weak-symbol /usr/lib/libitpp.so.2.2.0
_ZThn8_NSt13basic_fstreamIcSt11char_traitsIcEED0Ev
W: itpp undefined-non-weak-symbol /usr/lib/libitpp.so.2.2.0
_ZTv0_n12_NSt13basic_fstreamIcSt11char_traitsIcEED1Ev
W: itpp undefined-non-weak-symbol /usr/lib/libitpp.so.2.2.0
_ZTv0_n12_NSt13basic_fstreamIcSt11char_traitsIcEED0Ev
       ..... (continued)
     - Linkage is incorrect. You can check this by:
       $ ldd -r /usr/lib/libitpp.so.2.2.0
       Some people say that this is not a blocker, while
       other perple say this is a blocker. My opinion is, since this is a library
       and is thought to be used by other package, this warning IS a blocker for
       this package.

* Requires:
  - Check the Requires for -devel packages (see the section Requires of
    http://fedoraproject.org/wiki/Packaging/Guidelines: For -devel package,
    dependency for the package should be checked manually).

* BuildRequies:
  - redundant BuildRequires is found.
    * perl (included in mimimal buildroot)
    * tetex, tetex-dvips <- required by tetex-latex

* Timestamps
  - cp AUTHORS ChangeLog NEWS README TODO \
      $RPM_BUILD_ROOT/%{_docdir}/%{name}-%{version}
    * Use "cp -p"

2. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :
* MUST: If (and only if) the source package includes the text of the license(s)...
  - Include "COPYTING" in main package. This is a MUST item.
Comment 10 Ed Hill 2006-10-08 13:18:39 EDT
Hi Mamoru, thanks for the feedback.  I'll fix the license inclusion, the 
redundant BRs and the time-stamp issue and will post another SRPM.

So how did you get rpmlint to produce all the undefined-non-weak-symbol
warnings?  When I run rpmlint against all of the rpms generated here on 
my system, I only get these two lines:

  W: itpp-devel no-documentation
  E: itpp-debuginfo script-without-shebang 
     /usr/src/debug/itpp-3.10.5/itpp/base/itpp_version.h

which, in my opinion, are both safe to ignore.
Comment 11 Mamoru TASAKA 2006-10-08 13:58:51 EDT
(In reply to comment #10)
> Hi Mamoru, thanks for the feedback.  I'll fix the license inclusion, the 
> redundant BRs and the time-stamp issue and will post another SRPM.
> 
> So how did you get rpmlint to produce all the undefined-non-weak-symbol
> warnings?  

Try: "rpmlint itpp (with itpp installed)". This rpmlint can be gained
only when used for installed rpms. See:
https://www.redhat.com/archives/fedora-extras-list/2006-September/msg00825.html

Well, for now these warning can be ignored because I tried to link
against libitpp.so and it succeeded so this is NOT a blocker.

[tasaka1@localhost PROGRAM]$ cat itpp-check.cpp 
int main(){
  return 0;
}
[tasaka1@localhost PROGRAM]$ g++ -Wall -O2 -o itpp-check itpp-check.cpp
`itpp-config --libs` -L/usr/lib/atlas

However, I recommend that you report this to upstream.
Anyway, fix the dependency for -devel package.

> When I run rpmlint against all of the rpms generated here on 
> my system, I only get these two lines:
> 
>   W: itpp-devel no-documentation
>   E: itpp-debuginfo script-without-shebang 
>      /usr/src/debug/itpp-3.10.5/itpp/base/itpp_version.h
> 
> which, in my opinion, are both safe to ignore.

The first one (no-documentation issue) can be ignored, I think too.
The latter one is because:
-rwxr-xr-x    1 root    root  1542 Jul 12 18:33
/usr/src/debug/itpp-3.10.5/itpp/base/itpp_version.h
The permission should be 0644, not 0755.

Comment 12 Ed Hill 2006-10-08 14:15:04 EDT
Yes, I agree that the "ldd -r ..." business is not a blocker because I'm 
building custom software that uses the itpp header and libraries and it 
doesn't just compile and link -- it actually runs.  ;-)

And please be more explicit about your "fix the dependency for -devel 
package" comment.  The SRPM already has the line:

  Requires:       %{name} = %{version}-%{release}

for the -devel sub-package so what exactly needs to be fixed?
Comment 13 Mamoru TASAKA 2006-10-08 14:18:17 EDT
Some remark:

> [tasaka1@localhost PROGRAM]$ cat itpp-check.cpp 
> int main(){
>   return 0;
> }
> [tasaka1@localhost PROGRAM]$ g++ -Wall -O2 -o itpp-check itpp-check.cpp
> `itpp-config --libs` -L/usr/lib/atlas

The reason I had to add "-L/usr/lib/atlas" is that liblapack.so is
under /usr/lib/atlas but you deleted "-L/usr/lib/atlas" from
itpp-config and itpp.pc. "-L/usr/lib/atlas" is actually needed so
re-add this.

> Anyway, fix the dependency for -devel package.
What I mean is:
$ itpp-config --libs
-litpp -lfftw3 -llapack -latlas -lblas -lgfortranbegin -lgfortran -lm -lgcc_s
(as said in above, -L/usr/lib/atlas should be added).
This means that itpp-devel should also require
fftw-devel atlas-devel and gcc-gfortran .
Comment 14 Rex Dieter 2006-10-08 14:42:08 EDT
Re: comment #12
> Yes, I agree that the "ldd -r ..." business is not a blocker

Means there are undefined symbols, preventing prelink from functioning.  This
really should be fixed.

Re: comment #13
> deps for -devel
Related to shared lib undefined symbols, the *library* ought to link against all
those things, not itpp-using apps, they ought need only:
-litpp
Comment 15 Ed Hill 2006-10-08 15:33:05 EDT
Here's a re-spin:

  http://mitgcm.org/eh3/fedora_misc/itpp-3.10.5-5.src.rpm

It should address everything mentioned in this review except the unresolved 
symbols.  I just don't have the time this weekend to delve into them.  If 
someone (anyone?) wants to submit patches to do the linking then by all 
means please do so!  :-)
Comment 16 Mamoru TASAKA 2006-10-09 11:50:27 EDT
(In reply to comment #14)
> Means there are undefined symbols, preventing prelink from functioning.  This
> really should be fixed.

Well, I have found that undefined non-weak symbols are in libstdc++.so.
Actually, unless this is fixed, I cannot compile the following:

[tasaka1@localhost PROGRAM]$ g++ -O2 -o itpp-check itpp-check.cpp -litpp -nostdlib 
[tasaka1@localhost PROGRAM]$ cat itpp-check.cpp
int main(){
  return 0;
}
[tasaka1@localhost PROGRAM]$ g++ -O2 -o itpp-check itpp-check.cpp -litpp -nostdlib
/usr/bin/ld: warning: cannot find entry symbol _start; defaulting to 080481c0
/usr/lib/gcc/i386-redhat-linux/4.1.1/../../../libitpp.so: undefined reference to
`std::runtime_error::runtime_error(std::basic_string<char,
std::char_traits<char>, std::allocator<char> > const&)'
/usr/lib/gcc/i386-redhat-linux/4.1.1/../../../libitpp.so: undefined reference to
`std::basic_ostringstream<char, std::char_traits<char>, std::allocator<char>
>::~basic_ostringstream()'
/usr/lib/gcc/i386-redhat-linux/4.1.1/../../../libitpp.so: undefined reference to
`virtual thunk to std::basic_iostream<char, std::char_traits<char>
>::~basic_iostream()'
..........

> Re: comment #13
> > deps for -devel
> Related to shared lib undefined symbols, the *library* ought to link against all
> those things, not itpp-using apps, they ought need only:
> -litpp

Explicit linking against external libraries are always needed when
header files in the library use some types, 
structure or so which are defined by other packages then the
header files include other external header files.
( I have not checked if this package falls under this case).
Comment 17 Ralf Corsepius 2006-10-09 12:01:20 EDT
(In reply to comment #16)
> (In reply to comment #14)
> > Means there are undefined symbols, preventing prelink from functioning.  This
> > really should be fixed.
> 
> Well, I have found that undefined non-weak symbols are in libstdc++.so.
> Actually, unless this is fixed, I cannot compile the following:
> 
> [tasaka1@localhost PROGRAM]$ g++ -O2 -o itpp-check itpp-check.cpp -litpp
-nostdlib 

1. Why do you expect this to work? -nostdlib disables GCC's internal libs (such
a gcc_s, stdc++), so this is a non-issue.

2. There is nothing wrong in using undefined non-weak symbols. If this causes
prelink to fail, then prelink is broken ...
Comment 18 Mamoru TASAKA 2006-10-09 12:24:12 EDT
(In reply to comment #17)
> > [tasaka1@localhost PROGRAM]$ g++ -O2 -o itpp-check itpp-check.cpp -litpp
> -nostdlib 
> 
> 1. Why do you expect this to work? -nostdlib disables GCC's internal libs (such
> a gcc_s, stdc++), so this is a non-issue.
As you said, I _EXPLICITLY_ disabled default linkage provided by gcc and
this should success if libitpp.so is linked against libstdc++.so.6 properly.

> 2. There is nothing wrong in using undefined non-weak symbols. If this causes
> prelink to fail, then prelink is broken ...

I am not familiar with prelink mechanism, however explanation by Jakub:
http://sources.redhat.com/ml/libc-alpha/2003-05/msg00034.html
perhaps means that undefined non-weak symbols can cause some problems
with prelink.

Comment 19 Mamoru TASAKA 2006-10-09 12:42:48 EDT
(In reply to comment #18)
> (In reply to comment #17)
> > > [tasaka1@localhost PROGRAM]$ g++ -O2 -o itpp-check itpp-check.cpp -litpp
> > -nostdlib 
> > 
> > 1. Why do you expect this to work? -nostdlib disables GCC's internal libs (such
> > a gcc_s, stdc++), so this is a non-issue.
> As you said, I _EXPLICITLY_ disabled default linkage provided by gcc and
> this should success if libitpp.so is linked against libstdc++.so.6 properly.

Oops.. This breaks start up symbol, however, linkage aganst libstdc++.so.6
is broken, anyway.

Comment 20 Ralf Corsepius 2006-10-09 12:46:53 EDT
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > > [tasaka1@localhost PROGRAM]$ g++ -O2 -o itpp-check itpp-check.cpp -litpp
> > > -nostdlib 
> > > 
> > > 1. Why do you expect this to work? -nostdlib disables GCC's internal libs
(such
> > > a gcc_s, stdc++), so this is a non-issue.
> > As you said, I _EXPLICITLY_ disabled default linkage provided by gcc and
> > this should success if libitpp.so is linked against libstdc++.so.6 properly.
> 
> Oops.. This breaks start up symbol, however, linkage aganst libstdc++.so.6
> is broken, anyway.
Nope, you're simply trying to overengineer a non-issue.
Comment 21 Mamoru TASAKA 2006-10-09 13:29:06 EDT
(In reply to comment #20)
> (In reply to comment #19)
> > Oops.. This breaks start up symbol, however, linkage aganst libstdc++.so.6
> > is broken, anyway.
> Nope, you're simply trying to overengineer a non-issue.

Well, I don't this is a non-issue. At least, this is unwilling
problem. And this undefined non-weak symbols problem disappeard 
by linking against libstdc++.so.6 ( I confirmed ) , so the fix should be
done as such.

[tasaka1@localhost ~]$ ldd -r /usr/lib/libitpp.so
        linux-gate.so.1 =>  (0x004ba000)
        libfftw3.so.3 => /usr/lib/libfftw3.so.3 (0x00eb7000)
        liblapack.so.3 => /usr/lib/atlas/liblapack.so.3 (0x0064a000)
        libatlas.so.3 => /usr/lib/atlas/libatlas.so.3 (0x00f49000)
        libblas.so.3 => /usr/lib/atlas/libblas.so.3 (0x07673000)
        libgfortran.so.1 => /usr/lib/libgfortran.so.1 (0x003da000)
        libm.so.6 => /lib/libm.so.6 (0x00b1c000)
        libgcc_s.so.1 => /lib/libgcc_s.so.1 (0x00459000)
        libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x004bb000)
        libc.so.6 => /lib/libc.so.6 (0x00b98000)
        libpthread.so.0 => /lib/libpthread.so.0 (0x00465000)
        /lib/ld-linux.so.2 (0x00b7d000)

( after fix, no undefined non-weak symbol appears any more )
Comment 22 Ed Hill 2006-10-09 16:14:51 EDT
I agree with Ralf in comment #20.

If you (Mamoru) really want to straighten out this linkage issue
then please work with upstream to get it into their regular build 
system.

So, getting back to the actual review, are there any blockers left 
here?
Comment 23 Mamoru TASAKA 2006-10-10 10:02:37 EDT
(In reply to comment #22)
> I agree with Ralf in comment #20.
> 
> If you (Mamoru) really want to straighten out this linkage issue
> then please work with upstream to get it into their regular build 
> system.

?? You want to become the maintainer of this package, don't you?
Then it is you who should report this problem (undefined non-weak
problem) to upsteam because this problem is surely a _BUG_ of this
package and _YOU_ must fix this when someone opens bugzilla 
entry about this issue. Indeed in other review request I asked the
submitter to contact upstream to fix some problem.
Comment 24 Denis Leroy 2006-10-10 10:28:31 EDT
Quick note on undefined-non-weak-symbols: these warnings can be hard to get rid
of sometimes. If no easy fix is found, i think it's better to ignore the
warnings rather than introduce traumatic patches that might cause other
problems. You'd want to at least verify that the package works on all arches,
i.e. link test examples to make sure... And yes, package maintainer should
report the problem upstream :-)
Comment 25 Ralf Corsepius 2006-10-10 11:03:11 EDT
I am going to reiterate myself: There is no real problem in this case.
Comment 26 Rex Dieter 2006-10-10 11:07:38 EDT
Ralf, thanks for the opinion, but many (including me and the reviewer) disagree.
Comment 27 Mamoru TASAKA 2006-10-10 11:14:30 EDT
(In reply to comment #22)
> So, getting back to the actual review, are there any blockers left 
> here?

Well,
* %{_datadir}/%{name}/ is not owned by any package. Main package
  should own this.

(In reply to comment #24)
> these warnings can be hard to get rid of sometimes.
For this package, this can be resolved by following:
------------------------------------
.................
%configure --with-blas="-latlas -lblas" --disable-dependency-tracking  \
  --with-docdir=%{_docdir}/%{name}-%{version} --disable-static

sed -i -e 's|-lgcc_s|-lgcc_s -lstdc++|' config.status
./config.status

make %{?_smp_mflags}
.................
------------------------------------
  ... and as this is actually a bug and can be fixed as above, this bug
  should be fixed before releasing in FE.

* Well, 
  
(In reply to comment #16)
> 
> > Re: comment #13
> > > deps for -devel
> > Related to shared lib undefined symbols, the *library* ought to link against all
> > those things, not itpp-using apps, they ought need only:
> > -litpp
> 
> Explicit linking against external libraries are always needed when
> header files in the library use some types, 
> structure or so which are defined by other packages then the
> header files include other external header files.
> ( I have not checked if this package falls under this case).

As I said, explicit linking is needed when 
header files includes other header files which are in other packages.
I checked this package by:

 ( for f in `rpm -ql itpp-devel` ; do cat $f 2>/dev/null | grep '^#[
\t]*include' | sed -e 's|^[ \t][ \t]*||' | sed -n -e 's|.*<\(.*\)>.*|\1|p' ;
done ) | sort | uniq 

and it seems it is not a case for this package.
Then as Rex said in the comment #14, I  doubt that 
itpp.pc and itpp-config should have:
"-lfftw3 -llapack -latlas -lblas   -L/usr/lib/atlas -lgfortranbegin -lgfortran
-lm -lgcc_s -lstdc++" .
Please check if this linkage is truly required.
Comment 28 Ralf Corsepius 2006-10-10 11:16:03 EDT
(In reply to comment #26)
> Ralf, thanks for the opinion, but many (including me and the reviewer) disagree.

OK, Rex, elaborate which problems this issue causes in this particular case and
what it breaks.
Comment 29 Ralf Corsepius 2006-10-10 11:21:55 EDT
(In reply to comment #27)
> (In reply to comment #22)
>
> (In reply to comment #24)
> > these warnings can be hard to get rid of sometimes.
> For this package, this can be resolved by following:
> ------------------------------------
> .................
> %configure --with-blas="-latlas -lblas" --disable-dependency-tracking  \
>   --with-docdir=%{_docdir}/%{name}-%{version} --disable-static
> 
> sed -i -e 's|-lgcc_s|-lgcc_s -lstdc++|' config.status
> ./config.status
Dead wrong - Never ever patch config.status.


Comment 30 Rex Dieter 2006-10-10 11:40:45 EDT
> Dead wrong - Never ever patch config.status.
Agreed.  There needs to be a better/cleaner solution.
Comment 31 Rex Dieter 2006-10-10 11:42:18 EDT
> OK, Rex, elaborate which problems this issue causes in this particular 
> case and what it breaks.

Undefined symbols in shared libraries is bad, period, especially if easily
avoidable (which it apparently is in this case).
Comment 32 Ralf Corsepius 2006-10-10 11:49:36 EDT
(In reply to comment #31)
> > OK, Rex, elaborate which problems this issue causes in this particular 
> > case and what it breaks.
> 
> Undefined symbols in shared libraries is bad, period, especially if easily
> avoidable (which it apparently is in this case).
This is your personal preference on a particular design, but this doesn't cause
any malfunction.

glibc (c.f. man 3 getopt), libstdc++ and many other libs are heavily using such
constructs.
 
Comment 33 Mamoru TASAKA 2006-10-10 12:35:30 EDT
(In reply to comment #28)
> (In reply to comment #26)
> > Ralf, thanks for the opinion, but many (including me and the reviewer) disagree.
> 
> OK, Rex, elaborate which problems this issue causes in this particular case and
> what it breaks.
> 

The main problem for undefined non-weak symbols is that this may produce
symbols conflict. 
As you can see by "rpm -q --requires itpp-3.10.5-5.fc6", this package
(without this problem fixed) does NOT require libstdc++.so.6 (because
libitpp.so.2.2.0 is not linked against it) although it surely require it.
So this package can be installed which uses libstdc++.so.5, for example,
which surely cause symbol conflicts.

Comment 34 Denis Leroy 2006-10-10 12:51:09 EDT
> As you can see by "rpm -q --requires itpp-3.10.5-5.fc6", this package
> (without this problem fixed) does NOT require libstdc++.so.6 (because
> libitpp.so.2.2.0 is not linked against it) although it surely require it.
> So this package can be installed which uses libstdc++.so.5, for example,
> which surely cause symbol conflicts.

Assuming a system that only has compat-libstdc++-33 installed but not libstdc++.
However nothing prevents you from manually adding the Requires: in the package.


Comment 35 Mamoru TASAKA 2006-10-10 12:55:26 EDT
(In reply to comment #34)
> Assuming a system that only has compat-libstdc++-33 installed but not libstdc++.
> However nothing prevents you from manually adding the Requires: in the package.

That I said "libstdc++.so.5" is only a example. The conflict can also occur
when soname of libstdc++ changes.
Comment 36 Mamoru TASAKA 2006-10-10 12:59:24 EDT
(In reply to comment #29)
> (In reply to comment #27)
> > (In reply to comment #22)
> >
> > (In reply to comment #24)
> Dead wrong - Never ever patch config.status.

If you say so, another example is

%build
chmod 644 itpp/base/itpp_version.h

for f in `find . -name Makefile.in` ; do
  sed -i -e '/^LIBS/s|^\(.*\)$|\1\nLIBS += -lstdc++|' $f
done

export LDFLAGS="-L/usr/lib/atlas"
export CPPFLAGS="-I/usr/include/atlas"
export F77=gfortran
...........

Comment 37 Ralf Corsepius 2006-10-10 13:01:58 EDT
(In reply to comment #36)

> for f in `find . -name Makefile.in` ; do
>   sed -i -e '/^LIBS/s|^\(.*\)$|\1\nLIBS += -lstdc++|' $f
> done

The same applies to Makefile.in: Never ever patch them.
Comment 38 Ralf Corsepius 2006-10-10 13:03:36 EDT
Sorry, forgot: Never ever explicitly link against libstdc++.
Comment 39 Mamoru TASAKA 2006-10-10 13:08:20 EDT
(In reply to comment #38)
> Sorry, forgot: Never ever explicitly link against libstdc++.

Explicit linking is needed because libtool (in this package) uses
-nostdlib.
Comment 40 Mamoru TASAKA 2006-10-10 13:18:35 EDT
(In reply to comment #38)
> Sorry, forgot: Never ever explicitly link against libstdc++.

Note: explicit linking against libgcc_s.so is done already (as -nostdlib
is used).
Comment 41 Ed Hill 2006-10-10 14:17:19 EDT
Hi Mamoru, thank you for pointing out the unowned dir in comment #27 and 
here's an update that fixes it:

  http://mitgcm.org/eh3/fedora_misc/itpp-3.10.5-6.src.rpm


In regards to the undefined symbols, I tried using:

  export LDFLAGS="-L/usr/lib/atlas -stdc++"

and it appears to do the trick.  However, it violates Ralf's assertion in 
comment #38 so I did not (for the above SRPM) add it to the spec.  Of 
course, its something that we can revisit.

So, are there any blockers left here?
Comment 42 Ed Hill 2006-10-10 14:29:07 EDT
Theres a missing character in the above comment.  It should have been:

  export LDFLAGS="-L/usr/lib/atlas -lstdc++"
Comment 43 Mamoru TASAKA 2006-10-11 10:58:49 EDT
(In reply to comment #41)

> In regards to the undefined symbols, I tried using:
> 
>   export LDFLAGS="-L/usr/lib/atlas -stdc++"
> 
> and it appears to do the trick.  However, it violates Ralf's assertion in 
> comment #38 so I did not (for the above SRPM) add it to the spec.  

This case explicit linking by "-lstdc++" is actually needed
( as I said in comment #39 and #40 ) because this package used "-nostdlic"
when linking (this is not unusual). So your linking

export LDFLAGS="-L/usr/lib/atlas -lstdc++"

is acceptable and must be done. Please add "-lstdc++".

-------

Then as I said in comment #27 ( and Rex said in comment #14 ), would you check
if "-lfftw3 -llapack -latlas -lblas   -L/usr/lib/atlas   -lgfortranbegin
-lgfortran -lm -lgcc_s -lstdc++" is really needed for
/usr/lib/itpp.pc and /usr/bin/itpp-config ?
I checked all header files in -devel package and no files are included from
external packages. If linkage is fixed by "-lstdc++", I think the above
("-lfftw3 ........" ) is  not necessary ( and should be deleted ).
Comment 44 Mamoru TASAKA 2006-10-11 11:07:59 EDT
(In reply to comment #43)
 
> Then as I said in comment #27 ( and Rex said in comment #14 ), would you check
> if "-lfftw3 -llapack -latlas -lblas   -L/usr/lib/atlas   -lgfortranbegin
> -lgfortran -lm -lgcc_s -lstdc++" is really needed for
> /usr/lib/itpp.pc and /usr/bin/itpp-config ?

Of course  -L${libdir} -litpp is needed anyway.
Comment 45 Ed Hill 2006-10-11 22:36:19 EDT
OK, this adds "-lstc++":

  http://mitgcm.org/eh3/fedora_misc/itpp-3.10.5-7.src.rpm

and both itpp-config and /usr/lib/pkgconfig/itpp.pc appear to work correctly
for me.
Comment 46 Mamoru TASAKA 2006-10-12 12:30:15 EDT
(In reply to comment #45)
> and both itpp-config and /usr/lib/pkgconfig/itpp.pc appear to work correctly
> for me.

Yes, it should work correctly. I just wonder whether
"-lfftw3 -llapack -latlas -lblas   -L/usr/lib/atlas   -lstdc++ -lgfortranbegin
-lgfortran -lm -lgcc_s" is REALLY needed for the two file (itpp-config &
itpp.pc) .

"-lfftw3....." is really needed when compiling libitpp.so (i.e., in
building this package).  However, once
linking is corectly done (as in -7 src.rpm) and since no header files include
header files in other packages, including "-lfftw3".... to itpp-config &
itpp.pc is perhaps redundant.

In short, I think that itpp.pc can be:
----------------------------------------------------------
prefix=/usr
exec_prefix=/usr
libdir=/usr/lib
includedir=/usr/include

Name: IT++
Description: IT++ is a C++ library of mathematical, signal processing, speech
processing, and communications classes and fu
nctions
Version: 3.10.5
URL: http://itpp.sourceforge.net/
Libs: -L${libdir} -litpp
Cflags: 
--------------------------------------------------------------------------

and itpp-config can be:
--------------------------------------------------------------------------
...............................
    --libs)
      echo -L${libdir} -litpp 
      ;;
    --libs-opt)
      echo -L${libdir} -litpp 
      ;;
    --libs-debug)
      echo -L${libdir} -litpp 
      ;;
........................................................
------------------------------------------------------------------------------

Please check if this is possible. Other things are okay.
Comment 47 Ed Hill 2006-10-12 13:53:53 EDT
Yes, but is it a _blocker_ for this review?

The points I'm trying to make are that (1) I'm rather confident that it 
works safely and correctly as it is currently packaged in -7, (2) I'm 
not 100% certain that what you suggest will work in all cases [how can 
we be certain?] and (3) what you suggest is a further deviation from 
the way upstream does things and therefore it should be done cautiously.

So, I prefer to leave it as-is.
Comment 48 Mamoru TASAKA 2006-10-12 14:04:16 EDT
(In reply to comment #47)
> Yes, but is it a _blocker_ for this review?
> 
> The points I'm trying to make are that 
> (1) I'm rather confident that it 
> works safely and correctly as it is currently packaged in -7, 

It is true.

> (2) I'm 
> not 100% certain that what you suggest will work in all cases [how can 
> we be certain?] 

As I explained, since linkage is now correct and header files are
"consistent", the external linkage should not be necessary.

> (3) what you suggest is a further deviation from 
> the way upstream does things and therefore it should be done cautiously.

Well, please report this argument to upstream. Anyway it is recommended
(I think) that the maintainer in Fedora and upstream has good connection.

> So, I prefer to leave it as-is.

Okay. Then for now I don't block this any longer.

----------------------------------------------------------------------------------------
   This package (itpp) is now APPROVED by me.

Comment 49 Ed Hill 2006-10-12 22:40:32 EDT
Hi Mamoru, thank you for the review!

The -7 version built successfully for devel, an FC-5 branch has been 
requested, and the undefined non-weak symbols issue has been reported 
to upstream:

http://sourceforge.net/tracker/index.php?func=detail&aid=1576333&group_id=37044&atid=418758

so I think that covers everything.
Comment 50 Mamoru TASAKA 2006-10-13 09:54:56 EDT
Thank you for reporting to upstream.

Well, I see you successfully imported itpp to FE-devel.
Now I checked that CVS has itpp FE-5 entry. So please build this against
FE-5. If it is done, you can close this bug.
Comment 51 Kevin Fenzi 2006-12-05 22:38:32 EST
Hey Ed: 

I don't see this package in owners.list. Can you please add it?
See: 

http://fedoraproject.org/wiki/Extras/Contributors#head-f6f080b4c48fe519c98a29364a740953f90179e7
Comment 52 Ed Hill 2006-12-06 08:22:24 EST
Hi Kevin, thank you for pointing out the omission!

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