This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 226428 - Merge Review: speex
Merge Review: speex
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Saou
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:00 EST by Nobody's working on this, feel free to take it
Modified: 2008-08-08 07:21 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-08 07:21:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
matthias: fedora‑review+


Attachments (Terms of Use)
Spec file patch (1.61 KB, patch)
2007-08-31 06:47 EDT, Matthias Saou
no flags Details | Diff
Speex 1.2-0.4.beta3 spec file patch (840 bytes, patch)
2008-04-24 11:43 EDT, Matthias Saou
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:00:57 EST
Fedora Merge Review: speex

http://cvs.fedora.redhat.com/viewcvs/devel/speex/
Initial Owner: davidz@redhat.com
Comment 1 Pace Willisson 2007-02-03 17:12:06 EST
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 17:23:22 EST
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 06:47:19 EDT
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 06:55:33 EDT
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 07:24:53 EDT
Ping? It would be nice to get this package cleaned up for Fedora 8.
Comment 6 Matthias Saou 2007-10-22 11:14:10 EDT
Too late for Fedora 8... ping again?
Comment 7 Tom "spot" Callaway 2007-10-22 16:36:39 EDT
Paging the maintainer to this thread...
Comment 8 Marcela Mašláňová 2008-01-24 09:35:32 EST
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 08:19:56 EST
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 05:57:22 EST
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 06:52:18 EST
in the meantime are the packages here:
http://mmaslano.fedorapeople.org/speex/
Comment 12 Matthias Saou 2008-02-04 10:31:04 EST
 * 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 11:21:25 EST
I updated the files on the:
http://mmaslano.fedorapeople.org/speex/
Comment 14 Marcela Mašláňová 2008-03-26 11:26:14 EDT
Is there any other problems with review? I'd like to close it.
Comment 15 Matthias Saou 2008-04-24 11:42:17 EDT
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 11:43:10 EDT
Created attachment 303651 [details]
Speex 1.2-0.4.beta3 spec file patch
Comment 17 Marcela Mašláňová 2008-04-25 06:41:20 EDT
Thanks, I've overlooked these things. You can check it in cvs.
Comment 18 Matthias Saou 2008-04-25 07:05:47 EDT
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 06:51:32 EDT
Ping? Approving now anyway, since the last details that could be changed aren't
blockers.
Comment 20 Marcela Mašláňová 2008-05-13 07:22:13 EDT
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 07:36:10 EDT
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 10:01:56 EDT
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 10:24:49 EDT
So is this all fixed and reviewed now?
Comment 24 Marcela Mašláňová 2008-07-18 10:30:49 EDT
Yes, it is. It should be closed by reviewer.
Comment 25 Matthias Saou 2008-08-08 07:21:30 EDT
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.