Fedora Merge Review: MAKEDEV http://cvs.fedora.redhat.com/viewcvs/devel/MAKEDEV/ Initial Owner: nalin
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