Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 207782
Summary: | Review Request: itpp - C++ library for math, signal/speech processing, and communications | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ed Hill <ed> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | mtasaka, pertusus |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-10-13 15:27:07 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
Ed Hill
2006-09-23 03:55:21 UTC
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 ;) MUST FIX: * *-devel contains a *.pc => According to the Packaging Guidelines, *-devel MUST Require: pkgconfig 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. (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. Well, what is the status of this review? 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... :-) (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. 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. 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. 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. (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. 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? 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 . 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 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! :-) (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). (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 ... (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. (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. (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. (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 ) 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? (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. 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 :-) I am going to reiterate myself: There is no real problem in this case. Ralf, thanks for the opinion, but many (including me and the reviewer) disagree. (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. (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. (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. > Dead wrong - Never ever patch config.status.
Agreed. There needs to be a better/cleaner solution.
> 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).
(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. (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. > 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.
(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. (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 ........... (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. Sorry, forgot: Never ever explicitly link against libstdc++. (In reply to comment #38) > Sorry, forgot: Never ever explicitly link against libstdc++. Explicit linking is needed because libtool (in this package) uses -nostdlib. (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). 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? Theres a missing character in the above comment. It should have been: export LDFLAGS="-L/usr/lib/atlas -lstdc++" (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 ). (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. 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. (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. 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. (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. 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. 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. 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 Hi Kevin, thank you for pointing out the omission! |