Spec URL: http://fcami.fedorapeople.org/srpms/sino.spec SRPM URL: http://fcami.fedorapeople.org/srpms/sino-3.1.21-1.fc16.src.rpm Description: Sino (short for "size is no object") is a high performance free text search engine. It was originally written in 1995 and has been mainly used to provide production level search facilities for most of the Legal Information Institutes that form part of the Free Access to Law Movement (FALM). Fedora Account System Username: fcami
doesn't build in mock : + CFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -fPIC' + make -j4 perl make[1]: Entering directory `/builddir/build/BUILD/sino-3.1.21/src' make[1]: warning: jobserver unavailable: using -j1. Add `+' to parent make rule. make[2]: Entering directory `/builddir/build/BUILD/sino-3.1.21/src' make[2]: Nothing to be done for `cc'. make[2]: Leaving directory `/builddir/build/BUILD/sino-3.1.21/src' make[2]: Entering directory `/builddir/build/BUILD/sino-3.1.21/libperl' `./pcc` -c -I `perl -MConfig -e 'print $Config{archlib}'`/CORE sinoapi_wrap.c sinoapi_wrap.c:702:20: fatal error: EXTERN.h: No such file or directory compilation terminated. make[2]: *** [sinoapi_wrap.o] Error 1 make[2]: Leaving directory `/builddir/build/BUILD/sino-3.1.21/libperl' make[1]: *** [perl] Error 2 make[1]: Leaving directory `/builddir/build/BUILD/sino-3.1.21/src' make: *** [perl] Error 2 erreur : Mauvais statut de sortie pour /var/tmp/rpm-tmp.Hbmfwl (%build) Mauvais statut de sortie pour /var/tmp/rpm-tmp.Hbmfwl (%build)
Ooops, good catch, thanks. Spec URL: http://fcami.fedorapeople.org/srpms/sino.spec SRPM URL: http://fcami.fedorapeople.org/srpms/sino-3.1.21-2.fc16.src.rpm
I'm taking this review.
Hi François, thanks for packaging! Here are some comments: * Note that: Explicit dependency on perl-devel is not allowed (consider Module::Build). For more info [1]. * There should be also BR MODULE_COMPAT_* dependency [1]: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version)) * Weird thing is, that the sino does not require libsino, is it statically linked. Is that necessary? * Explicit library dependency shouldn't be needed [2]. If yes, the %{_isa} should be given & comment added. * No subpackage -> subpackage dependancy is given (except the lib one). Is that correct? * I'm not sure about: %post & %postun -n perl-sinoAPI -p /sbin/ldconfig I'm not a perl guru but as this subpackage does not install libraries into ldconfig path, and thus it should not be needed? * imo we don't need %posttrans * scriptlets also * Your hacks for so library may look ugly, would it be possible to be solved upstream? Just a note now for this, the 'mv -f lib/libsino.so lib/libsino.so.3' command seems to redundant there. * License seems to be s/GPLv3/GPLv3+/ and license of malloc.c seems to be not GPLv3+ (btw. this file seems to be unused, but it is quite good - seems to be very similar to what tcsh has (tc.alloc.c) and it causes weird kernel problems #443643). * Manual page for sinodisp is not found. I could be considered like small problem, but I can't find any documentation on this! No --help, no manual page, also 'rpm -qd sino-cgi' is silent. No documentation found. Nits: * I would comment the patches a little. Downstream/upstream, etc. * consider adding -p to install command (preserve timestamps) * IIRC, defattr is not needed even in EPEL5, version in RHEL5 is 4.4.2.3. Thanks for working on this, Pavel [1] https://fedoraproject.org/wiki/Packaging:Perl [2] https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
Hi Pavel, Thank you very much for taking this review! * dependency on perl-devel => fixed. * MODULE_COMPAT_* dependency => fixed. * sino/sinomake are statically linked => this is how upstream builds. I plan to switch these binaries to dynamic linking at some point. * Explicit library dependency shouldn't be needed [2]. If yes, the %{_isa} should be given & comment added. => I am not sure of the line where this should be changed :/ * No subpackage -> subpackage dependancy is given (except the lib one). Is that correct? => I think so. * %post, %postun, %posttrans -n perl-sinoAPI -p /sbin/ldconfig => removed. * Your hacks for so library may look ugly, would it be possible to be solved upstream? => I wish, but I am pretty sure upstream needs to make sure nothing is broken on other UNIX like Solaris, and I do not have a Solaris test box. The alternative is to ship the static library and I would rather not do that... (and yes, the hack is ugly). * 'mv -f lib/libsino.so lib/libsino.so.3' redundant => I needed to move it out of the way, I switched to 'rm -f'. * license of malloc.c seems to be not GPLv3+ and this file seems to be unused => Good catch, added rm -f at the end of prep. * Manual page for sinodisp is not found (...) I can't find any documentation on this! => Sorry, I should have fixed it earlier. The documentation is at the beginning of sinodisp.c, so I now extract that and create a sinodisp.txt file which is installed by the sino-cgi package as documentation. I moved the sinodisp binary to /var/www/cgi-bin because that's where it really belongs. Nits: * I would comment the patches a little. Downstream/upstream, etc. => for now nothing's been upstreamed, but I noted what could, and could not be upstreamed, in the new spec. * consider adding -p to install command (preserve timestamps) => done. * IIRC, defattr is not needed even in EPEL5, version in RHEL5 is 4.4.2.3. => removed. thanks. Thank you! Spec URL: http://fcami.fedorapeople.org/srpms/sino.spec SRPM URL: http://fcami.fedorapeople.org/srpms/sino-3.1.21-3.fc18.src.rpm
Hi François, thanks for the progress! ==== for the previous comments: > * sino/sinomake are statically linked > => this is how upstream builds. I plan to switch these binaries to > dynamic linking at some point. I would suggest you to do it now, I'm not sure how big deal it is but packaging guidelines covers this quite simply: Cite Fedora's Packaging:Guidelines [1]: -> Static linkage is a special exception and should be decided on a case-by-case basis. The packager must provide rationale for linking statically, including precedences where available, to FESCO for approval. -> Programs which don't need to notify FESCo If a library you depend on only provides a static version your package can link against it provided that you BuildRequire the *-static subpackage. Packagers in such a situation should be aware that if a shared library becomes available, that you should adjust your package to use the shared library. I think that the most easy one will be to link it dynamically (not create the *-static package). > * Explicit library dependency shouldn't be needed [2]. If yes, the %{_isa} > should be given & comment added. > => I am not sure of the line where this should be changed :/ 58 | %package -n libsino-devel 59 | Group: Development/Libraries 60 | Summary: Files for development using libsino 61 | Requires: lib%{name} = %{version}-%{release} ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ It is probably good to be there .. but it should be documented as mentioned in guidelines [2]. And the %{_isa} should be added. > * %post, %postun, %posttrans -n perl-sinoAPI -p /sbin/ldconfig > => removed. Yet the %posttrans is probably unnecessary. > * 'mv -f lib/libsino.so lib/libsino.so.3' redundant > => I needed to move it out of the way, I switched to 'rm -f'. Sorry, I made bad statement here. I thought the command was completely redundant, as the following 'mv' may overwrite the target (by adding -f). This is nit - not a problem. > * license of malloc.c seems to be not GPLv3+ and this file seems to be unused > => Good catch, added rm -f at the end of prep. I'm not sure here if this is correct. It looks little bit dirty because malloc.c is in distribution. I would agree with this step if upstream is OK to remove it in future release, but otherwise the malloc.c is distributed (for some compilation time purposes) and I would do the 's/GPLv3/GPLv3+ and BSD/'. Note the '+' there! > * Manual page for sinodisp is not found (...) I can't find any documentation > on this! > => Sorry, I should have fixed it earlier. The documentation is at the > beginning of sinodisp.c, so I now extract that and create a sinodisp.txt > file which is installed by the sino-cgi package as documentation. I moved > the sinodisp binary to /var/www/cgi-bin because that's where it really > belongs. Thanks for this! I'm not sure if the 'head -24' auto-scripting is ok for future releases (maintenance hell) but it is not problem for me. ==== Some new comments: * The installation of images: %{_datadir}/%{name}-%{version}/images %{_datadir}/%{name}-%{version}/images/* $ rpm -qf /usr/share/sino-3.1.21 file /usr/share/sino-3.1.21 is not owned by any package Consider adding '%dir %{_datadir}/%{name}-%{version}/'. * would it be possible to split the very long lines? [1] https://fedoraproject.org/wiki/Packaging:Guidelines [2] https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
Hello Pavel! Thanks for your comments. :) * Requires: %{name}%{?_isa} = %{version}-%{release} => fixed. * ldconfig in posttrans => I think it is needed, see https://www.redhat.com/archives/rhl-devel-list/2007-June/msg02497.html for instance. * sinodisp had no documentation => hehe, no problem. :) * malloc.c => I am pretty sure it is not needed after looking at the sources. I am going to ask upstream to remove it from future releases. * long lines => split. * /usr/share/sino-3.1.21 not owned by any package => ooops, my bad. fixed. * explicit Requires: => added %{_isa} and a note. TODO: * sino / sinomake / sinoshow / sinodisp are statically linked => this requires building the shared library first, or relinking afterwards... Hmmm. Let me think about it, but I agree, I need to fix that. In the meantime: Spec URL: http://fcami.fedorapeople.org/srpms/sino.spec SRPM URL: http://fcami.fedorapeople.org/srpms/sino-3.1.21-4.fc18.src.rpm Thanks again!
> --- Comment #7 from François Cami <fdc> --- > > * ldconfig in posttrans > => I think it is needed, see > https://www.redhat.com/archives/rhl-devel-list/2007-June/msg02497.html for > instance. This thread was about removing ldconfig from %post/%postun, iirc. The problem was (and actually is) that each package installing some shared library is calling ldconfig based on package guidelines. Calling ldconfig in %posttrans seems to be redundant and unnecessarily time consuming. > * malloc.c > => I am pretty sure it is not needed after looking at the sources. I am going > to ask upstream to remove it from future releases. Thanks! Some link here would be nice, in spec respectively. > TODO: > * sino / sinomake / sinoshow / sinodisp are statically linked > => this requires building the shared library first, or relinking > afterwards... Hmmm. Let me think about it, but I agree, I need to fix that. As you probably agree that this is not according to our Fedora's PG, it is the MUST now, sorry for that. Thanks for your work, otherwise it seems to be OK to me now. Pavel
Hello Pavel, Re shared library and must: yes, I completely agree with you. Please give me a few days for this. Thanks a lot. François
Ping, François, are you still interested in packaging sino?
Hi Pavel, Sorry, this has slipped through my radar. I'm currently not doing any packaging at the moment so I'll close this one. Thanks for your advice so far!