Bug 438105

A few questions:

1) I think at least COPYING should be a %doc, which would get rid of 1 of the
rpmlint warnings.

2) Is "no-dependency-on libconcord" normal? I suspect not. Take for example
libtiff, where libtiff-devel depends on libtiff:

  %package devel
  Summary: Development tools for programs which will use the libtiff library
  Group: Development/Libraries
  Requires: %{name} = %{version}-%{release}

3) Out of curiosity, how often are you intending to push updates of this
package; i.e. is this review for a CVS version simply because 0.20 isn't out yet
and we need to review something with the new project nane, then it'll switch to
releases, or ...?

A few more notes:

1) This might be GPLv3+ not GPLv2+. See my recent email on concordance-users
requesting license clarification.

2) I haven't looked at the SRPM yet, but does it include the "examples"
directory from CVS? If so, we should probably remove that directory, because
those files certainly aren't GPL, and are probably (C) Logitech, who might not
like Fedora distributing them. When using official releases, you'll certainly
have to repackage the releases like I did for fxload (see the shell script and
spec in fxload for how to do it)

3) The %defattr should probably specify explicit permissions, at least that's
what I was told in my fxload review:

%defattr(0644,root,root,0755)

4) You certainly shouldn't distribute *.a, and I *think* not *.la either, even
in -devel.

5) My comment about making COPYING a %doc to fix the rpmlint warning about docs
was incorrect; I misread that as being re: the main package, not -devel. Still,
if you want to shut rpmlint up, there is a TODO there you could package in -devel.

A few more, then I'll stop for the night!

1) Missing "Requires: libusb", or does that get picked up automatically by rpm
based on running ldd on libconcord.so?

2) I don't think you need "Requires: /sbin/ldconfig" since it's part of the base
package set.

If you are going to include this, it seems common to only require it for script
execution time, e.g.:

libgeda/devel/libgeda.spec:16:Requires(post): /sbin/ldconfig
libgeda/devel/libgeda.spec:17:Requires(postun): /sbin/ldconfig

3) There are more (double and more) blank lines in the spec than there probably
should be, according to examples I've seen of existing spec files. Not a
functional problem, but best to keep formatting consistent with existing specs
and fedora Wiki examples.

(In reply to comment #1)
> 1) I think at least COPYING should be a %doc, which would get rid of 1 of 
the
> rpmlint warnings.

moved to -devel

> 2) Is "no-dependency-on libconcord" normal?

Fixed

> 3) Out of curiosity, how often are you intending to push updates of this
> package; i.e. is this review for a CVS version simply because 0.20 isn't out 
yet
> and we need to review something with the new project nane, then it'll switch 
to
> releases, or ...?

Currently I'm tracking CVS because it's the first version that will:
1) use the concordance name which is needed to get into Fedora
2) uses the split library/cli

(In reply to comment #2)
> 1) This might be GPLv3+ not GPLv2+. See my recent email on concordance-users
> requesting license clarification.

Fixed

> 2) I haven't looked at the SRPM yet, but does it include the "examples"
> directory from CVS?

Doesn't seem to exist.

> 3) The %defattr should probably specify explicit permissions, at least 
that's
> what I was told in my fxload review:
> 
> %defattr(0644,root,root,0755)

I don't see this in the example spec files, or the packaging or review 
guidelines.

> 4) You certainly shouldn't distribute *.a, and I *think* not *.la either, 
even
> in -devel.

Strange that rpmlint didn't pick that up; fixed.

> 5) My comment about making COPYING a %doc to fix the rpmlint warning about 
docs
> was incorrect; I misread that as being re: the main package, not -devel. 
Still,
> if you want to shut rpmlint up, there is a TODO there you could package 
in -devel.
> 

I'll pick up the COPYING file once it's in CVS; I saw your messages on the 
list and it looks like upstream will be renaming licensing.txt soon, as well.

(In reply to comment #3)
> 1) Missing "Requires: libusb", or does that get picked up automatically by 
rpm
> based on running ldd on libconcord.so?

Explicit Requires are discouraged; the dependency should be picked up 
automatically.

> 2) I don't think you need "Requires: /sbin/ldconfig" since it's part of the 
base
> package set.
> 
> If you are going to include this, it seems common to only require it for 
script
> execution time, e.g.:
> 
> libgeda/devel/libgeda.spec:16:Requires(post): /sbin/ldconfig
> libgeda/devel/libgeda.spec:17:Requires(postun): /sbin/ldconfig

It's a library so it's required.

> 3) There are more (double and more) blank lines in the spec than there 
probably
> should be, according to examples I've seen of existing spec files. Not a
> functional problem, but best to keep formatting consistent with existing 
specs
> and fedora Wiki examples.

I'm following the format of the example spec files in rpmdevtools.

Spec URL: 
https://rpm.silfreed.net:8002/?raw-file/95dce3f15d6f/libconcord/libconcord.spec
SRPM URL: 
http://www.silfreed.net/download/repo/packages/libconcord/libconcord-0.20-0.2.20080318cvs.src.rpm

%changelog
* Tue Mar 18 2008 Douglas E. Warner <silfreed> 
0.20-0.2.20080318cvs
- adding devel dependancy on main package
- moved TODO from main package to devel
- fixed license to be GPLv3+, not GPLv2+
- disabling static linking
- removing static libraries

> I don't see this in the example spec files...

Out of curiosity, what example spec files are you referring to? I haven't seen
any, and I'd love to read them. I think the wiki only has snippets.

> > 2) I don't think you need "Requires: /sbin/ldconfig" since it's part of
> > the base> package set.
> >
> > If you are going to include this, it seems common to only require it for
> > script execution time, e.g.:
> > 
> > libgeda/devel/libgeda.spec:16:Requires(post): /sbin/ldconfig
> > libgeda/devel/libgeda.spec:17:Requires(postun): /sbin/ldconfig
> 
> It's a library so it's required.

Are you sure? I searched the whole devel CVS tree, and it looks like all
libraries only require ldconfig in post/postun, not for the entire package.

Anyway. Hopefully I'll get a chance to do a real complete review this weekend.
My gut instinct is that everything's OK.

Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on: i386 (targetted at F8 using mock running on i386 F8)
     NOTE: See SUGGESTED ITEMS below for serious problems though.
 [x] Rpmlint output:
 [x] Package is not relocatable.
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other
legal requirements as defined in the legal section of Packaging Guidelines.
 [!] License field in the package spec file matches the actual license.
     License type: GPLv3+
     NOTE: Upstream is fixing incorrectly marked GPLv2+ files. I believe we need
to wait until this is done, then complete the review using another CVS snapshot.
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
     NOTE: Upstream renaming license.txt to COPYING. I'd suggest waiting until
this is done and grabbing another CVS snapshot before actually importing the
SRPM into Fedora CVS.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
     MD5SUM this package    : N/A
     MD5SUM upstream package: N/A
     NOTE: CVS snapshot. Manually checked out, and checked for correctness using
"diff -urN"
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [!] ldconfig called in %post and %postun if required.
     NOTE: "Requires" for /sbin/ldconfig should be for Requires(post) and
Requires(postun) only, to be consistent with existing Fedora packages.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [x] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [x] Development .so files in -devel subpackage, if present.
 [x] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: i386
 [!] Package should compile and build into binary rpms on all supported
architectures.
     NOTE: Package does not build for ANY supported arch's for dist-f9:
     http://koji.fedoraproject.org/koji/taskinfo?taskID=526551
     NOTE: Package does build for ALL supported arch's for
dist-f8-updates-candidate:
     http://koji.fedoraproject.org/koji/taskinfo?taskID=526556
     There's no point approving the package until it builds for dist-f9 though.
 [x] Package functions as described.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files are correct.
 [x] File based requires are sane.

=== SUMMARY ===
1. Need to wait for upstream to fix licensing text in all files to be consistent.
2. /sbin/ldconfig Requires should be tweaked to be consisted with all other
Fedora packages.
3. Package doesn't build for dist-f9. Upstream should fix (I'll look into this.)

(In reply to comment #8)
> > I don't see this in the example spec files...
> 
> Out of curiosity, what example spec files are you referring to? I haven't 
seen
> any, and I'd love to read them. I think the wiki only has snippets.

rpmdevtools package - /etc/rpmdevtools/*.spec

> Are you sure? I searched the whole devel CVS tree, and it looks like all
> libraries only require ldconfig in post/postun, not for the entire package.

Right; this was what I meant, and I overlooked it the first time.  I'll fix 
these so it's just a require for post/postun.

Spec URL: 
https://rpm.silfreed.net:8002/?raw-file/0b6b2b721ee7/libconcord/libconcord.spec
SRPM URL: 
http://www.silfreed.net/download/repo/packages/libconcord/libconcord-0.20-0.3.20080318cvs.src.rpm

* Mon Mar 24 2008 Douglas E. Warner <silfreed> 
0.20-0.3.20080318cvs
- moved Requires ldconfig to post/postun
- adding patch for gcc 4.3 to define extra headers

Looks like we just need to get upstream to get the GCC patch included upstream 
(not necessary, I guess, just nice) and wait for the licensing fixes.

Thanks for the review!

> Looks like we just need to get upstream to get the GCC patch included
> upstream (not necessary, I guess, just nice) and wait for the licensing
> fixes.

I believe this is all done upstream now. If you spin a new .spec with this, I'll
approve it.

BTW, latest upstream CVS has Python and Perl bindings now. Do you intend to add
these into sub-packages sometime (after initial approval/checkin/build/push is
fine by me)? I can try and help out with the Python bindings at least if you need.

FYI, official release 0.20 is out now, which includes everything this review was
waiting on.

Spec URL: 
https://rpm.silfreed.net:8002/index.cgi/raw-file/4629a257a781/libconcord/libconcord.spec
SRPM 
URL:http://www.silfreed.net/download/repo/packages/libconcord/libconcord-0.20-1.src.rpm

%changelog
* Mon Apr 21 2008 Douglas E. Warner <silfreed> 0.20-1
- update to 0.20
- removing unneeded gcc 4.3 patch


I was going to include the python and perl bindings as subpackages, but 
realized I can't do this as noarch subpackages of an arch-specific package.  
Do you think it's better to include them as arch-specific packages or separate 
noarch packages?

Re: The bindings - I reviewed a bunch of existing .spec files, and having
arch-specific Python sub-packages seems to be the way it works.

I'm OK with shipping libconcord without bindings first, then adding them later
if you want. I don't know if that would require another review for the
sub-packages then though - I'll see if I can find the answer.

Hopefully, I'll get a chance to finalize the review tonight.

I have a few more comments since the .spec has been updated.

1)

The .spec in the SRPM and the separate URL you posted don't quite match; only 1
has a "BuildRequires: python". For completeness of review, these should match.

2) 

You may as well use %{mainpkg} to replace both instances of "concordance" in the
Source0 URL.

3)

In the %setup line, %name should be %{name}. At least for consistency, even if
not functionality.

4)

I believe the canonical hostname for SF downloads is downloads.sourceforge.net,
not internap.dl.sourceforget.net (Source0 URL)

5)

I don't think "%files ../xxx" is legal. Instead, I think you need to install
those files in %install, then package them from the installation directory.

Take a look at fftw2 or cmigemo (I looked at devel branches) for other .spec
files that work this way.

(In reply to comment #16)
> 1) The .spec in the SRPM and the separate URL you posted don't quite match; 
only 1
> has a "BuildRequires: python". For completeness of review, these should 
match.

This must be rpmbuild stripping out the BR since this SRPM is the one from 
rpmbuild and not the re-packaged one from mock.  I guess I'll just remove it 
from the spec file as well.
 
> 2) You may as well use %{mainpkg} to replace both instances of "concordance" 
in the
> Source0 URL.

I like to keep the package name explicitly in the download URL.

> 3) In the %setup line, %name should be %{name}. At least for consistency, 
even if
> not functionality.

Thanks, fixed.

> 4) I believe the canonical hostname for SF downloads is 
downloads.sourceforge.net,
> not internap.dl.sourceforget.net (Source0 URL)

Thansk - I missed that when I moved from CVS back to the real source.  Fixed.

> 5) I don't think "%files ../xxx" is legal. Instead, I think you need to 
install
> those files in %install, then package them from the installation directory.
> 
> Take a look at fftw2 or cmigemo (I looked at devel branches) for other .spec
> files that work this way.

I think I'll tackle this a different way by removing the '%setup -n' stuff and 
cd to the proper directory for configuring and building.

Spec URL: 
http://www.silfreed.net/download/repo/packages/libconcord/libconcord-0.20-2.src.rpm
SRPM URL: 
https://rpm.silfreed.net:8002/index.cgi/file/a8df2c0857ab/libconcord/libconcord.spec

%changelog
* Mon Apr 21 2008 Douglas E. Warner <silfreed> 0.20-2
- fixed Source0 url
- changing to build/install dir rather than setting it in setup macro
- adding perl/python bindings

One problem I found, via rpmlint is:

libconcord-perl.i386: E: non-standard-executable-perm
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/auto/concord/concord.so
0555

Also, I think (at least in F9/devel) you need to package Python egg-info files;
see http://fedoraproject.org/wiki/Packaging/Python/Eggs.

Related to this, in %build, there is:

%if 0%{?fedora} >= 8

I think this should be:

%if 0%{?fedora} > 8

because F8 also needs the "import setuptools" stuff to create eggs.

Ignore the following part of comment 20; I didn't check "%files python"
carefully enough to notice that all files in %{python_sitearch} are packaged.

> Also, I think (at least in F9/devel) you need to package Python egg-info files;
> see http://fedoraproject.org/wiki/Packaging/Python/Eggs.

However, everything "Related to ..." and after still applies, I believe.

Perl bindings looks ok. Only fix the file from #19. It should have 644
permission as others perl scripts. Do you have some perl tests for it? Usually
you should run them with 'make tests' from spec file.

(In reply to comment #20)
> Also, I think (at least in F9/devel) you need to package Python egg-info 
files;
> see http://fedoraproject.org/wiki/Packaging/Python/Eggs.

The python stuff wants to install to sitelib instead of sitearch now that I'm 
building the egg-info for F8.   Are you aware of any way to fix this?

(In reply to comment #22)
> Perl bindings looks ok. Only fix the file from #19. It should have 644
> permission as others perl scripts. Do you have some perl tests for it? 
Usually
> you should run them with 'make tests' from spec file.

When I enable the tests I get the following error:
+ make test
PERL_DL_NONLAZY=1 /usr/bin/perl "-Iblib/lib" "-Iblib/arch" test.pl
Can't load 'blib/arch/auto/concord/concord.so' for module concord: 
blib/arch/auto/concord/concord.so: undefined symbol: get_flash_part_num 
at /usr/lib64/perl5/5.8.8/x86_64-linux-thread-multi/DynaLoader.pm line 230.
 at blib/lib/concord.pm line 11
Compilation failed in require at test.pl line 38.
BEGIN failed--compilation aborted at test.pl line 38.
make: *** [test_dynamic] Error 2

Re comment #22: Shouldn't the permission be 755 like any other .so file? All .so
files I have in /usr/lib/perl have 755 permissions.

Re comment #23: Is this a problem - perhaps that's just the way it works?
Although, I guess on 64-bit archs, it means the 32-bit package would conflict. I
guess check how other Python packages work.

Re comment #24: This sounds like there's a bug in the Perl bindings, perhaps
referencing a symbol that libconcord.so doesn't expose? I'd guess the  upstream
mailing list would be the most likely place to find a fix?

This should fix the perl .so permission (755) and installs the python bindings 
to %{python_sitelib} (multilib-cmp.py doesn't complain).

Spec URL: 
https://rpm.silfreed.net:8002/index.cgi/file/da8ff28c83e0/libconcord/libconcord.spec
SRPM URL: 
http://www.silfreed.net/download/repo/packages/libconcord/libconcord-0.20-2.src.rpm

%changelog
* Wed Apr 23 2008 Douglas E. Warner <silfreed> 0.20-3
- fixed Source0 url to downloads.sourceforge.net instead of dl.sourceforge.net
- fixing mode of concord.so
- fixing python egg generation; adding buildrequire for 
python-setuptools-devel

The separate .spec file content doesn't match the SRPM again.

Also, I assume you want to add the "--install-purelib %{python_sitearch}" to the
Python install command so 32/64 bit packages don't conflict, right?

I'm leaving on vacation for just over 2 weeks this Monday (May 5th), so I won't
be able to review this package during that time. Hopefully the final issues can
be fixed and I can approve this before then...

I plan on adding the perl tests at an additional time.  I'd like to get the 
package through review before Stephen goes on vacation.

Spec 
URL:https://rpm.silfreed.net:8002/index.cgi/file/37414f878d6a/libconcord/libconcord.spec
SRPM URL:
http://www.silfreed.net/download/repo/packages/libconcord/libconcord-0.20-4.src.rpm

%changelog
* Sat May 03 2008 Douglas E. Warner <silfreed> 0.20-4
- installing python packages to sitearch again
- adding additional docs

Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on: i386 (F8 system, F8 mock, devel mock)
 [x] Rpmlint output:
     Minor warnings:
     libconcord-perl.i386: W: no-documentation
     libconcord-python.i386: W: no-documentation
 [x] Package is not relocatable.
 [x] Buildroot is correct
     %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
 [x] Package is licensed with an open-source compatible license and meets other
legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type: GPLv3+
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
     MD5SUM this package    : c2487e851864f38c4da4fe02093652a5
     MD5SUM upstream package: c2487e851864f38c4da4fe02093652a5
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [x] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [!] Package does not contain duplicates in %files.
     TODO is packaged in both libconcord and libconcord-devel.
     I would suggest simply removing it from libconcord-devel, judging by other
packages.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [x] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [x] Development .so files in -devel subpackage, if present.
 [x] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: i386 (F8 and devel)
 [x] Package should compile and build into binary rpms on all supported
architectures.
     Tested all arches via koji scratch build for F8.
 [x] Package functions as described.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files are correct.
 [x] File based requires are sane.

=== SUMMARY ===

APPROVED.

Before actually building/pushing this, you should resolve the duplicate TODO file.

Minor notes: There is a README for the Perl bindings in the source package. You
might want to package that with libconcord-perl. Latest CVS has a similar README
for the Python bindings. If you feel really energetic, you could add that file
as an extra source, and package that in libconcord-python.

New Package CVS Request
=======================
Package Name: libconcord
Short Description: Library to talk to Logitech® Harmony® universal remote 
controls
Owners: silfreed
Branches: F-8 F-9
Cvsextras Commits: yes

cvs done.

concordance-0.20-4.fc8,libconcord-0.20-5.fc8 has been submitted as an update for Fedora 8

concordance-0.20-4.fc9,libconcord-0.20-5.fc9 has been submitted as an update for Fedora 9

libconcord-0.20-5.fc9, concordance-0.20-4.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

libconcord-0.20-5.fc9, concordance-0.20-4.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

libconcord-0.20-5.fc8, concordance-0.20-4.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.