Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=886300 --- Comment #6 from Pavel Raiskup <praiskup@xxxxxxxxxx> --- 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 -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=LBnTScBFwd&a=cc_unsubscribe _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review