(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.