Bug 221084 - Review Request: dkms - Dynamic Kernel Module Support
Review Request: dkms - Dynamic Kernel Module Support
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael E Brown
Fedora Package Reviews List
Depends On:
  Show dependency treegraph
Reported: 2007-01-01 09:26 EST by Matt Domsch
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2007-06-22 12:29:36 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mebrown: fedora‑review+

Attachments (Terms of Use)

  None (edit)
Description Matt Domsch 2007-01-01 09:26:40 EST
Spec URL: http://domsch.com/linux/fedora/extras/dkms/dkms.spec
SRPM URL: http://download.fedora.redhat.com/pub/fedora/linux/extras/development/SRPMS/dkms-2.0.13-1.fc6.src.rpm
This package contains the framework for the Dynamic
Kernel Module Support (DKMS) method for installing
kernel module RPMS.

DKMS has been included in Fedora Extras since March 15, 2005, though we couldn't find a formal review ticket for it.  Gary Lerhaupt, Matt Domsch, and Tom Callaway all reviewed the spec at that time prior to its importation.
Comment 1 Till Maas 2007-03-07 12:04:24 EST
You use hardcoded directory names instead of macros, this violates the Packaging
Comment 2 Michael E Brown 2007-03-19 12:09:33 EDT
Package SRPM no longer resolves. 404.
Comment 3 Michael E Brown 2007-03-19 12:12:10 EDT
    $ rpmlint dkms-2.0.13-1.fc6.src.rpm
    W: dkms strange-permission dkms-2.0.13.tar.gz 0666
probably ok

    W: dkms strange-permission dkms.spec 0666
probably ok

    W: dkms unversioned-explicit-provides dkms-minimal
should fix?

    W: dkms macro-in-%changelog doc
should fix?

    W: dkms macro-in-%changelog kernelver
should fix?

    W: dkms no-%build-section
possibly add empty %build?

    W: dkms mixed-use-of-spaces-and-tabs (spaces: line 60, tab: line 28)
should fix?

    $ rpmlint ./dkms-2.0.13-1.fc6.noarch.rpm
    E: dkms devel-dependency kernel-devel
probably ok.

    E: dkms script-without-shebang /var/lib/dkms/dkms_dbversion
fix perms on this file? (no +x)

    W: dkms dangerous-command-in-%post mv
probably ok

    W: dkms dangerous-command-in-%trigger mv
probably ok

    E: dkms init-script-without-chkconfig-preun /etc/init.d/dkms_autoinstaller
should fix?

    W: dkms service-default-enabled /etc/init.d/dkms_autoinstaller
probably ok.

    E: dkms subsys-not-used /etc/init.d/dkms_autoinstaller
not sure what this means.

    W: dkms incoherent-init-script-name dkms_autoinstaller
probably ok.
Comment 4 Michael E Brown 2007-03-19 12:12:44 EDT
* rpmlint results above
* meets naming guidelines
* spec file name maches %{name}.spec
* meets Packaging guidelines with one exception
* open source license
* license matches actual license
* text of license included in %doc
* spec in american english
* spec legible
* source matches upstream
* package builds on all arches
* no build deps
* no shared libs
* no Prefix: (not relocatable)
* owns its dirs
* do duplicate %files
* includes %defattr, most %files entries properly set perms.
* clean section removes buildroot
* consistent use of macros
* contains code
* no large docs, no -doc subpackage
* files in %doc are not used in runtime
* no headers
* no libs (static or dynamic)
* is not a -devel package
* no libtool archives
* no GUI app, no .desktop files
* doesnt own files/dirs used by other packages

* upstream includes license text
* builds in mock
* compiles on supported arches
* functions as described
* scriptlets sane
* no -devel
* no pkgconfig files
* no deps outside std system dirs.
Comment 5 Michael E Brown 2007-03-19 12:13:17 EDT
* name matches upstream tarball. No illegal delimiters.
* no multiple packages with same base name
* spec file named correctly
* versioned properly.
* %{dist} tag properly used
* no doc subpackage
* is not an addon package

* license is GPL
* no patents
* no emulators
* no binary firmware
* no pre-built binaries.
* good changelog
* no 'copyright', 'packager' or 'vendor' tags
* summary tag does not end in dot
* no prereq
* Source: tag documents where to find upstream.
* good buildroot tag
* no file deps
* no build deps (it is a shell script)
* american english
* utf8 spec file
* relevant docs included
* no compiled code (compiler flags irrelevant)
* no debuginfo since no compiled code.
* no libraries (static or otherwise)
* no rpath issues
* config files properly (noreplace)
* init script isnt marked %{config}
* no .desktop files (not a GUI app)
* consistent variable usage.
* no %makeinstall macro usage
* no locale files
* scriptlets dont modify dirs they shouldnt
* no conditional dependencies (--with/--without)
* not relocatable
* no content
* owns its dirs
* not a webapp
Comment 6 Michael E Brown 2007-03-19 12:16:45 EDT
Review Summary:

MUST fix:
* hardcoded dir names. doesnt use macros for dir names.

* TIMESTAMPS: consider "install -p", "mv -p", and "cp -p" in %install and
* no tranlated summary/descriptions (acceptable)

Need to fix rpmlint errors.

The dkms_dbversion is being set 0755 due to %attr() on /var/lib/dkms. Probably
should make /var/lib/dkms a %dir, and separately list dkms_dbver.

SUMMARY: approved with the above changes.
Comment 7 Michael E Brown 2007-03-19 12:18:44 EDT
Possibly change "Dell Computer Corporation" in spec description to "Dell, Inc."
Comment 8 Michael E Brown 2007-03-19 12:20:16 EDT
List of macros for directory names:
Comment 9 Matt Domsch 2007-06-22 12:28:55 EDT
no longer blocks FE-NEW.

Note You need to log in before you can comment on or make changes to this bug.