main inclusion: screen-profiles

[Dustin Kirkland ]

Binary package hint: screen-profiles

https://wiki.ubuntu.com/MainInclusionReportScreenProfiles

:-Dustin

[Loïc Minier (lool)]

Packaging:
- you bdep on gettext and po4a, but these are only used in prebuild which isn't called on the buildds
- 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)
- 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).
- clean:
        rm -rf debian/${PACKAGE} debian/files debian/${PACKAGE}.debhelper.log
I think you want dh_clean?!?
- would be nice to pass -i to dh_* calls in binary-indep
- get-orig-source is .PHONY
- you don't need dh_installdirs / debian/dirs
- you miss a dh_md5sums call
- you should depend on ${misc:Depends} with debhelper >= 5

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
W: screen-profiles: binary-without-manpage usr/bin/screen-profiles-helper
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.

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?)

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

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.

[Loïc Minier (lool)]

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. :-)

[Dustin Kirkland  (kirkland)]

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...

[Loïc Minier (lool)]

> > - 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.

By default, dh_* commands act on all packages of debian/control; currently you only have Arch: all packages, so all packages equals all arch indep packages. IOW you're calling a command acting on arch indep and arch specific packages in an arch indep target (binary-indep). But it's just a nice to have to fix this because you would certainly notice if you were to add some arch specific packages (hopefully ;-).

> 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.

You can only check the owner if the file exists; when the file didn't exist, you would create it: this created a window where another user / process could create the symlink.

There's a regression in your new upload:
W: screen-profiles source: package-lacks-versioned-build-depends-on-debhelper 6
(you're using compat 6).

One thing which would be nice to do is to use the Description to explain why update-notifier-common and bc are useful dependencies to install (since these are optional).

Albeit probably useless by most metrics, you could add an "Enhances: screen".

You don't need to pass debian/screen-profiles debian/files debian/screen-profiles.debhelper.log to dh_clean, it cleans these by default.

You should use dpkg-parsechangelog to parse debian/changelog instead of seds.
DEBVERSION := $(shell dpkg-parsechangelog | sed -n -e 's/^Version: //p')
VERSION := $(shell echo $(DEBVERSION) | sed -e 's/-[^-]*$$//')
careful: doesn't strip epochs.

[Loïc Minier (lool)]

Approved; I know you'll fix the debhelper bdep quickly, and this is not too grave anyway as we have 6 in >= hardy.

[Martin Pitt (pitti)]

Promoted.

[Dustin Kirkland  (kirkland)]

On Sat, Jan 17, 2009 at 5:17 AM, Loïc Minier <email address hidden> wrote:
> By default, dh_* commands act on all packages of debian/control;
> currently you only have Arch: all packages, so all packages equals all
> arch indep packages. IOW you're calling a command acting on arch indep
> and arch specific packages in an arch indep target (binary-indep). But
> it's just a nice to have to fix this because you would certainly notice
> if you were to add some arch specific packages (hopefully ;-).

Okay, fix committed.

> There's a regression in your new upload:
> W: screen-profiles source: package-lacks-versioned-build-depends-on-debhelper 6
> (you're using compat 6).

Fix committed.

> One thing which would be nice to do is to use the Description to explain why update-notifier-common and bc are useful dependencies to install (since these are optional).
>
> Albeit probably useless by most metrics, you could add an "Enhances:
> screen".

Fix committed.

> You don't need to pass debian/screen-profiles debian/files debian
> /screen-profiles.debhelper.log to dh_clean, it cleans these by default.

Fix committed.

> You should use dpkg-parsechangelog to parse debian/changelog instead of seds.
> DEBVERSION := $(shell dpkg-parsechangelog | sed -n -e 's/^Version: //p')
> VERSION := $(shell echo $(DEBVERSION) | sed -e 's/-[^-]*$$//')
> careful: doesn't strip epochs.

Hmm, looks like I could get it to take care of part of the sed, but in
the end, I still end up with sed's in line. I'll keep looking at
this. But it looks to be relatively low priority.

:-Dustin