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
Summary: | Review Request: VDR - Video Disk Recorder | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ville Skyttä <scop> |
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | rh-bugzilla |
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-10-16 19:12:22 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779, 190344, 190345, 190346 |
Description
Ville Skyttä
2006-05-01 14:01:34 UTC
(More plugins may follow later if/when this package is in.) 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. (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. 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. 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. 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. 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. 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). 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. 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/ 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. 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. :) 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. (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). 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. ;) 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... :) Package Change Request ====================== Package Name: vdr New Branches: F-8 cvs done. |