See the 1.9-0ubuntu1 release, where the following are addressed:

> Packaging:
> - you bdep on gettext and po4a, but these are only used in prebuild
>   which isn't called on the buildds

Fixed.

> - get-orig-source has:
>        [ -d ../../${PACKAGE} ] && mv ../../${PACKAGE} ../../${PACKAGE}-${VER} 
> || true
> you can use a leading "-" to disable error checking:
>        -[ -d ../../${PACKAGE} ] && mv ../../${PACKAGE} ../../${PACKAGE}-${VER}
> or even better, honor errors:
>        [ ! -d ../../${PACKAGE} ] || mv ../../${PACKAGE} 
> ../../${PACKAGE}-${VER}
> (or use if then fi)

Fixed.  I'm going with the leading "-".

> - There's a bunch of ../../ in the get-orig-source rule; I understand
>   it needs to be called from the debian/ dir; perhaps you should enforce
>   this just like dh_testdir does (or require that the rule be run as
>   "debian/rules get-orig-source" and use dh_testdir itself).

Fixed.  Will now be caued using './debian/rules get-orig-source'

> - clean:
>        rm -rf debian/${PACKAGE} debian/files debian/${PACKAGE}.debhelper.log
> I think you want dh_clean?!?

Fixed.  Thanks.

> - would be nice to pass -i to dh_* calls in binary-indep

Hmm, why would that be nice?  Doesn't look like it's necessary.

> - get-orig-source is .PHONY

Fixed.

> - you don't need dh_installdirs / debian/dirs

Fixed.

> - you miss a dh_md5sums call

Fixed

> - you should depend on ${misc:Depends} with debhelper >= 5

Fixed.

> This is what lintian has to say:
> I: screen-profiles source: debian-watch-file-is-missing
> W: screen-profiles source: debhelper-but-no-misc-depends screen-profiles
> I: screen-profiles source: build-depends-without-arch-dep debhelper
> I: screen-profiles source: build-depends-without-arch-dep gettext
> I: screen-profiles source: build-depends-without-arch-dep po4a
> W: screen-profiles: binary-without-manpage usr/bin/screen-launcher

Created manpage.

> W: screen-profiles: binary-without-manpage usr/bin/screen-profiles-
helper

Created manpage.

> I: screen-profiles: no-md5sums-control-file

> These are covered in the above remarks more or less.

> Please mention the copyrights of Canonical and Nick Barcet.

Fixed.

> Typo in man page:
>        select-screen-profile is an application lists the available screen pro‐
>        files on a system and prompts the user to select one.
> ("which lists" perhaps?)

Fixed.

> Would be nice to use "set -e" in non-trivial shell scripts.

Fixed.

> mycache=/var/tmp/updates-available-$USER is a very bad idea: /var/tmp
> is +t and anybody can write there; I can create a
> /var/tmp/updates-available-root symlink to /etc/shadow as my user and
> wait for updates-available to be run as root.
> You need to find another place, or another mechanism. For instance you
> could source all files /var/tmp/updates-available-$USER-XXXXXXXX which
> are owned by $USER and take the most recent one, delete the others.

Fixed.  I tried to handle this by ensuring that the user owned the file,
with the -O test.  I thought that would make it safe?  Well, in any case,
I've moved this to $HOME/.screenrc-updates-available.  This is only really
ever used in <jaunty code.  It's here so that I don't have to compile
separate binaries for hardy/intrepid/jaunty.


> So I'd insist on fixing the missing MD5 and the tmpdir race in /var/tmp at
> least, but the more you fix the merrier. :-)

Thanks for the careful review.  I think I've fixed just about
everything.

:-Dustin

-- 
main inclusion: screen-profiles
https://bugs.launchpad.net/bugs/317214
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Reply via email to