Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 226428 - Merge Review: speex
Summary: Merge Review: speex
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Matthias Saou
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 21:00 UTC by Nobody's working on this, feel free to take it
Modified: 2008-08-08 11:21 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-08-08 11:21:30 UTC
matthias: fedora-review+


Attachments (Terms of Use)
Spec file patch (deleted)
2007-08-31 10:47 UTC, Matthias Saou
no flags Details | Diff
Speex 1.2-0.4.beta3 spec file patch (deleted)
2008-04-24 15:43 UTC, Matthias Saou
no flags Details | Diff

Description Nobody's working on this, feel free to take it 2007-01-31 21:00:57 UTC
Fedora Merge Review: speex

http://cvs.fedora.redhat.com/viewcvs/devel/speex/
Initial Owner: davidz@redhat.com

Comment 1 Pace Willisson 2007-02-03 22:12:06 UTC
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


Comment 2 Tom "spot" Callaway 2007-02-03 22:23:22 UTC
It does meet the naming guidelines.

Devel without docs is ok.

Ehh, I don't think we need BR: automake here.


Comment 3 Matthias Saou 2007-08-31 10:47:19 UTC
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.

Comment 4 Matthias Saou 2007-08-31 10:55:33 UTC
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.

Comment 5 Matthias Saou 2007-09-12 11:24:53 UTC
Ping? It would be nice to get this package cleaned up for Fedora 8.

Comment 6 Matthias Saou 2007-10-22 15:14:10 UTC
Too late for Fedora 8... ping again?

Comment 7 Tom "spot" Callaway 2007-10-22 20:36:39 UTC
Paging the maintainer to this thread...

Comment 8 Marcela Mašláňová 2008-01-24 14:35:32 UTC
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 + :)

Comment 9 Matthias Saou 2008-02-03 13:19:56 UTC
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?

Comment 10 Marcela Mašláňová 2008-02-04 10:57:22 UTC
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


Comment 11 Marcela Mašláňová 2008-02-04 11:52:18 UTC
in the meantime are the packages here:
http://mmaslano.fedorapeople.org/speex/

Comment 12 Matthias Saou 2008-02-04 15:31:04 UTC
 * 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

Comment 13 Marcela Mašláňová 2008-02-04 16:21:25 UTC
I updated the files on the:
http://mmaslano.fedorapeople.org/speex/

Comment 14 Marcela Mašláňová 2008-03-26 15:26:14 UTC
Is there any other problems with review? I'd like to close it.

Comment 15 Matthias Saou 2008-04-24 15:42:17 UTC
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)

Comment 16 Matthias Saou 2008-04-24 15:43:10 UTC
Created attachment 303651 [details]
Speex 1.2-0.4.beta3 spec file patch

Comment 17 Marcela Mašláňová 2008-04-25 10:41:20 UTC
Thanks, I've overlooked these things. You can check it in cvs.

Comment 18 Matthias Saou 2008-04-25 11:05:47 UTC
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.

Comment 19 Matthias Saou 2008-05-13 10:51:32 UTC
Ping? Approving now anyway, since the last details that could be changed aren't
blockers.

Comment 20 Marcela Mašláňová 2008-05-13 11:22:13 UTC
I think I fixed all mentioned things form #18 in peex-1.2-0.8.beta3... Sooo
could we close it?

Comment 21 Matthias Saou 2008-05-13 11:36:10 UTC
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@redhat.com> - 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@fedoraproject.org> -
1.2-0.5.beta3
 - Autorebuild for GCC 4.3

Comment 22 Marcela Mašláňová 2008-05-13 14:01:56 UTC
Thank you for your help with review. 

The last changes from #21 will be in the latest version of speex.

Comment 23 Peter Robinson 2008-07-18 14:24:49 UTC
So is this all fixed and reviewed now?

Comment 24 Marcela Mašláňová 2008-07-18 14:30:49 UTC
Yes, it is. It should be closed by reviewer.

Comment 25 Matthias Saou 2008-08-08 11:21:30 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.