Bug 219097 - Review Request: vdr-wapd - WAP daemon for VDR
Summary: Review Request: vdr-wapd - WAP daemon for VDR
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thorsten Leemhuis
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2006-12-10 18:44 UTC by Ville Skyttä
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-03-29 16:19:34 UTC
Type: ---
Embargoed:
fedora: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)

Description Ville Skyttä 2006-12-10 18:44:37 UTC
http://cachalot.mine.nu/6/SRPMS/vdr-wapd.spec
http://cachalot.mine.nu/6/SRPMS/vdr-wapd-0.8-15.cmn6.src.rpm

The wapd plugin lets VDR listen to WAP requests to allow remote
control by WML enabled browsers such as mobile phones.

Comment 1 Thorsten Leemhuis 2007-03-24 16:27:55 UTC
Note: This is a review without testing the runtime capabilities -- I don't have
the hardware and software setup to test it at hand, so I'll focus on the package
itself. I'm confident it'll work, as it is in a nother repo for some time
already. If someone has a problems with it speak up, but testing runtime
capabilities is afaics no must (it's only a should).

 rpmlint
E: vdr-wapd non-standard-uid /etc/vdr/plugins/wapaccess vdr
E: vdr-wapd non-readable /etc/vdr/plugins/wapaccess 0640
E: vdr-wapd non-standard-uid /etc/vdr/plugins/waphosts vdr
E: vdr-wapd non-readable /etc/vdr/plugins/waphosts 0640
 acceptable in this case afaics.

 package meets naming and versioning guidelines.
 specfile is properly named, is cleanly written and uses macros consistently.
 dist tag is present.
 build root is correct.
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 license field matches the actual license.
 license is open source-compatible. 
 BuildRequires are proper.
 %clean is present.
 package builds in mock.
 package installs properly
 debuginfo package looks complete.
 final provides and requires are sane:

 owns the directories it creates.
 doesn't own any directories it shouldn't.
 no duplicates in %files.
 no scriptlets present.
 code, not content.
 documentation is small, so no -docs subpackage is necessary.
 %docs are not necessary for the proper functioning of the package.
 no headers.
 no pkgconfig files.
 no libtool .la droppings.
 not a GUI app.

Problems: 
 * upstream not reachable atm; did it move?

 * the uid stuff rpmlint complains about is acceptable, but shouldn't it
"Requires(pre): vdr", to make sure the users gets created before installation of
the plug-in?

Notes:
 * regarding the selinux comment in vdr-wapd-proxy.conf -- isn't there a
possibility to make it "simply work" without offloading configuration to the user?

 * "BuildRequires:  sed >= 3.9.5" -- sed is in the Exceptions list in the
Packaging Guidelines, so this should not be needed afaics

 * Regarding the summary: would be nice to have a slightly more verbose one, as
probably many users won#t know what a "WAP daemon" is or does

 * what's that "LIBDIR=." in the makefile? Looks suspicious; a comment might be
nice, if there is a good reasons for this (I suppose there is)

Comment 2 Ville Skyttä 2007-03-24 17:01:28 UTC
(In reply to comment #1)
>  * upstream not reachable atm; did it move?

No idea, it was there when I last checked not too long ago.  Sent mail upstream
asking what's up.

>  * the uid stuff rpmlint complains about is acceptable, but shouldn't it
> "Requires(pre): vdr", to make sure the users gets created before installation 
> of the plug-in?

No, there's no dependency loop present, so plain Requires: (on vdr(abi))
suffices.  Further, there's no %pre script that would require those users
(actually, no %pre script at all), so Requires(pre) wouldn't be quite correct
anyway. 
http://rpm.org/max-rpm-snapshot/s1-rpm-depend-manual-dependencies.html#S3-RPM-DEPEND-FINE-GRAINED

>  * regarding the selinux comment in vdr-wapd-proxy.conf -- isn't there a
> possibility to make it "simply work" without offloading configuration to the 
> user?

I don't know, but I'd say it's a generic mod_proxy issue, not particularly a
vdr-wapd one.  Note that proxying is completely optional and the proxy snippet
is installed as a doc only to avoid a dependency on httpd, so user configuration
is required anyway if one wishes to do that.

>  * what's that "LIBDIR=." in the makefile? Looks suspicious; a comment might 
> be nice, if there is a good reasons for this (I suppose there is)

It's the same in all VDR plugins, just a directory where the compiled shared
object is copied into during build - VDR plugins have kind of a combined
build+install step (see Makefile and also the VDR plugin creator helper
scriptlet in /usr/bin/vdr-newplugin if you have vdr-devel installed).  Added a
one-liner comment about it.

http://cachalot.mine.nu/6/SRPMS/vdr-wapd.spec
http://cachalot.mine.nu/6/SRPMS/vdr-wapd-0.8-16.cmn6.src.rpm

* Sat Mar 24 2007 Ville Skyttä <ville.skytta at iki.fi> - 0.8-16
- Improvement suggestions from #219097: drop build dependency on sed,
  improve summary and description.



Comment 3 Ville Skyttä 2007-03-26 18:30:33 UTC
Upstream replied, telling that the site is undergoing a hosting provider change
and thus is expected to be gone/broken for a while.  The plugin homepage is
available temporarily at http://www.heiligenmann.de/vdr/vdr/plugins/wapd.html
where sources can be verified; I'll alter the URL and SourceX locations in the
specfile later when I hear about what's the new "permanent" URL.

Comment 4 Thorsten Leemhuis 2007-03-27 17:12:25 UTC
k, then everything looks fine afaics

APPROVED



Comment 5 Ville Skyttä 2007-03-27 18:23:01 UTC
Thanks!

New Package CVS Request
=======================
Package Name: vdr-wapd
Short Description: WAP remote control interface for VDR
Owners: ville.skytta
Branches: FC-6
InitialCC: 

Comment 6 Ville Skyttä 2007-03-29 16:19:34 UTC
Done.


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