Spec URL: https://rpm.silfreed.net:8002/?raw-file/aebf3acfb13d/libconcord/libconcord.spec SRPM URL: http://www.silfreed.net/download/repo/packages/libconcord/libconcord-0.20-0.1.20080318cvs.src.rpm Description: Library to talk to Logitech® Harmony® universal remote controls $ rpmlint SRPMS/libconcord-0.20-0.1.20080318cvs.src.rpm RPMS/libconcord-*0.20-0.1.20080318*.rpm libconcord-devel.i386: W: no-documentation libconcord-devel.i386: W: no-dependency-on libconcord libconcord-devel.x86_64: W: no-documentation libconcord-devel.x86_64: W: no-dependency-on libconcord
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.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.