[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#595817: package review



hello tong,

i'm having a look at your libpam-ssh-agent-auth package, as i'd like to
use it too.

here are some things you should have a look at before the package can be
uploaded:

* changelog: dots inside the debian part of the version number are
  reserved for non-maintainer uploads (NMUs); when you're changing the
  package yourself, just increment the debian part. even better: use the
  dch utility, which will usually do the right thing. it will also
  prevent formating errors like missing lines as in the older versions
  by jamie.

  also, the changelog should probably be condensed into a single log
  entry for the first upload (ie. the version should be 0.9.5-1), and it
  should definitely contain a "(Closes: #595817)" text, so that the ITP
  bug gets closed when it hits the archive.

* control: where did you take the build-depends from? i was curious
  about groff-base, had my doubts about mime-support, but
  libgl1-mesa-glx very definitely does not belong here.

* control: if you want to have a paragraph break in the description,
  leave an empty line (as it is common with many plain-text systems).

  empty line in the context of a debian/control field means that the
  line contains a single indented dot (otherwise, it could be mistaken
  for the beginning of a new section):

  > keyring in a forwarded ssh-agent.
  > .
  > This module can be used to provide authentication for anything run locally that

* copyright: as you are now modifying the debian packaging, your name
  should be listed in the "The Debian packaging is copyright" section of
  the copyright file.

  please also check whether the license described there really applies
  to the source code -- i only checked punctually, but the license text
  in pam_ssh_agent_auth.c is different from debian/copyright.

* dirs / libpam-ssh-agent-auth.dirs: you only need one of those; see the
  man page of dh_installdirs. what debian/dirs does can be read from the
  debhelper man page (section "debhelper config files"), but in short,
  debian/dirs is a shorthand for debian/$PACKAGE.dirs if there is only
  one binary package in your source package.

* README: the situation with README is a little confusing -- it's inside
  debian/, and its heading states it is about debian, but its content is
  not specific to debian. actually, this file should be in the upstream
  tarball outside the debian/ dir (which should not be in the upstream
  tarball anyway).

  in my opinion, this is nothing you can directly change well -- just
  ship the README file by adding a line "debian/README" to debian/docs
  (to be picked up by dh_installdocs) for the time being, and talk to
  upstream about dropping his debian directory and shipping this README
  as a generic readme file.

* README.Debian:

  * this should definitely not be executable.
  
  * README.Debian files are usually wrapped to around 80 characters, and
    uses empty lines for paragraph separation. as far as i know, there
    is no definite "non-markup" language used there, just common sense,
    so your heading indications with leading '=' and the '+' around
    commands are not technically wrong, but a more widespread formatting
    would be:


    > The problem
    > ===========
    > 
    > `sudo` is a mechanism for unprivileged user to gain privileged
    > accesses.

    don't bother too much with this, just be aware and have a look at
    other README.Debian files around.

  * for others who want to base their decisions on my review: i have not
    actually read the whole text.

* rules: do you understand what all the lines there are doing? given the
  standards version, chances are that this file has been generated some
  time ago, and procedures have changed.

  a rules file like this has the same effect:

  > #!/usr/bin/make -f
  > # -*- makefile -*-
  > 
  > # Uncomment this to turn on verbose mode.
  > #export DH_VERBOSE=1
  > 
  > %:
  > 	dh $@
  > 
  > override_dh_auto_configure:
  > 	dh_auto_configure -- --libexecdir=/lib/security --with-mantype=man

  (debian/libpam-ssh-agent-auth.install is not needed any more then.)

* please have a look at the warnings lintian throws when running over
  the package. the biggest problems are also listed on the mentors
  page[1], but you can always check the results of a build process using
  `lintian ../libpam-ssh-agent-auth_0.9.5-2.2_amd64.changes -IE
  --color`. you should aim for fixing them (though some of them don't
  need to be fixed right now; for example, spelling-error-in-manpage is
  better forwarded upstream and fixed with the next upstream release).

best regards
chrysn

[1] http://mentors.debian.net/package/libpam-ssh-agent-auth

-- 
To use raw power is to make yourself infinitely vulnerable to greater powers.
  -- Bene Gesserit axiom

Attachment: signature.asc
Description: Digital signature


Reply to: