Bug 456038 (DarkIce) - Review Request: DarkIce - Live Audio Streamer
Summary: Review Request: DarkIce - Live Audio Streamer
Keywords:
Status: CLOSED NEXTRELEASE
Alias: DarkIce
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Brian Pepple
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-07-21 04:28 UTC by Clint Savage
Modified: 2008-12-04 01:32 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2008-12-04 01:32:43 UTC
Type: ---
Embargoed:
bdpepple: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Clint Savage 2008-07-21 04:28:12 UTC
SRPM URL: http://herlo.fedorapeople.org/files/darkice-0.19-1.fc9.src.rpm
Spec URL: http://herlo.fedorapeople.org/files/darkice.spec
Description: DarkIce is a live audio streamer. It records audio from an audio interface (e.g. sound card), encodes it and sends it to a streaming server.

DarkIce can record from:
* OSS audio devices
* ALSA audio devices
* Solaris audio interface
* Jack sources
* uLaw audio input through a serial interface

As an aside, it supports lame and faac libs, but these are not included.

This is my first package, I need a sponsor.

Comment 1 Clint Savage 2008-07-21 06:15:16 UTC
While working on this spec file, I was attempting to get it to build with mock.
 Because of this, there is a small patch that was generated as well.  Thanks to
ricky for the patch and ianweller for general help.

Patch0: http://herlo.fedorapeople.org/files/darkice-0.19-configure.patch

I've also updated the src.rpm and the spec file to match.

Comment 2 Tim Colles 2008-07-25 09:20:38 UTC
Hi. This is just an informal review with some comments you might
find helpful.

  * The description line starting "DarkIce is a live ..." exceeds 79
    characters and wraps around on a standard terminal, you should
    break this up.

  * Your %changelog entry version is 0.19.1-1 but your package name
    and version is 0.19-1 - they should be consistent.

  * The "GPL" license is invalid it should be "GPLv2" or "GPLv2+" for
    example, see https://fedoraproject.org/wiki/Licensing.

  * The Group "Applications/Sound and Video" does not exist in the
    official list in /usr/share/doc/rpm-4.4.2.3/GROUPS (or equivalent
    rpm version) whereas it should be one from this list according
    to http://docs.fedoraproject.org/drafts/rpm-guide-en/ch13s02.html
    (section 13.2.2.7), although I don't know how current that is.

  * You should have a %config in front of %{_sysconfdir}/darkice.cfg
    as its a configuration file (then changes to it will be preserved
    across rpm upgrades).

  * The "Requires: libogg" and "Requires: libvorbis" lines are superfluous,
    you should let rpm find these lib dependencies on its own.

  * Consider using %{name}/%{version} in the Source0 line (since you have
    used them in the Patch0 line)?

  * You use $RPM_BUILD_ROOT in the %install block but %{buildroot} in the
    %clean block - choose one or the other and be consistent.

  * Provided configuration file is broken when program run as I got
    the following error:

      DarkIce: ConfigSection.cpp:117: format missing in section icecast-0 [0]

    It seems inconsistent with the man page which says "format" applies
    to [icecast2-x] sections not [icecast-x] sections.

  * Not sure you need an explicit BuildRequires line for "pkgconfig", nor
    libogg and libvorbis (as these are pre-requisites of the equivalent
    -devel packages anyway).


Comment 3 Clint Savage 2008-07-25 19:44:53 UTC
Thank you for the review.

Updated SPEC/SRPM are now available in the same location as above.

I've updated and fixed all of the above issues, with the exception of the following:

 * Provided configuration file is broken when program run as I got
    the following error:

      DarkIce: ConfigSection.cpp:117: format missing in section icecast-0 [0]

    It seems inconsistent with the man page which says "format" applies
    to [icecast2-x] sections not [icecast-x] sections.

The issue with this error is that you are attempting to use darkice to connect
with an icecast 1.x.x server.  In the man page, it specifically states that the
[icecast-x] sections are to be used only with the lame encoder.  

From the man page:

----------------------------------------------------------------------------

[icecast-x]

       This  section describes an output to an IceCast 1.3.x server or Darwin
Streaming Server , while encoding with a lame encoder.

---------------------------------------------------------------------------

Since Fedora doesn't support lame, twolame or faac, this support has been
removed from the package.  My question is, does that mean we should modify the
man page to match?  Or leave it in place?

Cheers,

Clint

Comment 4 Rahul Sundaram 2008-08-15 02:43:38 UTC
Can't the lame related items be run-time detected? Would you ask upstream if they are interested in something like this or gstreamer? If it can't be done, might be worth it to patch the man page or add a README.dist file explaining what has been changed or maybe provide a macro to rebuild it.

Comment 5 Clint Savage 2008-08-15 03:48:15 UTC
Rahul,

What do you mean 'lame related items be run-time detected'?  The application can be built without the lame libraries altogether, no patch needed.  Why would we want to include them?

Cheers,

Clint

Comment 6 Rahul Sundaram 2008-08-15 04:01:33 UTC
I didn't say we would want to include them. I was thinking of a xine-lib like solution to the problem where third repos can add what is being removed here.

Comment 7 Clint Savage 2008-08-15 04:13:12 UTC
Rahul,

Oh, if a thirdparty repo wants to build darkice-nonfree or something, its a simple matter of requiring the right libraries.  The configure script would take care of that already.  I have simply omitted those libraries in my build.

Make more sense?

Comment 8 Brian Pepple 2008-10-12 04:20:49 UTC
MD5Sum:
590c152cde2d62fef422f9f773560e95  darkice-0.19.tar.gz

Good:
* Source URL is canonical
* Upstream source tarball verified
* Package name conforms to the Fedora Naming Guidelines
* Group Tag is from the official list
* Buildroot has all required elements
* All paths begin with macros
* Make succeeds even when %{_smp_mflags} is defined
* Files have appropriate permissions and owners
* Rpmlint produces only the following warning, which can be safely ignored:
darkice.x86_64: W: conffile-without-noreplace-flag /etc/darkice.cfg
* Package installs and uninstalls cleanly

Bad:
* License should be GPLv2+, which you can fix when you import the package into CVS.

+1 APPROVED, and I'll also be you sponsor.

Comment 9 Clint Savage 2008-10-15 21:49:46 UTC
New Package CVS Request
=======================
Package Name: darkice
Short Description:  Live audio streamer
Owners: herlo
Branches: F-9, F-10
InitialCC:

Comment 10 Kevin Fenzi 2008-10-15 22:02:13 UTC
cvs done.

Comment 11 Brian Pepple 2008-12-04 01:09:20 UTC
Hey Clint, if this has been built you can close the bug.

Step #13: http://fedoraproject.org/wiki/PackageMaintainers/NewPackageProcess


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