Fedora Merge Review: speex http://cvs.fedora.redhat.com/viewcvs/devel/speex/ Initial Owner: davidz
Upstream source file location incorrect. Should be: http://downloads.us.xiph.org/releases/speex/speex-1.2beta1.tar.gz Not sure if package meets naming guidelines: Upstream calls it "speex-1.2beta1". rpm is called "speex-1.2-0.2.beta1" rpmlint complains that devel package has no documentation. All of the potential files are in the base package. Does speex-devel need "BuildRequires: automake" to install the file /usr/share/aclocal/speex.m4 ? Stuff that is ok: license; source tar file matches upstream; ldconfig
It does meet the naming guidelines. Devel without docs is ok. Ehh, I don't think we need BR: automake here.
Created attachment 183061 [details] Spec file patch I'll start a formal review. Attached is a suggested patch to the current development spec file : - Update to 1.2.0beta2. - Remove rpath. - Exclude static library.
I've gone through everything, and it all looks okay. Apart from additional minor cleanups to the spec file (ugly mix of spaces and tabs), I only have one more suggestion : Split off a speex-tools package which would contain the two binaries and manual.pdf. That way, only the small library often pulled in as a dependency would be in the main speex package. This is what is often done in Fedora packages.
Ping? It would be nice to get this package cleaned up for Fedora 8.
Too late for Fedora 8... ping again?
Paging the maintainer to this thread...
Hi, with new maintainer is here new version of speex-1.2-0.4.beta3. This should solve the mentioned issues. Please look and add + :)
Continuing review : * There is still a mess mix of spaces and tabs in the spec file (minor) * Passing --with-ogg-libraries=%{_libdir} is not needed * The "chmod a-x README" makes more sense in %prep than in %install * Why "--enable-static" as the static libs are excluded in %files? * Why are libspeexdsp.so and libspeexdsp.a in the main package? (major!) * The "rm -f $RPM_BUILD_ROOT%{_docdir}/speex-*/manual.pdf" line is wrong, should be "rm -f $RPM_BUILD_ROOT%{_docdir}/speex/manual.pdf" * The "%{_defaultdocdir}/speex/manual.pdf" line in %files should be removed to avoid including the PDF manual twice (after fixing the above rm). And what about my suggestion of splitting out the two binaries into their own sub-package, like many other codec library packages do?
The splitting packages is ok for me. I don't know in which group should be speex-tools have been. Do you? I've also problem with removing %makeinstall macro. Maybe it can't be removed because of strange autotolls. And the last problem with moving .so files into -devel made these messages: rpmlint i686/speex-devel-1.2-0.4.beta3.i686.rpm speex-devel.i686: W: no-documentation speex-devel.i686: E: library-without-ldconfig-postin /usr/lib/libspeexdsp.so.1.4.0 speex-devel.i686: E: library-without-ldconfig-postun /usr/lib/libspeexdsp.so.1.4.0
in the meantime are the packages here: http://mmaslano.fedorapeople.org/speex/
* The group could be "Applications/Multimedia". * The tools' man pages should also be moved to the tools sub-directory. * Instead of removing and excluding the *.a files, use --disable-static * You can remove the *.la in %install or %exclude them in the devel package * You need to have : * %{_libdir}/libspeex*.so.* in the main package * %{_libdir}/libspeex*.so in the devel package
I updated the files on the: http://mmaslano.fedorapeople.org/speex/
Is there any other problems with review? I'd like to close it.
Sorry for the long silence. Attached is a quick patch to correct some minor issues. Overall things look good now. The patch : - Changes to use the preferred "make install DESTDIR=" install method - Changes the %files lists to be more consistent (glob vs. non glob + all *.la excludes in the devel package)
Created attachment 303651 [details] Speex 1.2-0.4.beta3 spec file patch
Thanks, I've overlooked these things. You can check it in cvs.
OK, one last minor thing : The %{_defaultdocdir}/speex line isn't needed. I see this was supposed to fix bug #439284 but I think it's wrong as the /usr/share/doc/speex directory isn't included in the package. Also, you should escape the macro in the %changelog by putting %%{_defaultdocdir}/speex so that it doesn't get expanded to the value in there. The rest looks all good, modulo some weird (yet very minor) tabs and spaces mixing inside the spec file.
Ping? Approving now anyway, since the last details that could be changed aren't blockers.
I think I fixed all mentioned things form #18 in peex-1.2-0.8.beta3... Sooo could we close it?
I still see the "%{_defaultdocdir}/speex" line, which is probably including an empty directory or the same files as the %doc of the main package, given that the manual.pdf is removed in %install. And the macro in the %changelog still isn't escaped. These are the changes I'm taking about : --- speex.spec 25 Apr 2008 10:39:11 -0000 1.25 +++ speex.spec 13 May 2008 11:35:37 -0000 @@ -79,7 +79,6 @@ %files tools %defattr(-,root,root,-) -%{_defaultdocdir}/speex %doc doc/manual.pdf %{_bindir}/speexenc %{_bindir}/speexdec @@ -96,7 +95,7 @@ (CVE-2008-1686, #441239, https://trac.xiph.org/changeset/14701) * Mon Mar 31 2008 Marcela Maslanova <mmaslano> - 1.2-0.6.beta3 -- 439284 add owner to %{_defaultdocdir}/speex +- 439284 add owner to %%{_defaultdocdir}/speex * Tue Feb 19 2008 Fedora Release Engineering <rel-eng> - 1.2-0.5.beta3 - Autorebuild for GCC 4.3
Thank you for your help with review. The last changes from #21 will be in the latest version of speex.
So is this all fixed and reviewed now?
Yes, it is. It should be closed by reviewer.
I have removed some spaces from the current devel spec file, as tabs are used in most places (this is only a cosmetic change). All the rest looks good, so closing.