Bug#672185: RFS: gnome-session-shutdown/1.81-1 [ITP] -- Shutdown command for GNOME

[Jakub Wilk <jwilk@debian.org>]

(I don't intend to sponsor this package.)

* Jacopo Lorenzetti <jacopol@cyan.xubiq.com>, 2012-05-09, 02:13:
> http://mentors.debian.net/debian/pool/main/g/gnome-session-shutdown/gnome-session-shutdown_1.81-1.dsc

I'd use "=" instead of ":=" in debian/rules, so that the variable is not 
uselessly evaluated even when it's not used. (And it won't be used most 
of the time.)

The definition of DEBIAN_DIR could be simplified to:

$(dir $(firstword ${MAKEFILE_LIST}))

That said, I wouldn't bother with implementing get-orig-source target 
when pristine upstream tarballs is being used.

What is build-dependency on docbook2x for?

I see some bugs and weirdnesses in upstream code:

Line 46:
> import string

string module is so 90s! ;) Most of functions from that module 
(including the ones you use) are deprecated.

Line 52:
> # If running outside X try connecting to main display
> try:
> 	display = os.environ['DISPLAY']
> except KeyError:
> 	os.environ['DISPLAY'] = ':0.0'

This is weird. I haven't seen any program behaving that way. It's user 
responsibility to set DISPLAY correctly, and I wouldn't want any program 
to second-guess me on that matter.

Line 233:
> 			wall.communicate(message)[0]

This looks a bit odd. Was the expression supposed to be assigned to 
something? If not, the "[0]" part is redundant.

Line 287:
> 			pidf.close

This is no-op. I guess it should be: "pidf.close()".

Line 288:
> 		except:
> 			return False

Catching all exceptions is almost always wrong (unless you re-raise them 
later), for two reasons:
- you don't want to catch things like KeyboardInterrupt;
- you don't want to inadvertently ignore an exception that was raised 
because of a bug in your program.

Please catch only exceptions you actually expect to be raised during 
normal program execution.

Line 304:
> 			if e.errno != errno.EEXIST:

You didn't import the errno module.

Lines 313-131: similar problems like in 287-288.

Line 349:
> 	gettext.install('gnome-session-shutdown', LOCALE_DIR, unicode=1)

AFAIK if you can omit the LOCALE_DIR part, Python will do the right 
thing. No need to hard-code path to locale directory.

Line 360:
> 		for fname in os.listdir('/proc'):
> 			if fname.isdigit():
> 				if int(fname) >= 1000:

This looks very dubious to me. Why ">= 1000"?

Running pygettext on the source file results in:

$ pygettext gnome-session-shutdown
*** gnome-session-shutdown:131: Seen unexpected token "+"
*** gnome-session-shutdown:375: Seen unexpected token "+"
*** gnome-session-shutdown:397: Seen unexpected token "+"
*** gnome-session-shutdown:429: Seen unexpected token "+"

--
Jakub Wilk