Bug 585607 - Review Request: gnump3d - The GNU Streaming MP3 / OGG Vorbis Server
Summary: Review Request: gnump3d - The GNU Streaming MP3 / OGG Vorbis Server
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-04-25 01:46 UTC by Paulo Roma Cavalcanti
Modified: 2010-12-22 17:55 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-12-22 17:55:35 UTC
Type: ---
Embargoed:
j: fedora-review?


Attachments (Terms of Use)

Description Paulo Roma Cavalcanti 2010-04-25 01:46:57 UTC
Spec URL: 
http://roma.fedorapeople.org/specs/gnump3d.spec

SRPM URL: 
http://roma.fedorapeople.org/srpms/gnump3d-3.0-3.fc12.src.rpm

Description:

GNUMP3d is a streaming server for MP3's, OGG vorbis, and other
streamable audio files. It is designed to be:

* Small, stable, self-contained, and secure.
* Simple to install, configure, and use.
* Portable across different varieties of Unix.

Scratch build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=2135876

Rpmlint output:

[cascavel:~/RPMS124/lcgrpms] rpmlint gnump3d-3.0-3.fc12.noarch.rpm
gnump3d.noarch: W: spelling-error %description -l en_US vorbis -> Orbison, orbits, forbids
gnump3d.noarch: W: spelling-error %description -l en_US streamable -> stream able, stream-able, streamline
gnump3d.noarch: W: non-standard-uid /var/cache/gnump3d gnump3d
gnump3d.noarch: W: non-standard-gid /var/cache/gnump3d gnump3d
gnump3d.noarch: W: non-standard-uid /var/log/gnump3d/error.log gnump3d
gnump3d.noarch: W: non-standard-gid /var/log/gnump3d/error.log gnump3d
gnump3d.noarch: W: non-conffile-in-etc /etc/logrotate.d/gnump3d
gnump3d.noarch: W: non-standard-uid /var/cache/gnump3d/song.tags gnump3d
gnump3d.noarch: W: non-standard-gid /var/cache/gnump3d/song.tags gnump3d
gnump3d.noarch: W: non-standard-uid /var/log/gnump3d gnump3d
gnump3d.noarch: W: non-standard-gid /var/log/gnump3d gnump3d
gnump3d.noarch: W: non-standard-uid /var/log/gnump3d/access.log gnump3d
gnump3d.noarch: W: non-standard-gid /var/log/gnump3d/access.log gnump3d
gnump3d.noarch: W: non-standard-uid /var/cache/gnump3d/serving gnump3d
gnump3d.noarch: W: non-standard-gid /var/cache/gnump3d/serving gnump3d
1 packages and 0 specfiles checked; 0 errors, 15 warnings.

Comment 1 Jason Tibbitts 2010-11-19 14:29:20 UTC
It's nice that you posted rpmlint output.  However, it's expected that you'll also address that output.

Also, you missed this one:
  gnump3d.src:14: W: unversioned-explicit-provides perl(gnump3d::readtags)
because it looks like you didn't run rpmlint on the src.rpm.  You should always do that.

I think all of the rpmlint complaints are justifiable, but it's expected that you will justify them, not the reviewer.

Does this really have no external dependencies on any kind of mp3 or vorbis decoder?

Why would you bzip the init and logrotate files?  It doesn't save any space and only makes them more difficult to inspect.

This does not seem to follow the packaging guidelines for adding the user and group:  https://fedoraproject.org/wiki/Packaging:UsersAndGroups

Comment 2 Paulo Roma Cavalcanti 2010-11-20 10:39:05 UTC
(In reply to comment #1)
> It's nice that you posted rpmlint output.  However, it's expected that you'll
> also address that output.
> 
> Also, you missed this one:
>   gnump3d.src:14: W: unversioned-explicit-provides perl(gnump3d::readtags)
> because it looks like you didn't run rpmlint on the src.rpm.  You should always
> do that.

gnump3d does not work without this Provide statement, and I do not think it needs to be versioned.

> 
> I think all of the rpmlint complaints are justifiable, but it's expected that
> you will justify them, not the reviewer.
> 

The others rpmlint complaints are due to the new group and user created, and the logrotate file is in its appropriate place.

> Does this really have no external dependencies on any kind of mp3 or vorbis
> decoder?

I think not: 

http://www.linux.com/archive/feature/61154

> 
> Why would you bzip the init and logrotate files?  It doesn't save any space and
> only makes them more difficult to inspect.

I unpacked them in the new version.

> 
> This does not seem to follow the packaging guidelines for adding the user and
> group:  https://fedoraproject.org/wiki/Packaging:UsersAndGroups

Well, I just adopted the recommended template now. 

Spec URL: 
http://roma.fedorapeople.org/specs/gnump3d.spec

SRPM URL: http://roma.fedorapeople.org/srpms/gnump3d-3.0-4.fc12.src.rpm


Thnaks.

Comment 3 Jason Tibbitts 2010-11-23 03:33:39 UTC
OK, here's a review.  Unfortunately there is significant work to be done before this package can be acceptable.

Note that if you're not targeting RHEL4 or 5, you can remove BuildRoot:, %clean and the first line of %install.

Note that your manual "Requires: perl" is unnecessary; rpmlint already finds the perl dependency.  (Actually it finds four of them, but that's not really your problem.)

I was going to comment on the lack of indentation in your %pre script making it tough to read, but it looks like the example from the UsersAndGroups page somehow lost its indentation; I fixed it.

Your license tag seems to be incorrect.  All of the source files which have the proper GPL license header indicate GPLv2+.  Unpack the tarball and see:
  grep -rC1 'of the License' .
(This doesn't count the improperly bundled libraries, which are a separate issue.)

/etc/logrotate.d/gnump3d needs to be marked %config(noreplace) as admins do edit these files and will expect them to be preserved.  rpmlint complains about this:
  gnump3d.noarch: W: non-conffile-in-etc /etc/logrotate.d/gnump3d
It is indeed in the right place; rpmlint is properly warning that it's not marked properly as a config file.

I've no way to properly test this as all my music is FLAC.  I guess I could convert some of it, but it's taken long enough to review this as it is.

The file lib/gnump3d/WMA.pm seems to be a bundled copy of http://search.cpan.org/~daniel/Audio-WMA-1.3/.  This is a blocker.  It also makes me wonder what else they're bundling; a quick check shows IP.pm, MD5.pm and mp3info.pm are ripped off from some other packages as well.  It's nice that they say the software is self-contained, but achieving that through bundling is not proper and must be fixed.

* source files match upstream.  sha256sum:
  1ac5bd0e850b0e18ccd9d19219f5108fa84b50d8ae3825a361e8b907eab7f19c
   gnump3d-3.0.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
X license field should be GPLv2+
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper (none).
* package builds in mock (rawhide, x86_64).
* package installs properly.
X rpmlint has a valid complaint.
* final provides and requires are sane:
   config(gnump3d) = 3.0-4.fc15
   perl(gnump3d::IP) = 3.14
   perl(gnump3d::MD5) = 1.6
   perl(gnump3d::Request)  
   perl(gnump3d::WMA) = 0.7
   perl(gnump3d::base64)  
   perl(gnump3d::config) = 0.01
   perl(gnump3d::files)  
   perl(gnump3d::filetypes)  
   perl(gnump3d::lang::lookup)  
   perl(gnump3d::mp3info) = 1.13
   perl(gnump3d::mp4info) = 1.03
   perl(gnump3d::ogginfo) = 0.01
   perl(gnump3d::readtags)  
   perl(gnump3d::sorting) = 0.01
   perl(gnump3d::tagcache)  
   perl(gnump3d::url) = 0.01
   perl(plugins::copying) = 1..2
   perl(plugins::info) = 1..2
   perl(plugins::now) = 1..2
   perl(plugins::playlist) = 1..2
   perl(plugins::prefs) = 1..2
   perl(plugins::random) = 1..2
   perl(plugins::recent) = 1..2
   perl(plugins::search) = 1..2
   perl(plugins::size) = 1..2
   perl(plugins::stats) = 1..2
   perl(plugins::tagbrowse) = 1..2
   gnump3d = 3.0-4.fc15
  =
   /bin/bash  
   /bin/sh  
   /sbin/chkconfig  
   /sbin/service  
   /usr/bin/perl  
   config(gnump3d) = 3.0-4.fc15
?  perl  <------- unneeded manual dependency
   perl >= 0:5.004
   perl >= 0:5.005
   perl >= 0:5.005_62
   perl >= 0:5.006
   perl(AutoLoader)  
   perl(Carp)  
   perl(Encode)  
   perl(Encode::Guess)  
   perl(English)  
   perl(Env)  
   perl(Exporter)  
   perl(File::Find)  
   perl(Getopt::Long)  
   perl(IO::File)  
   perl(IO::Socket)  
   perl(POSIX)  
   perl(Pod::Usage)  
   perl(Socket)  
   perl(Symbol)  
   perl(gnump3d::IP)  
   perl(gnump3d::MD5)  
   perl(gnump3d::Request)  
   perl(gnump3d::WMA)  
   perl(gnump3d::config)  
   perl(gnump3d::files)  
   perl(gnump3d::filetypes)  
   perl(gnump3d::lang::lookup)  
   perl(gnump3d::mp3info)  
   perl(gnump3d::mp4info)  
   perl(gnump3d::ogginfo)  
   perl(gnump3d::readtags)  
   perl(gnump3d::sorting)  
   perl(gnump3d::tagcache)  
   perl(gnump3d::url)  
   perl(integer)  
   perl(overload)  
   perl(sigtrap)  
   perl(strict)  
   perl(vars)  
   perl(warnings)  
   shadow-utils  

X contains many bundled libraries.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files.
* scriptlets are OK (user/group creation).
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

Comment 4 Jason Tibbitts 2010-12-22 17:35:02 UTC
No response in a month; I guess I'll just close this out if nothing happens soon.


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