Bug 224458

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.