Requesting review in preparation for the Fedora merge. libsilc is used primary by gaim as a secure encrypted IRC-like protocol. http://cvs.fedora.redhat.com/viewcvs/devel/libsilc/ Description: SILC Client Library libraries for SILC clients.
two minors in the spec file: - use %{?dist} instead of hardcoded .fc6 in the Release tag - use http://www.silcnet.org/download/toolkit/sources/silc-toolkit-%{version}.tar.bz2 as Source0
Is a review of this still needed?
Should we close this Review Request?
CC'ing Stu, who seems to be the current maintainer of this package. I think this does still need a review, because it's still required by libpurple which in turn is needed by a pile of things. I'll take a look. First off, let's look at the rpmlint complaints: libsilc-devel.x86_64: W: no-documentation Yeah, the documentation is in the -doc package. libsilc.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsilcclient-1.1.so.2.0.1 /lib64/libdl.so.2 Not a huge deal, you might be able to fix this with the libtool tweak from http://fedoraproject.org/wiki/PackageMaintainers/CommonRpmlintIssues#unused-direct-shlib-dependency if it bothers you. Then there are 249 of these: libsilc.x86_64: W: undefined-non-weak-symbol /usr/lib64/libsilcclient-1.1.so.2.0.1 silc_hash_ptr libsilc.x86_64: W: undefined-non-weak-symbol /usr/lib64/libsilcclient-1.1.so.2.0.1 silc_packet_set_keys It seems that libsilcclient doesn't actually link against libsilc, which would seem to be a bug to me. Am I missing something? Anyway, those don't stop me from doing a full review. The way dependencies are filtered is a bit fragile because it ignores any rpmbuild customizations involving the dependency generator. I don't think it's a significant issue, but something akin to what many Perl modules which generates the filter on the fly might work better. There's some discussion at http://fedoraproject.org/wiki/Packaging/Perl#Filtering_Requires:_and_Provides In any case, could you add a comment to the spec with a note on why you need to filter the dependencies? From the README file I'd expect to see silc and silcd binaries somewhere, and something other than libsilc packages to hold them, but the source doesn't seem to include any actual applications. I guess if it grew some then the base package would be misnamed but we don't usually worry about naming issues in merge reviews unless there are egregious issues. It might be nice to clarify the meaning of SILC in %description. Currently you either have to install the package and read the docs or search the network to figure out just what this package is supposed to do. * source files match upstream: fcfb34cd0bb858a711ba0af4a2fce60ae64e485b35c67dcdad764cc6a5feac1f silc-toolkit-1.1.7.tar.bz2 * package meets naming and versioning guidelines. * specfile is properly named, is cleanly written and uses macros consistently. * summary is OK. ? description could use some clarification. * dist tag is present. * build root is OK. * license field matches the actual license. * license is open source-compatible. * license text included in package. * latest version is being packaged. * BuildRequires are proper. * compiler flags are appropriate. * %clean is present. * package builds in mock (rawhide, x86_64). * package installs properly. * debuginfo package looks complete. ? rpmlint has several possibly valid complaints. * final provides and requires are sane: libsilc-1.1.7-1.fc10.x86_64.rpm libsilc-1.1.so.2()(64bit) libsilcclient-1.1.so.2()(64bit) libsilc = 1.1.7-1.fc10 = /sbin/ldconfig libsilc-devel-1.1.7-1.fc10.x86_64.rpm pkgconfig(silc) = 1.1.7 libsilc-devel = 1.1.7-1.fc10 = libsilc = 1.1.7-1.fc10 pkgconfig libsilc-doc-1.1.7-1.fc10.x86_64.rpm libsilc-doc = 1.1.7-1.fc10 = * %check is not present; no test suite upstream. * shared libraries present; ldconfig called properly. * owns the directories it creates. * doesn't own any directories it shouldn't. * no duplicates in %files. * file permissions are appropriate. * scriptlets OK (ldconfig) * code, not content. * %docs are in a -doc subpackage. * %docs are not necessary for the proper functioning of the package. * headers are in the -devel subpackage. * pkgconfig files are in the -devel subpackage. * no static libraries. * no libtool .la files.
(In reply to comment #4) > CC'ing Stu, who seems to be the current maintainer of this package. Thanks! > From the README file I'd expect to see silc and silcd binaries somewhere, and > something other than libsilc packages to hold them, but the source doesn't seem > to include any actual applications. I guess if it grew some then the base > package would be misnamed but we don't usually worry about naming issues in > merge reviews unless there are egregious issues. This is due to the slightly odd way upstream creates 3 separate packages (silc-client, silc-server and silc-toolkit/libsilc) from the same source tree. Would you prefer me to patch the README to exclude mention of the client and server, just drop the README, or do nothing? I assume for fixes to the other issues I should just commit them to CVS?
I don't think there's any point in fixing the README; it was just mildly confusing to me and as long as it actually applies to this package then it should be included. Just commit any fixes you'd like to make to CVS and add a note here. You don't have to build or anything like that unless you want to.
(In reply to comment #4) > libsilc.x86_64: W: unused-direct-shlib-dependency > /usr/lib64/libsilcclient-1.1.so.2.0.1 /lib64/libdl.so.2 > Not a huge deal, you might be able to fix this with the libtool tweak from > http://fedoraproject.org/wiki/PackageMaintainers/CommonRpmlintIssues#unused-direct-shlib-dependency > if it bothers you. Fixed in CVS (don't link libsilcclient to libdl) > Then there are 249 of these: > libsilc.x86_64: W: undefined-non-weak-symbol > /usr/lib64/libsilcclient-1.1.so.2.0.1 silc_hash_ptr > libsilc.x86_64: W: undefined-non-weak-symbol > /usr/lib64/libsilcclient-1.1.so.2.0.1 silc_packet_set_keys > It seems that libsilcclient doesn't actually link against libsilc, which would > seem to be a bug to me. Am I missing something? Fixed in CVS, no you are missing nothing, that was a bug. > The way dependencies are filtered is a bit fragile because it ignores any > rpmbuild customizations involving the dependency generator. I don't think it's > a significant issue, but something akin to what many Perl modules which > generates the filter on the fly might work better. There's some discussion at > http://fedoraproject.org/wiki/Packaging/Perl#Filtering_Requires:_and_Provides I changed the filtering to use a dynamically generated script similar to the example. > In any case, could you add a comment to the spec with a note on why you need to > filter the dependencies? It was mentioned in the %changelog, but I've copied that comment to the body of the spec file too. The filtering is to fix bug #245323. > It might be nice to clarify the meaning of SILC in %description. Currently you > either have to install the package and read the docs or search the network to > figure out just what this package is supposed to do. I've updated the description in CVS.
Thanks for doing this. rpmlint has much less complaining to do now; just the bogus "no-documentation" warning for the -devel package. Everything I could find to complain about is fixed. APPROVED Note that there's no need for you to do anything at all since the package is already in the distro; this just gets the review cleared out.