Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=225784 Stepan Kasal <skasal@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flag|needinfo?(skasal@xxxxxxxxxx | |) | --- Comment #12 from Stepan Kasal <skasal@xxxxxxxxxx> 2009-04-17 06:23:58 EDT --- Thank you for the review. I have fixed most of the issues, the remaining ones are explained below. Tell me if I missed any. (In reply to comment #11) > gdbm.x86_64: W: shared-lib-calls-exit /usr/lib64/libgdbm.so.2.0.0 > exit@xxxxxxxxxxx > gdbm.x86_64: W: shared-lib-calls-exit /usr/lib64/libgdbm.so.2.0.0 > exit@@GLIBC_2.2.5 > > Should be investigated. exit() is called from _gdbm_fatal() which gets called if a malloc or read fails. The reason why calling exit from a library is a bad idea is that the caller should get a chance to clean up. With gdbm, you can register a hook function that gets called from _gdbm_fatal before the exit. This may not be the best design decision, but it still somehow works. Concerning this and the fact that the code has not changed the last five years, I conclude that is would not be a good idea to try to patch this issue. (At leat until a real life problem related to the issue appears.) > * 1.8.3 was released but not sure whether is it good idea to incorporate it Might be a good idea, but I believe that can be postponed after finishing this review. > Patch0: gdbm-1.8.0-jbj.patch > Patch1: gdbm-1.8.0-fhs.patch > Patch3: gdbm-1.8.0-64offset.patch > > Could be: 0-1-2 I don't think it is advisable to renumber each time a patch is dropped. Consequently, the sequence of the numbers can hardly be maintained. > * discouraged: %makeinstall install-compat > > https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used Quote: "%makeinstall macro [...] must NOT be used when make install DESTDIR=%{buildroot} works" grep -r DESTDIR gdbm-1.8.0 returns nothig. Most projects get DESTDIR support for free from Automake but this one does not use Automake, as mentioned above. > > # refresh config.sub, the original one does not recognize "redhat" > > # as vendorname: > > for f in /usr/share/automake-1.1?/config.sub; do > > : > > done > > cp -p $f . > > libtoolize --force --copy > > aclocal > > autoconf > > Perhaps autoreconf and patching the build system seems better to me. But not > that important. config.{sub,guess} scripts are totally autonomous shell script. They can get copied to the project by "automake --add-missing --force-missing" but, unfortunately, this project does not use Automake. Otherwise, autoreconf is just another way to call the appropriate tools. Calling them directly is not a bad idea either, it is more transparent. Perhaps you meant that we could patch the build system to use Automake. That's a valid idea, I might decide to do that in future. But keeping the original build system should also be acceptible for the review. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA contact for the bug. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review