[Bug 1025720] Re: Package cedarview-drm

[Steve Langasek]

Hi Alberto,

Reviewing the package in the new queue, I spotted a few issues.  The
first few of these need to be fixed before I will accept the package
into precise.  Can you please take care of these and reupload?

Depends: [...]  findutils (>= 4),

findutils is an Essential package, and version 4 is older than hardy.
This dependency should be dropped.

debian/grub.d/11_custom_cmdline: this is NIHing the grub-gfxpayload
blacklist handling already implemented in grub via /usr/share/grub-
gfxpayload-lists/blacklist.  Instead of shipping a file under
/etc/grub.d which hard-codes disabling of vt handoff even when the
hardware is not present, please add a blacklist file to this directory
like the ones in the grub-gfxpayload-lists package and call 'update-
grub-gfxpayload' (instead of update-grub') from your package's postinst
and postrm (on remove/purge only, and guarded with || true).  Creating
the blacklist should be straightforward, you're already processing a
list of PCI IDs in debian/rules.

You should add a dependency on grub-gfxpayload-lists for this.

debian/lightdm.conf: why is this necessary?  including this file in the
package means that merely installing the driver package causes unity3d
to stop working out of the box, even if this video hardware is not
present.  (The same reason not to hard-disable vt.handoff.)  Does the
Unity auto-detection of 3D support not work correctly on this hardware
for some reason?  If it's *really* confirmed to be needed, then ok; but
we should definitely use the autodetection if possible.

lintian reports the following issues with the maintainer scripts:

W: cedarview-drm: possible-bashism-in-maintainer-script postinst:94 '&>'
W: cedarview-drm: command-with-path-in-maintainer-script postrm:39 /usr/sbin/update-initramfs

Looking more closely, I see that this section of the postinst is
entirely redundant with what dh_dkms is supposed to automatically add.
But dh_dkms is currently a complete no-op, because it requires the conf
file to be present as debian/dkms, not debian/dkms.conf.  So at a
minimum, the bashisms in the postinst need to be fixed - but it would be
better to remove this section of the postinst, rename
debian/dkms.conf.in to debian/dkms.conf, and leverage dh_dkms that
you're already build-depending on and calling.  This also removes the
need for an auto-generated debian/prerm.


Below are some further suggestions on improving the readability / maintainability / correctness of the package.  I don't consider these blockers for accepting the package into precise, but I would appreciate seeing them fixed in a subsequent upload to quantal.

%:
        dh --with dkms $@

Note the ordering of options to dh: in recent versions, the target $@
must come as the first argument.

%:
        dh $@ --with dkms

override_dh_auto_build:
clean: regen-from-templates
        dh_quilt_unpatch
        rm -f $(CURDIR)/debian/dkms.conf

        dh clean

I think this should be written:

override_dh_auto_build:
override_dh_auto_clean: regen-from-templates

- This is already a 3.0 (quilt) format source package, so any quilt handling here is redundant. (the build-dependency should also be dropped.)
- You don't need to rm -f debian/dkms.conf from debian rules, you can just list this file name in debian/clean.

- Much of the override_dh_auto_install target could be moved into a
debian/install file; no real reason to have the files installed twice
(once by dh_auto_install and again by dh_install).

- debian/dirs is probably entirely pointless.  You should only need this
file if you are shipping empty directories in the package.

- debian/copyright references http://dep.debian.net/deps/dep5/, which is
an obsolete draft.  This should ideally be updated to conform to
copyright-format 1.0 (http://www.debian.org/doc/packaging-manuals
/copyright-format/1.0/).

I'm attaching a diff that attempts to address most of these issues.  Can
you look at whether this looks sane to you, and if so, can you please
incorporate the changes into your package and reupload?

Rejecting the package for now due to the first set of issues.  These
would be acceptable to fix post-upload if this were being uploaded to
the devel series first, but since this is being pushed to precise and
each subsequent upload requires a separate SRU, these need to be
addressed the first time through.

** Patch added: "cedarview-drm-fixes.diff"
   https://bugs.launchpad.net/ubuntu/+bug/1025720/+attachment/3239085/+files/cedarview-drm-fixes.diff

-- 
You received this bug notification because you are a member of Ubuntu
Package Archive Administrators, which is subscribed to the bug report.
https://bugs.launchpad.net/bugs/1025720

Title:
  Package cedarview-drm

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+bug/1025720/+subscriptions