Bug 226119

Todd: Are you reviewing this package? 
Please set the fedora-review flag to ? if you are, and if not, you might want to
move it back to being assigned to nobody. 

Sorry for not getting to this one sooner Kevin.  I'll finish up a full review
shortly.

Greetings Chris and Nalin,

Here be a merge review for MAKEDEV.

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
See below - Meets Packaging Guidelines.
See below - License = GPLv2+
See below - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
See below - Sources match upstream md5sum
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK - Package compiles and builds on at least one arch. (see koji :)
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:
    MAKEDEV-3.23-3.i386.rpm
    Provides:
    ---------
    config(MAKEDEV) = 3.23-3
    MAKEDEV = 3.23-3
    Requires:
    ---------
    /bin/sh  
    /usr/sbin/groupadd  
    /usr/sbin/useradd  
    config(MAKEDEV) = 3.23-3
    grep  
    libc.so.6  
    libselinux.so.1  
    mktemp  
    rtld(GNU_HASH)  
    
    MAKEDEV-3.23-3.src.rpm
    Provides:
    ---------
    (none)
    Requires:
    ---------
    libselinux-devel

SHOULD Items:

OK - Should build in mock. (again see koji for proof :)
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have sane scriptlets.
OK - Should package latest version
See below - Outstanding bugs on package.

Issues:

1. http://fedoraproject.org/wiki/Packaging/SourceURL suggests adding a comment
above Source0 when we are upstream.  Would there be any value in setting up a
fedorahosted project for the sources to MAKEDEV?

2. The License tag needs to be updated.  However, the source package license
also needs adjusted to meet Red Hat policy.  The license should be GPLv2 only
(no "or any later version") according to CopyrightGuidelines on the Red Hat
intranet (so I am told).  MAKEDEV.c, mksock.c, and sel.h all include the "or any
later version" in the header.  This should be fixed and the License tag then
changed to GPLv2.

3. The buildroot tag must be one of the following, according to
http://fedoraproject.org/wiki/Packaging/Guidelines#line-181:
    %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
    %{_tmppath}/%{name}-%{version}-%{release}-root

4. rpmlint output:

$ rpmlint MAKEDEV-3.23-3.src.rpm 
MAKEDEV.src:10: W: buildprereq-use libselinux-devel
MAKEDEV.src:12: W: prereq-use /usr/sbin/groupadd, /usr/sbin/useradd, grep, mktemp
MAKEDEV.src:465: W: macro-in-%changelog post
MAKEDEV.src:494: W: macro-in-%changelog _sysconfdir
MAKEDEV.src: W: summary-ended-with-dot A program used for creating device files
in /dev.
MAKEDEV.src: W: invalid-license GPL

$ rpmlint MAKEDEV-3.23-3.i386.rpm
MAKEDEV.i386: W: summary-ended-with-dot A program used for creating device files
in /dev.
MAKEDEV.i386: W: invalid-license GPL
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/00macros
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01alsa
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01cciss
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01cdrom
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01console
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01dac960
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01ftape
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01generic
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01ia64
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01ibcs
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01ida
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01ide
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01ipfilter
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01isdn
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01linux-2.6.x
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01linux1394
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01mouse
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01qic
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01raid
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01redhat
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01s390
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01sound
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01std
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01undocumented
MAKEDEV.i386: W: conffile-without-noreplace-flag /etc/makedev.d/01v4l

I think the conffile-without-noreplace-flag be safely ignored, if it needs to
be.  But is it required for MAKEDEV to install the files in /etc/makedev.d from
the package, overwriting a users changes?  If /etc/makedev.d was marked as
noreplace, would things be highly likely to break if a user made some change to
one of the scripts and then MAKEDEV was updated?

Other than the config warning and the license tag, I've corrected the other
warnings and will attach a patch for your perusal.  I changed the user and group
creation slightly to match the guidelines in
http://fedoraproject.org/wiki/Packaging/UsersAndGroups

5. Are the requires on grep and mktemp still needed?  The only place I find
reference to them in the tarball is in the spec file.

6. There are two outstanding bugs:
    
Bug #196042: Permissions for /dev/net/tun are too restrictive.

    This looks like it could be closed without even adding the Conflicts
    proposed, since the kernel package it would conflict is not in any supported
    release.

Bug #425832: "MAKEDEV: mkdir: File exists" if SELinux context wrong on /dev

    A full fix for this might include making sure start_udev calls restorecon if
    it mounts /dev.  It does seem like it'd be nice to try and catch this and
    give a more useful error.  But it's certainly not a hugely pressing bug.

Created attachment 296525 [details]
Patch cleaning up most of the rpmlint complaints and other minor issues for merge review

Feel free to use some/all/none of this, with or without attribution.  I seek
neither fame nor fortune in the MAKEDEV changelog. :)

Okay, I took all your changes and also changed the license and added the
noreplace bit.  Thanks for the review and the patch.  This is all in build 3.23-4.

Thanks.  All looks good now.  One less merge review in the queue.

APPROVED