Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: VDR - Video Disk Recorder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190343
------- Additional Comments From ville.skytta@iki.fi 2006-05-29 14:49 EST ------- (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.