Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 190343 - Review Request: VDR - Video Disk Recorder
Summary: Review Request: VDR - Video Disk Recorder
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT 190344 190345 190346
TreeView+ depends on / blocked
 
Reported: 2006-05-01 14:01 UTC by Ville Skyttä
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-10-16 19:12:22 UTC
Type: ---
Embargoed:
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Ville Skyttä 2006-05-01 14:01:34 UTC
http://www.cadsoft.de/vdr/
http://cachalot.mine.nu/5/SRPMS/vdr-1.4.0-2.src.rpm

VDR implements a complete digital set-top-box and video recorder.
It can work with signals received from satellites (DVB-S) as well as
cable (DVB-C) and terrestrial (DVB-T) signals.  At least one DVB card
is required to run VDR.

Comment 1 Ville Skyttä 2006-05-01 14:08:15 UTC
(More plugins may follow later if/when this package is in.)

Comment 2 Enrico Scholz 2006-05-29 06:32:39 UTC
Sorry for the delay...

Comments:

* you are writing

  | %{__patch} -i ...
  | ...
  | ... make ...
  | install ...

  For consistency, you should either use everywhere the '%__XXXX'
  macros, or everywhere only 'XXXX'.

* the version in

  | BuildRequires:  glibc-kernheaders >= 2.4-9.1.94

  is unneeded; every target system has this version of the
  glibc-kernheaders package

* you are placing files into an unowned directory; so (despite of the
  no-ordered-package-removal problem) the

  | Requires: udev

  should be

  | Requires(pre):    udev
  | Requires(postun): udev

  (or '/etc/udev/rules.d' instead of 'udev')

  to guarantee that the directory exists before vdr files will be
  placed into it.

* the vdr recordings might be shared between several machines. It
  would be nice when the generated UIDs/GIDs would be identical
  everywhere. So please use 'fedora-usermgt' for the 'vdr' user and
  'video' group.

* I am not sure about the 'vdr' username; three-letter usernames
  produce a high chance for conflicts with local usernames so I would
  avoid them and use e.g. 'vdrdaemon' instead of.

  On the other side, 'vdr' has some history and not using 'vdr' might
  cause other problems

* ownership/location of the vdr configuration directory is another
  problem. I dislike the vdr:video owned /etc/vdr directory somehow
  because:

  * some configuration data is modified by the 'vdr' daemon (channels.conf,
    remote.conf, setup.conf) so it should be located in /var/lib/vdr

  * not all configuration data should be modifiable by the daemon
    (e.g. commands.conf) so permissions should should be root:video.


* rpmlint generates lot of

  | E: vdr non-standard-uid ... vdr
  | E: vdr non-standard-gid ... vdr

  warnings which can be ignored.

  Ditto for

  | W: vdr non-conffile-in-etc /etc/sysconfig/vdr-plugins.d/README


  The

  | E: vdr zero-length /etc/vdr/setup.conf

  is related to the previous comment about the configuration files

* I use the following tweak to the udev-rules to generates a predictable
  event device for the remote-control:

  | SUBSYSTEM=="input", SYSFS{../name}=="DVB on-card IR receiver", SYMLINK+="input/event-remote", GROUP="video", MODE="0660"

  This is valid for a Hauppauge Nexus-S; other cards will need another
  name.

Comment 3 Ville Skyttä 2006-05-29 18:49:03 UTC
(In reply to comment #2)
> Sorry for the delay...

NP, should the blocker be moved from FE-NEW to FE-REVIEW? ;)

> use everywhere the '%__XXXX' macros, or everywhere only 'XXXX'.

Done (the latter).

>   | BuildRequires:  glibc-kernheaders >= 2.4-9.1.94
>   is unneeded; every target system has this version of the
>   glibc-kernheaders package

Dropped altogether per
https://www.redhat.com/archives/fedora-maintainers/2006-May/msg00071.html

>   | Requires: udev
>   should be
>   | Requires(pre):    udev
>   | Requires(postun): udev

Disagreed.  Even if for some obscure reason udev would be installed after vdr
(already a very unlikely case), neither will automatically start during the
transaction, and after the transaction udev is available and has changed the dir
ownerships so it doesn't matter.

>   (or '/etc/udev/rules.d' instead of 'udev')

We really need udev instead of the dir, so I'm leaving that as is.

> please use 'fedora-usermgt' for the 'vdr' user and 'video' group.

I'm not going to add a dependency on fedora-usermgmt (not parroting my opinions
about it here, as I've already done that several times on mailing lists).  If
you think testing for availability of the f-u scripts and using them in %pre if
available would be useful, I could be persuaded to do that.  But I'd rather
leave that stuff out altogether.

>   On the other side, 'vdr' has some history and not using 'vdr' might
>   cause other problems

Seconded, left as is.

> * ownership/location of the vdr configuration directory is another
>   problem. I dislike the vdr:video owned /etc/vdr directory somehow
>   because:
>   * some configuration data is modified by the 'vdr' daemon (channels.conf,
>     remote.conf, setup.conf) so it should be located in /var/lib/vdr

Kind of agreed, but this package has an existing user base and I don't see too
good ways to migrate it on upgrades and would not at all want to break existing
setups either.  Do you have ideas how that could be done?

>   * not all configuration data should be modifiable by the daemon
>     (e.g. commands.conf) so permissions should should be root:video.

Do you mean for the config dir, and if yes, root:video 0755 or 0775?  What about
subdirs there?  The former would require going through quite a bit of code and
ensuring that the daemon and its plugins operate in a way that they'd still work
without having write access to the affected dirs, and the latter might be too
relaxed.  Doing the former and patching where needed would be a good thing to
do, but I think it's somewhat out of scope for the package's acceptance.

>   | SUBSYSTEM=="input", SYSFS{../name}=="DVB on-card IR receiver",
SYMLINK+="input/event-remote", GROUP="video", MODE="0660"

Added as an example to the udev rules file.

One thing outside of this list I'm considering to do is to rename the "runvdr"
script to something else; while it's somewhat compatible with the upstream
runvdr example, it doesn't implement the implied functionality (auto-restart on
certain exit codes, reloading of DVB modules).  Or implementing at least some of
that in the version shipped with the package.  Thoughts?

http://cachalot.mine.nu/5/SRPMS/vdr-1.4.0-5.src.rpm

* Mon May 29 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.0-5
- Address some review notes in #190343 comment 2:
- Add example udev rule for predictable remote control device naming.
- Drop glibc-kernheaders build dependency.
- Specfile cleanups.

* Sun May 28 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.0-4
- Apply upstream 1.4.0-2 maintenance patch.

* Sun May 14 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.0-3
- Apply upstream 1.4.0-1 maintenance patch.
- Drop unneeded version check from %%check.


Comment 4 Ville Skyttä 2006-06-14 16:19:04 UTC
http://cachalot.mine.nu/5/SRPMS/vdr-1.4.1-1.src.rpm

* Mon Jun 12 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-1
- 1.4.1, liemikuutio 1.6.

Comment 5 Ville Skyttä 2006-06-18 12:02:04 UTC
http://cachalot.mine.nu/5/SRPMS/vdr-1.4.1-2.src.rpm

* Sun Jun 18 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-2
- 1.4.1-1 + 1.4.1-1.ds.
- Drop glibc-kernheaders dependency from -devel too.
- Make -devel multilib friendly, add pkgconfig file.


Comment 6 Ville Skyttä 2006-07-04 17:37:46 UTC
http://cachalot.mine.nu/5/SRPMS/vdr-1.4.1-4.src.rpm

* Sat Jul  1 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-4
- Update liemikuutio patch to 1.7.
- Conditionally build the skincurses and sky plugins; disabled by default,
  rebuild with "--with plugins" to enable.
- Make symlinks relative.

* Fri Jun 23 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-3
- Move headers to %%{_includedir}.
- Add README.package to docs, describing some aspects of the package (#1063).
- Add LIBDIR to Make.config to ease local plugin builds (#1063).
- Update VDR_PLUGIN_ORDER in sysconfig snippet, loading potential output
  plugins before others.  See commentary in the file for details.
- Add example how to affect OSD time/date formats to sysconfig snippet.


Comment 7 Ville Skyttä 2006-08-06 13:55:03 UTC
http://cachalot.mine.nu/5/SRPMS/vdr-1.4.1-8.src.rpm

* Sun Aug 06 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-8
- Apply upstream 1.4.1-3 maintenance patch.

* Sun Jul 23 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-7
- Apply upstream 1.4.1-2 maintenance patch.
- Use VFAT compatible recording names by default.

* Sun Jul 16 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-6
- Don't use %bcond_with to appease buildsys.

* Sat Jul 15 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-5
- Update liemikuutio patch to 1.8.
- Patch dumpability to work with PR_SET_DUMPABLE changes in recent kernels,
  add corresponding warning to sysconfig snippet comment.


Comment 8 Ville Skyttä 2006-08-24 21:53:02 UTC
http://cachalot.mine.nu/5/SRPMS/vdr-1.4.1-11.src.rpm

* Mon Aug 21 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-11
- Set device permissions in both console.perms and udev (#202132).
- Sync restart and DVB module reload functionality with upstream runvdr.

* Fri Aug 18 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-10
- Fix build with recent kernel headers where _syscallX are no longer visible.
- Drop ia64 patch (superseded by the above) and the thread poison patch.

* Fri Aug 11 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.1-9
- Set device permissions using console.perms instead of udev rules
  to work around new pam trumping udev config (#202132).


Comment 9 Ville Skyttä 2006-08-27 13:10:16 UTC
http://cachalot.mine.nu/5/SRPMS/vdr-1.4.2-1.src.rpm

* Sun Aug 27 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.2-1
- 1.4.2, syscall and maintenance patches applied upstream.


Comment 10 Ville Skyttä 2006-09-19 20:42:41 UTC
As VDR deals with MPEG data from DVB cards, adding blocker on FE-Legal even
though I guess there should be no problems as the DVB hardware (or 3rd party
plugins not included here) does the actual decoding.  For more info, the VDR
homepage is http://www.cadsoft.de/vdr/

Comment 11 Ville Skyttä 2006-09-26 19:05:22 UTC
http://cachalot.mine.nu/5/SRPMS/vdr.spec
http://cachalot.mine.nu/5/SRPMS/vdr-1.4.3-1.src.rpm

* Sat Sep 23 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.3-1
- 1.4.3, 1.4.2-1.ds, liemikuutio 1.12.

* Sun Sep  3 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.2-2
- 1.4.2-1, liemikuutio 1.10.


Comment 12 Tom "spot" Callaway 2006-10-14 21:40:41 UTC
I don't see a reason for FE-Legal here. VDR isn't encoding or decoding MPEG
data. If the ability to parse/save an MPEG file is illegal, we have much bigger
legal concerns than VDR. Lifting FE-Legal, unless you really want a lawyer to
look at it. :)

Comment 13 Kevin Fenzi 2006-10-14 23:51:39 UTC
I'll take a stab at a review on this: 

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
9bb82d1f090dad746d784d147dbb0126  vdr-1.4.3.tar.bz2
9bb82d1f090dad746d784d147dbb0126  vdr-1.4.3.tar.bz2.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Headers/static libs in -devel subpackage.
OK - .pc files in -devel subpackage/requires pkgconfig
See below - -devel package Requires: %{name} = %{version}-%{release}
OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
See below - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
i386/x86_64 - Should build on all supported archs
See below - Should have subpackages require base package with fully versioned 
depend.
OK - Should have dist tag
OK - Should package latest version

Issues:

1. Shouldn't the -devel subpackage require:
Requires: %{name} = %{version}-%{release}
(And/Or perhaps the api version since you know it?)

2. Should these be even packaged since they don't contain anything:
%{_libdir}/vdr
%{_libdir}/bin
%{_datadir}/vdr
%{_datadir}/vdr/logos
Or are they needed for some of the other packages that depend on this one?

3. rpmlint says:

W: vdr conffile-without-noreplace-flag /etc/security/console.perms.d/95-
vdr.perms
(The note in the spec file explains this one)

E: vdr non-standard-uid /etc/vdr/diseqc.conf vdr
E: vdr non-standard-gid /etc/vdr/diseqc.conf video
E: vdr non-standard-uid /etc/vdr/reccmds.conf vdr
E: vdr non-standard-gid /etc/vdr/reccmds.conf video
E: vdr non-standard-uid /etc/vdr/themes vdr
E: vdr non-standard-gid /etc/vdr/themes video
E: vdr non-standard-uid /var/cache/vdr vdr
E: vdr non-standard-gid /var/cache/vdr video
E: vdr non-standard-uid /var/lib/vdr vdr
E: vdr non-standard-gid /var/lib/vdr video
E: vdr non-standard-uid /srv/audio vdr
E: vdr non-standard-gid /srv/audio video
E: vdr non-standard-uid /etc/vdr/themes/classic-default.theme vdr
E: vdr non-standard-gid /etc/vdr/themes/classic-default.theme video
E: vdr non-standard-uid /etc/vdr/remote.conf vdr
E: vdr non-standard-gid /etc/vdr/remote.conf video
E: vdr non-standard-uid /var/cache/vdr/epg.data vdr
E: vdr non-standard-gid /var/cache/vdr/epg.data video
E: vdr non-standard-uid /etc/vdr/timers.conf vdr
E: vdr non-standard-gid /etc/vdr/timers.conf video
E: vdr non-standard-uid /srv/vdr vdr
E: vdr non-standard-gid /srv/vdr video
E: vdr non-standard-uid /etc/vdr vdr
E: vdr non-standard-gid /etc/vdr video
E: vdr non-standard-uid /etc/vdr/channels.conf vdr
E: vdr non-standard-gid /etc/vdr/channels.conf video
E: vdr non-standard-uid /etc/vdr/themes/sttng-default.theme vdr
E: vdr non-standard-gid /etc/vdr/themes/sttng-default.theme video
E: vdr non-standard-uid /etc/vdr/svdrphosts.conf vdr
E: vdr non-standard-gid /etc/vdr/svdrphosts.conf video
E: vdr non-standard-uid /etc/vdr/keymacros.conf vdr
E: vdr non-standard-gid /etc/vdr/keymacros.conf video
E: vdr non-standard-uid /etc/vdr/sources.conf vdr
E: vdr non-standard-gid /etc/vdr/sources.conf video
E: vdr non-standard-uid /etc/vdr/setup.conf vdr
E: vdr non-standard-gid /etc/vdr/setup.conf video
E: vdr non-standard-uid /etc/vdr/plugins vdr
E: vdr non-standard-gid /etc/vdr/plugins video
E: vdr non-standard-uid /var/run/vdr vdr
E: vdr non-standard-gid /var/run/vdr video
E: vdr non-standard-uid /etc/vdr/commands.conf vdr
E: vdr non-standard-gid /etc/vdr/commands.conf video

All those can be ignored most likely.

W: vdr incoherent-subsys /etc/rc.d/init.d/vdr $prog

Something going on with the init sript or pid file?

E: vdr-devel only-non-binary-in-usr-lib

This can be ignored, there are binaries in /usr/sbin, so
this can't be noarch.

W: vdr non-conffile-in-etc /etc/sysconfig/vdr-plugins.d/README

This README is probibly needed to explain the plugins?

E: vdr zero-length /etc/vdr/remote.conf
E: vdr zero-length /etc/vdr/setup.conf
E: vdr zero-length /etc/vdr/channels.conf
E: vdr zero-length /etc/vdr/timers.conf 

Your note in the spec explains these.  


Comment 14 Ville Skyttä 2006-10-15 07:37:23 UTC
(In reply to comment #13)
> I'll take a stab at a review on this: 

Thanks!

> 1. Shouldn't the -devel subpackage require:
> Requires: %{name} = %{version}-%{release}
> (And/Or perhaps the api version since you know it?)

The devel subpackage is really independent of the main package.  The only 
thing that I can think of justifying the above dependency would be ownership 
of the %{_libdir}/vdr directory, but I think owning it in both the main 
package and devel is harmless and better than adding the dependency.  If you 
have strong opinions about this, I don't insist on it though.

> 2. Should these be even packaged since they don't contain anything:
> %{_libdir}/vdr
> %{_libdir}/bin
> %{_datadir}/vdr
> %{_datadir}/vdr/logos
> Or are they needed for some of the other packages that depend on this one?

Yes, they're used by plugin packages.

%{_libdir}/vdr is the dir where plugins are installed to and from where VDR 
loads them.  %{_libdir}/vdr/bin (not %{_libdir}/bin) contains various 
executables for plugins - those helpers are supposed to be in $PATH but 
there's no need to put them in eg. %{_bindir}.  %{_libdir}/vdr/bin is put into 
the vdr process's $PATH by the init script.  %{_datadir}/vdr is for various 
static data files for plugins, such as channel logos etc.

> W: vdr incoherent-subsys /etc/rc.d/init.d/vdr $prog
> Something going on with the init sript or pid file?

Nah, rpmlint bug, see bug 210261 comment 4 and later comments there.

> W: vdr non-conffile-in-etc /etc/sysconfig/vdr-plugins.d/README
> This README is probibly needed to explain the plugins?

Yes, it was needed for that.  Nowadays this and other packaging related things 
are documented in /usr/share/doc/vdr-*/README.package too, so it can be 
removed.

http://cachalot.mine.nu/6/SRPMS/vdr.spec
http://cachalot.mine.nu/6/SRPMS/vdr-1.4.3-3.src.rpm

* Sun Oct 15 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.4.3-3
- Apply upstream 1.4.3-1 maintenance patch.
- Sync with 1.4.3-1.ds, update liemikuutio patch to 1.13.
- Drop no longer needed README.plugins.d, README.package is enough (#190343).


Comment 15 Kevin Fenzi 2006-10-16 18:09:19 UTC
ok, I think that addresses all the questions I had. 
The version from comment #14 builds just fine here. 

This package is APPROVED. 
Don't forget to close this bug NEXTRELEASE once it's been imported and built. 

Also considering doing a review of a waiting package to spread out the 
reviewing load. ;) 

Comment 16 Ville Skyttä 2006-10-16 19:12:22 UTC
Devel built, FC-5 branch requested, owners.list and comps updated.  Thanks!

(In reply to comment #15)
> Also considering doing a review of a waiting package to spread out the 
> reviewing load. ;) 

Will try to find something... :)

Comment 17 Ville Skyttä 2007-10-16 21:10:42 UTC
Package Change Request
======================
Package Name: vdr
New Branches: F-8

Comment 18 Kevin Fenzi 2007-10-16 22:22:09 UTC
cvs done.


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