Merge lp:~stgraber/upstart/upstart-initctl2dot-python3 into lp:upstart

[Stéphane Graber]

This branch updates initctl2dot to work with both python2 and python3.
The tested versions were python2.7 and python3.3.

The main changes are:
 - Fixing a bunch of errors spotted by pyflakes.
 - Added universal_newlines=True to the subproces call
 - Stop using string.split, instead split use <str>.split
 - Re-indent the code, split lines, ... to please pep8

I also fixed a small bug where if a job name contained a dot, the .dot file
would be invalid. I'm simply replacing dots by underscores which appears to do
the trick.

The code should now be PEP-8 clean and the output appears to work as expected
when fed to graphviz.

[Steve Langasek (vorlon)]

On Wed, Nov 28, 2012 at 05:10:25PM -0000, Stéphane Graber wrote:
> === modified file 'scripts/initctl2dot.py'
> --- scripts/initctl2dot.py 2011-06-06 12:52:08 +0000
> +++ scripts/initctl2dot.py 2012-11-28 17:09:22 +0000
> @@ -50,522 +50,530 @@
> import re
> import fnmatch
> import os
> -from string import split
> import datetime
> from subprocess import (Popen, PIPE)
> from optparse import OptionParser

Shouldn't this also change the interpreter to /usr/bin/python3? I don't see
any reason why we would need this script to be bilingual, we ought to just
change it and go. If people want to use newer versions of upstart on OSes
that don't have python3, then they just won't be able to use this script...
but this is hardly a requirement anyway.

(Also, looking at this I notice we're shipping initctl2dot in /bin in the
Ubuntu package... that doesn't make much sense when the interpreter is in
/usr/bin.)

[lp:~stgraber/upstart/upstart-initctl2dot-python3]

1396. By Stéphane Graber on 2012-11-28

Switch interpreter to python3 (code still works under python2)

[Stéphane Graber (stgraber)]

> On Wed, Nov 28, 2012 at 05:10:25PM -0000, Stéphane Graber wrote:
> > === modified file 'scripts/initctl2dot.py'
> > --- scripts/initctl2dot.py 2011-06-06 12:52:08 +0000
> > +++ scripts/initctl2dot.py 2012-11-28 17:09:22 +0000
> > @@ -50,522 +50,530 @@
> > import re
> > import fnmatch
> > import os
> > -from string import split
> > import datetime
> > from subprocess import (Popen, PIPE)
> > from optparse import OptionParser
>
> Shouldn't this also change the interpreter to /usr/bin/python3? I don't see
> any reason why we would need this script to be bilingual, we ought to just
> change it and go. If people want to use newer versions of upstart on OSes
> that don't have python3, then they just won't be able to use this script...
> but this is hardly a requirement anyway.

Oops, even though I named the branch -python3 I completely forgot to change the default interpreter to python3 :)
Done now.

> (Also, looking at this I notice we're shipping initctl2dot in /bin in the
> Ubuntu package... that doesn't make much sense when the interpreter is in
> /usr/bin.)

Good point, we should change the packaging to put the script in /usr/bin.

[Barry Warsaw (barry)]

Just a couple more suggestions, since why not.

* No parens are needed in `from subprocess import Popen, PIPE`
* How about switching to argparse instead of optparse?
* In header(), the global statement isn't necessary because you're not assigning to options
* I'd probably rewrite the concats in header() into a triple-quoted multiline string, with {options.foo} replacements
* Remove the global statement from footer()
* There should be some better way to get rid of the multiple concats in footer() too
* sanitize() seems pretty inefficient. maybe that doesn't matter for this script, but it might be better written with a re.sub() where `repl` is a function that knows the mappings
* Why does mk_node_name() even exist? ;)
* show_events(): remove global and rewrite the concats
* Remove the globals from show_events()
* Remove the global in show_jobs()
* Remove the globals in show_jobs()
* In show_jobs(), the `if not restrictions_list: return` is kind of unnecessary.
* None of the show_*() methods need their globals either (just keep at it for the rest of this file ;) - you only need globals if you're *assigning* to a global variable (not if just referencing it, or calling methods like .append() on it)
* read_data(): `for line in ifh:` is probably good enough
* Might want to use a context manager to handle ifh in read_data(). For Python 3.3, see http://docs.python.org/3/library/contextlib.html#contextlib.ExitStack
* line 430: extra parens

Probably other stuff to be cleaned up, but those are the major things that jump out at me. I can take a crack at it if you want.

review: Needs Fixing

[lp:~stgraber/upstart/upstart-initctl2dot-python3]

1397. By Stéphane Graber on 2012-11-29

Cleanup subprocess import and use argparse instead of optparse

1398. By Stéphane Graber on 2012-11-29

Drop all unneeded global statements and do some extra code simplification.

1399. By Stéphane Graber on 2012-11-29

Remove some more globals and unused variable

1400. By Stéphane Graber on 2012-11-29

Simplify header and footer functions

1401. By Stéphane Graber on 2012-11-29

Fix indent

1402. By Stéphane Graber on 2012-11-29

Move the code replacing dots by underscores to the sanitise function

[Stéphane Graber (stgraber)]

> Just a couple more suggestions, since why not.
>
> * No parens are needed in `from subprocess import Popen, PIPE`

Done

> * How about switching to argparse instead of optparse?

Done

> * In header(), the global statement isn't necessary because you're not
> assigning to options

Done

> * I'd probably rewrite the concats in header() into a triple-quoted multiline
> string, with {options.foo} replacements

Done

> * Remove the global statement from footer()

Done

> * There should be some better way to get rid of the multiple concats in
> footer() too

Done. Pre-processed the ifs, then used a giant string with format, similar to header()

> * sanitize() seems pretty inefficient. maybe that doesn't matter for this
> script, but it might be better written with a re.sub() where `repl` is a
> function that knows the mappings

Kept it as-is for now as I'm not a fan of using regexps when not absolutely necessary :)

> * Why does mk_node_name() even exist? ;)

No good reason apparently, dropped.

> * show_events(): remove global and rewrite the concats

Done.

> * Remove the globals from show_events()

Done

> * Remove the global in show_jobs()

Done

> * Remove the globals in show_jobs()

Done

> * In show_jobs(), the `if not restrictions_list: return` is kind of
> unnecessary.

Right, at first read, I assumed that restrictions_list could be None, but rechecking the code, that's not actually the case, so iterating will always work and that if statement is unnecessary. Dropped.

> * None of the show_*() methods need their globals either (just keep at it for
> the rest of this file ;) - you only need globals if you're *assigning* to a
> global variable (not if just referencing it, or calling methods like .append()
> on it)

Done

> * read_data(): `for line in ifh:` is probably good enough

Agreed, done.

> * Might want to use a context manager to handle ifh in read_data(). For Python
> 3.3, see http://docs.python.org/3/library/contextlib.html#contextlib.ExitStack

I'd rather not depend on python 3.3. Some people may still want to use the fixed script on something slightly older, but that's good to know nevertheless.

> * line 430: extra parens

Fixed that one and another one a few lines further down.

> Probably other stuff to be cleaned up, but those are the major things that
> jump out at me. I can take a crack at it if you want.

As far as I can tell, the script still works with those changes and the output is similar.

[Barry Warsaw (barry)]

Much improved, thanks! Just a few more to go:

* Remove global jobs, events from read_data()
* Remove global jobs, cmd, default_color_* from main(), but keep global
  options and restrictions_list.

I'll go ahead and approve the merge on the above conditions.

[Barry Warsaw (barry)]

review: Approve

[lp:~stgraber/upstart/upstart-initctl2dot-python3]

1403. By Stéphane Graber on 2012-11-29

Remove last useless globals, fix .dot file syntax in footer, fix indentation of output file.

[Stéphane Graber (stgraber)]

Thanks for the comments. I removed those last few globals and fixed a mistake I did in the output file so that it's now valid. While I was at it, I also tweaked the code a bit so that the output file's indentation is correct.

[James Hunt (jamesodhunt)]

Thanks Stéphane. I found a new bug in testing (bug 1084985), but that is unrelated to the changes you've made, so merged.

review: Approve

=== modified file 'scripts/initctl2dot.py'
--- scripts/initctl2dot.py	2011-06-06 12:52:08 +0000
+++ scripts/initctl2dot.py	2012-11-29 16:35:44 +0000
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/python3
 # -*- coding: utf-8 -*-
 #---------------------------------------------------------------------
 #
@@ -50,522 +50,472 @@
 import re
 import fnmatch
 import os
-from string import split
 import datetime
-from subprocess import (Popen, PIPE)
-from optparse import OptionParser
+from subprocess import Popen, PIPE
+from argparse import ArgumentParser
 
-jobs   = {}
+options = None
+jobs = {}
 events = {}
 cmd = "initctl --system show-config -e"
-script_name =  os.path.basename(sys.argv[0])
-
-job_events = [ 'starting', 'started', 'stopping', 'stopped' ]
+script_name = os.path.basename(sys.argv[0])
 
 # list of jobs to restict output to
 restrictions_list = []
 
-default_color_emits    = 'green'
+default_color_emits = 'green'
 default_color_start_on = 'blue'
-default_color_stop_on  = 'red'
-default_color_event    = 'thistle'
-default_color_job      = '#DCDCDC' # "Gainsboro"
-default_color_text     = 'black'
-default_color_bg       = 'white'
+default_color_stop_on = 'red'
+default_color_event = 'thistle'
+default_color_job = '#DCDCDC'  # "Gainsboro"
+default_color_text = 'black'
+default_color_bg = 'white'
 
-default_outfile        = 'upstart.dot'
+default_outfile = 'upstart.dot'
 
 
 def header(ofh):
-  global options
-
-  str  = "digraph upstart {\n"
-
-  # make the default node an event to simplify glob code
-  str += "  node [shape=\"diamond\", fontcolor=\"%s\", fillcolor=\"%s\", style=\"filled\"];\n" \
-    % (options.color_event_text, options.color_event)
-  str += "  rankdir=LR;\n"
-  str += "  overlap=false;\n"
-  str += "  bgcolor=\"%s\";\n" % options.color_bg
-  str += "  fontcolor=\"%s\";\n" % options.color_text
-
-  ofh.write(str)
+    ofh.write("""digraph upstart {{
+  node [shape=\"diamond\", fontcolor=\"{options.color_event_text}\", """
+              """fillcolor=\"{options.color_event}\", style=\"filled\"];
+  rankdir=LR;
+  overlap=false;
+  bgcolor=\"{options.color_bg}\";
+  fontcolor=\"{options.color_text}\";
+""".format(options=options))
 
 
 def footer(ofh):
-  global options
-
-  epilog = "overlap=false;\n"
-  epilog += "label=\"Generated on %s by %s\\n" % \
-    (str(datetime.datetime.now()), script_name)
-
-  if options.restrictions:
-    epilog += "(subset, "
-  else:
-    epilog += "("
-
-  if options.infile:
-    epilog += "from file data).\\n"
-  else:
-    epilog += "from '%s' on host %s).\\n" % \
-      (cmd, os.uname()[1])
-
-  epilog += "Boxes of color %s denote jobs.\\n" % options.color_job
-  epilog += "Solid diamonds of color %s denote events.\\n" % options.color_event
-  epilog += "Dotted diamonds denote 'glob' events.\\n"
-  epilog += "Emits denoted by %s lines.\\n" % options.color_emits
-  epilog += "Start on denoted by %s lines.\\n" % options.color_start_on
-  epilog += "Stop on denoted by %s lines.\\n" % options.color_stop_on
-  epilog += "\";\n"
-  epilog += "}\n"
-  ofh.write(epilog)
+    if options.restrictions:
+        details = "(subset, "
+    else:
+        details = "("
+
+    if options.infile:
+        details += "from file data)."
+    else:
+        details += "from '%s' on host %s)." % (cmd, os.uname()[1])
+
+    ofh.write("  overlap=false;\n"
+              "  label=\"Generated on {datenow} by {script_name} {details}\\n"
+              "Boxes of color {options.color_job} denote jobs.\\n"
+              "Solid diamonds of color {options.color_event} denote events.\\n"
+              "Dotted diamons denote 'glob' events.\\n"
+              "Emits denoted by {options.color_emits} lines.\\n"
+              "Start on denoted by {options.color_start_on} lines.\\n"
+              "Stop on denoted by {options.color_stop_on} lines.\\n"
+              "\";\n"
+              "}}\n".format(options=options, datenow=datetime.datetime.now(),
+                            script_name=script_name, details=details))
 
 
 # Map dash to underscore since graphviz node names cannot
 # contain dashes. Also remove dollars and colons
 def sanitise(s):
-  return s.replace('-', '_').replace('$', 'dollar_').replace('[', \
-  'lbracket').replace(']', 'rbracket').replace('!', \
-  'bang').replace(':', '_').replace('*', 'star').replace('?', 'question')
+    return s.replace('-', '_').replace('$', 'dollar_') \
+            .replace('[', 'lbracket').replace(']', 'rbracket') \
+            .replace('!', 'bang').replace(':', 'colon').replace('*', 'star') \
+            .replace('?', 'question').replace('.', '_')
 
 
 # Convert a dollar in @name to a unique-ish new name, based on @job and
 # return it. Used for very rudimentary instance handling.
 def encode_dollar(job, name):
-  if name[0] == '$':
-    name = job + ':' + name
-  return name
-
-
-def mk_node_name(name):
-  return sanitise(name)
+    if name[0] == '$':
+        name = job + ':' + name
+    return name
 
 
 # Jobs and events can have identical names, so prefix them to namespace
 # them off.
 def mk_job_node_name(name):
-  return mk_node_name('job_' + name)
+    return sanitise('job_' + name)
 
 
 def mk_event_node_name(name):
-  return mk_node_name('event_' + name)
+    return sanitise('event_' + name)
 
 
 def show_event(ofh, name):
-    global options
-    str = "%s [label=\"%s\", shape=diamond, fontcolor=\"%s\", fillcolor=\"%s\"," % \
-      (mk_event_node_name(name), name, options.color_event_text, options.color_event)
+    str = "  %s [label=\"%s\", shape=diamond, fontcolor=\"%s\", " \
+          "fillcolor=\"%s\"," % (mk_event_node_name(name), name,
+                                 options.color_event_text, options.color_event)
 
     if '*' in name:
-      str += " style=\"dotted\""
+        str += " style=\"dotted\""
     else:
-      str += " style=\"filled\""
+        str += " style=\"filled\""
 
     str += "];\n"
 
     ofh.write(str)
 
+
 def show_events(ofh):
-  global events
-  global options
-  global restrictions_list
-
-  events_to_show = []
-
-  if restrictions_list:
-    for job in restrictions_list:
-
-      # We want all events emitted by the jobs in the restrictions_list.
-      events_to_show += jobs[job]['emits']
-
-      # We also want all events that jobs in restrictions_list start/stop
-      # on.
-      events_to_show += jobs[job]['start on']['event']
-      events_to_show += jobs[job]['stop on']['event']
-
-      # We also want all events emitted by all jobs that jobs in the
-      # restrictions_list start/stop on. Finally, we want all events
-      # emmitted by those jobs in the restrictions_list that we
-      # start/stop on.
-      for j in jobs[job]['start on']['job']:
-        if jobs.has_key(j) and jobs[j].has_key('emits'):
-          events_to_show += jobs[j]['emits']
-
-      for j in jobs[job]['stop on']['job']:
-        if jobs.has_key(j) and jobs[j].has_key('emits'):
-          events_to_show += jobs[j]['emits']
-  else:
-    events_to_show = events
-
-  for e in events_to_show:
-    show_event(ofh, e)
+    events_to_show = []
+
+    if restrictions_list:
+        for job in restrictions_list:
+
+            # We want all events emitted by the jobs in the restrictions_list.
+            events_to_show += jobs[job]['emits']
+
+            # We also want all events that jobs in restrictions_list start/stop
+            # on.
+            events_to_show += jobs[job]['start on']['event']
+            events_to_show += jobs[job]['stop on']['event']
+
+            # We also want all events emitted by all jobs that jobs in the
+            # restrictions_list start/stop on. Finally, we want all events
+            # emmitted by those jobs in the restrictions_list that we
+            # start/stop on.
+            for j in jobs[job]['start on']['job']:
+                if j in jobs and 'emits' in jobs[j]:
+                    events_to_show += jobs[j]['emits']
+
+            for j in jobs[job]['stop on']['job']:
+                if j in jobs and 'emits' in jobs[j]:
+                    events_to_show += jobs[j]['emits']
+    else:
+        events_to_show = events
+
+    for e in events_to_show:
+        show_event(ofh, e)
 
 
 def show_job(ofh, name):
-  global options
-
-  ofh.write("""
-    %s [shape=\"record\", label=\"<job> %s | { <start> start on | <stop> stop on }\", fontcolor=\"%s\", style=\"filled\", fillcolor=\"%s\"];
-    """ % (mk_job_node_name(name), name, options.color_job_text, options.color_job))
+    ofh.write("  %s [shape=\"record\", label=\"<job> %s | { <start> start on |"
+              " <stop> stop on }\", fontcolor=\"%s\", style=\"filled\", "
+              " fillcolor=\"%s\"];\n" % (mk_job_node_name(name), name,
+                                         options.color_job_text,
+                                         options.color_job))
 
 
 def show_jobs(ofh):
-  global jobs
-  global options
-  global restrictions_list
-
-  if restrictions_list:
-    jobs_to_show = restrictions_list
-  else:
-    jobs_to_show = jobs
-
-  for j in jobs_to_show:
-    show_job(ofh, j)
-    # add those jobs which are referenced by existing jobs, but which
-    # might not be available as .conf files. For example, plymouth.conf
-    # references gdm *or* kdm, but you are unlikely to have both
-    # installed.
-    for s in jobs[j]['start on']['job']:
-      if s not in jobs_to_show:
-        show_job(ofh, s)
-
-    for s in jobs[j]['stop on']['job']:
-      if s not in jobs_to_show:
-        show_job(ofh, s)
-
-  if not restrictions_list:
-    return
-
-  # Having displayed the jobs in restrictions_list,
-  # we now need to display all jobs that *those* jobs
-  # start on/stop on.
-  for j in restrictions_list:
-    for job in jobs[j]['start on']['job']:
-      show_job(ofh, job)
-    for job in jobs[j]['stop on']['job']:
-      show_job(ofh, job)
-
-  # Finally, show all jobs which emit events that jobs in the
-  # restrictions_list care about.
-  for j in restrictions_list:
-
-    for e in jobs[j]['start on']['event']:
-      for k in jobs:
-        if e in jobs[k]['emits']:
-          show_job(ofh, k)
-
-    for e in jobs[j]['stop on']['event']:
-      for k in jobs:
-        if e in jobs[k]['emits']:
-          show_job(ofh, k)
+    if restrictions_list:
+        jobs_to_show = restrictions_list
+    else:
+        jobs_to_show = jobs
+
+    for j in jobs_to_show:
+        show_job(ofh, j)
+        # add those jobs which are referenced by existing jobs, but which
+        # might not be available as .conf files. For example, plymouth.conf
+        # references gdm *or* kdm, but you are unlikely to have both
+        # installed.
+        for s in jobs[j]['start on']['job']:
+            if s not in jobs_to_show:
+                show_job(ofh, s)
+
+        for s in jobs[j]['stop on']['job']:
+            if s not in jobs_to_show:
+                show_job(ofh, s)
+
+    # Having displayed the jobs in restrictions_list,
+    # we now need to display all jobs that *those* jobs
+    # start on/stop on.
+    for j in restrictions_list:
+        for job in jobs[j]['start on']['job']:
+            show_job(ofh, job)
+        for job in jobs[j]['stop on']['job']:
+            show_job(ofh, job)
+
+    # Finally, show all jobs which emit events that jobs in the
+    # restrictions_list care about.
+    for j in restrictions_list:
+        for e in jobs[j]['start on']['event']:
+            for k in jobs:
+                if e in jobs[k]['emits']:
+                    show_job(ofh, k)
+
+        for e in jobs[j]['stop on']['event']:
+            for k in jobs:
+                if e in jobs[k]['emits']:
+                    show_job(ofh, k)
 
 
 def show_edge(ofh, from_node, to_node, color):
-  ofh.write("%s -> %s [color=\"%s\"];\n" % (from_node, to_node, color))
+    ofh.write("  %s -> %s [color=\"%s\"];\n" % (from_node, to_node, color))
 
 
 def show_start_on_job_edge(ofh, from_job, to_job):
-  global options
-  show_edge(ofh, "%s:start" % mk_job_node_name(from_job),
-    "%s:job" % mk_job_node_name(to_job), options.color_start_on)
+    show_edge(ofh, "%s:start" % mk_job_node_name(from_job),
+              "%s:job" % mk_job_node_name(to_job), options.color_start_on)
 
 
 def show_start_on_event_edge(ofh, from_job, to_event):
-  global options
-  show_edge(ofh, "%s:start" % mk_job_node_name(from_job),
-    mk_event_node_name(to_event), options.color_start_on)
+    show_edge(ofh, "%s:start" % mk_job_node_name(from_job),
+              mk_event_node_name(to_event), options.color_start_on)
 
 
 def show_stop_on_job_edge(ofh, from_job, to_job):
-  global options
-  show_edge(ofh, "%s:stop" % mk_job_node_name(from_job),
-    "%s:job" % mk_job_node_name(to_job), options.color_stop_on)
+    show_edge(ofh, "%s:stop" % mk_job_node_name(from_job),
+              "%s:job" % mk_job_node_name(to_job), options.color_stop_on)
 
 
 def show_stop_on_event_edge(ofh, from_job, to_event):
-  global options
-  show_edge(ofh, "%s:stop" % mk_job_node_name(from_job),
-    mk_event_node_name(to_event), options.color_stop_on)
+    show_edge(ofh, "%s:stop" % mk_job_node_name(from_job),
+              mk_event_node_name(to_event), options.color_stop_on)
 
 
 def show_job_emits_edge(ofh, from_job, to_event):
-  global options
-  show_edge(ofh, "%s:job" % mk_job_node_name(from_job),
-    mk_event_node_name(to_event), options.color_emits)
+    show_edge(ofh, "%s:job" % mk_job_node_name(from_job),
+              mk_event_node_name(to_event), options.color_emits)
 
 
 def show_edges(ofh):
-  global events
-  global jobs
-  global options
-  global restrictions_list
-
-  glob_jobs = {}
-
-  if restrictions_list:
-    jobs_list = restrictions_list
-  else:
-    jobs_list = jobs
-
-  for job in jobs_list:
-
-    for s in jobs[job]['start on']['job']:
-      show_start_on_job_edge(ofh, job, s)
-
-    for s in jobs[job]['start on']['event']:
-      show_start_on_event_edge(ofh, job, s)
-
-    for s in jobs[job]['stop on']['job']:
-      show_stop_on_job_edge(ofh, job, s)
-
-    for s in jobs[job]['stop on']['event']:
-      show_stop_on_event_edge(ofh, job, s)
-
-    for e in jobs[job]['emits']:
-      if '*' in e:
-        # handle glob patterns in 'emits'
-        glob_events = []
-        for _e in events:
-          if e != _e and fnmatch.fnmatch(_e, e):
-            glob_events.append(_e)
-        glob_jobs[job] = glob_events
-
-      show_job_emits_edge(ofh, job, e)
+    glob_jobs = {}
+
+    if restrictions_list:
+        jobs_list = restrictions_list
+    else:
+        jobs_list = jobs
+
+    for job in jobs_list:
+        for s in jobs[job]['start on']['job']:
+            show_start_on_job_edge(ofh, job, s)
+
+        for s in jobs[job]['start on']['event']:
+            show_start_on_event_edge(ofh, job, s)
+
+        for s in jobs[job]['stop on']['job']:
+            show_stop_on_job_edge(ofh, job, s)
+
+        for s in jobs[job]['stop on']['event']:
+            show_stop_on_event_edge(ofh, job, s)
+
+        for e in jobs[job]['emits']:
+            if '*' in e:
+                # handle glob patterns in 'emits'
+                glob_events = []
+                for _e in events:
+                    if e != _e and fnmatch.fnmatch(_e, e):
+                        glob_events.append(_e)
+                glob_jobs[job] = glob_events
+
+            show_job_emits_edge(ofh, job, e)
+
+        if not restrictions_list:
+            continue
+
+        # Add links to events emitted by all jobs which current job
+        # start/stops on
+        for j in jobs[job]['start on']['job']:
+            if j not in jobs:
+                continue
+            for e in jobs[j]['emits']:
+                show_job_emits_edge(ofh, j, e)
+
+        for j in jobs[job]['stop on']['job']:
+            for e in jobs[j]['emits']:
+                show_job_emits_edge(ofh, j, e)
+
+    # Create links from jobs (which advertise they emits a class of
+    # events, via the glob syntax) to all the events they create.
+    for g in glob_jobs:
+        for ge in glob_jobs[g]:
+            show_job_emits_edge(ofh, g, ge)
 
     if not restrictions_list:
-      continue
-
-    # Add links to events emitted by all jobs which current job
-    # start/stops on
-    for j in jobs[job]['start on']['job']:
-      if not jobs.has_key(j):
-        continue
-      for e in jobs[j]['emits']:
-        show_job_emits_edge(ofh, j, e)
-
-    for j in jobs[job]['stop on']['job']:
-      for e in jobs[j]['emits']:
-        show_job_emits_edge(ofh, j, e)
-
-  # Create links from jobs (which advertise they emits a class of
-  # events, via the glob syntax) to all the events they create.
-  for g in glob_jobs:
-    for ge in glob_jobs[g]:
-      show_job_emits_edge(ofh, g, ge)
-
-  if not restrictions_list:
-    return
-
-  # Add jobs->event links to jobs which emit events that current job
-  # start/stops on.
-  for j in restrictions_list:
-
-    for e in jobs[j]['start on']['event']:
-      for k in jobs:
-        if e in jobs[k]['emits'] and e not in restrictions_list:
-          show_job_emits_edge(ofh, k, e)
-
-    for e in jobs[j]['stop on']['event']:
-      for k in jobs:
-        if e in jobs[k]['emits'] and e not in restrictions_list:
-          show_job_emits_edge(ofh, k, e)
+        return
+
+    # Add jobs->event links to jobs which emit events that current job
+    # start/stops on.
+    for j in restrictions_list:
+        for e in jobs[j]['start on']['event']:
+            for k in jobs:
+                if e in jobs[k]['emits'] and e not in restrictions_list:
+                    show_job_emits_edge(ofh, k, e)
+
+        for e in jobs[j]['stop on']['event']:
+            for k in jobs:
+                if e in jobs[k]['emits'] and e not in restrictions_list:
+                    show_job_emits_edge(ofh, k, e)
 
 
 def read_data():
-  global jobs
-  global events
-  global options
-  global cmd
-  global job_events
-
-  if options.infile:
-    try:
-      ifh = open(options.infile, 'r')
-    except:
-      sys.exit("ERROR: cannot read file '%s'" % options.infile)
-  else:
-    try:
-      ifh = Popen(split(cmd), stdout=PIPE).stdout
-    except:
-      sys.exit("ERROR: cannot run '%s'" % cmd)
-
-  for line in ifh.readlines():
-      record = {}
-      line = line.rstrip()
-
-      result = re.match('^\s+start on ([^,]+) \(job:\s*([^,]*), env:', line)
-      if result:
-        _event = encode_dollar(job, result.group(1))
-        _job   = result.group(2)
-        if _job:
-          jobs[job]['start on']['job'][_job] = 1
-        else:
-          jobs[job]['start on']['event'][_event] = 1
-          events[_event] = 1
-        continue
-
-      result = re.match('^\s+stop on ([^,]+) \(job:\s*([^,]*), env:', line)
-      if result:
-        _event = encode_dollar(job, result.group(1))
-        _job   = result.group(2)
-        if _job:
-          jobs[job]['stop on']['job'][_job] = 1
-        else:
-          jobs[job]['stop on']['event'][_event] = 1
-          events[_event] = 1
-        continue
-
-      if re.match('^\s+emits', line):
-        event = (line.lstrip().split())[1]
-        event = encode_dollar(job, event)
-        events[event] = 1
-        jobs[job]['emits'][event] = 1
-      else:
-        tokens = (line.lstrip().split())
-
-        if len(tokens) != 1:
-          sys.exit("ERROR: invalid line: %s" % line.lstrip())
-
-        job_record      = {}
-
-        start_on        = {}
-        start_on_jobs   = {}
-        start_on_events = {}
-
-        stop_on         = {}
-        stop_on_jobs    = {}
-        stop_on_events  = {}
-
-        emits           = {}
-
-        start_on['job']    = start_on_jobs
-        start_on['event']  = start_on_events
-
-        stop_on['job']     = stop_on_jobs
-        stop_on['event']   = stop_on_events
-
-        job_record['start on'] = start_on
-        job_record['stop on']  = stop_on
-        job_record['emits']    = emits
-
-        job = (tokens)[0]
-        jobs[job] = job_record
+    if options.infile:
+        try:
+            ifh = open(options.infile, 'r')
+        except:
+            sys.exit("ERROR: cannot read file '%s'" % options.infile)
+    else:
+        try:
+            ifh = Popen(cmd.split(), stdout=PIPE,
+                        universal_newlines=True).stdout
+        except:
+            sys.exit("ERROR: cannot run '%s'" % cmd)
+
+    job = None
+    for line in ifh:
+        line = line.rstrip()
+
+        result = re.match('^\s+start on ([^,]+) \(job:\s*([^,]*), env:', line)
+        if result:
+            _event = encode_dollar(job, result.group(1))
+            _job = result.group(2)
+            if _job:
+                jobs[job]['start on']['job'][_job] = 1
+            else:
+                jobs[job]['start on']['event'][_event] = 1
+                events[_event] = 1
+            continue
+
+        result = re.match('^\s+stop on ([^,]+) \(job:\s*([^,]*), env:', line)
+        if result:
+            _event = encode_dollar(job, result.group(1))
+            _job = result.group(2)
+            if _job:
+                jobs[job]['stop on']['job'][_job] = 1
+            else:
+                jobs[job]['stop on']['event'][_event] = 1
+                events[_event] = 1
+            continue
+
+        if re.match('^\s+emits', line):
+            event = line.lstrip().split()[1]
+            event = encode_dollar(job, event)
+            events[event] = 1
+            jobs[job]['emits'][event] = 1
+        else:
+            tokens = line.lstrip().split()
+
+            if len(tokens) != 1:
+                sys.exit("ERROR: invalid line: %s" % line.lstrip())
+
+            job_record = {}
+
+            start_on = {}
+            start_on_jobs = {}
+            start_on_events = {}
+
+            stop_on = {}
+            stop_on_jobs = {}
+            stop_on_events = {}
+
+            emits = {}
+
+            start_on['job'] = start_on_jobs
+            start_on['event'] = start_on_events
+
+            stop_on['job'] = stop_on_jobs
+            stop_on['event'] = stop_on_events
+
+            job_record['start on'] = start_on
+            job_record['stop on'] = stop_on
+            job_record['emits'] = emits
+
+            job = (tokens)[0]
+            jobs[job] = job_record
 
 
 def main():
-  global jobs
-  global options
-  global cmd
-  global default_color_emits
-  global default_color_start_on
-  global default_color_stop_on
-  global default_color_event
-  global default_color_job
-  global default_color_text
-  global default_color_bg
-  global restrictions_list
-
-  description = "Convert initctl(8) output to GraphViz dot(1) format."
-  epilog = \
-    "See http://www.graphviz.org/doc/info/colors.html for available colours."
-
-  parser = OptionParser(description=description, epilog=epilog)
-
-  parser.add_option("-r", "--restrict-to-jobs",
-      dest="restrictions",
-      help="Limit display of 'start on' and 'stop on' conditions to " +
-      "specified jobs (comma-separated list).")
-
-  parser.add_option("-f", "--infile",
-      dest="infile",
-      help="File to read '%s' output from. If not specified, " \
-      "initctl will be run automatically." % cmd)
-
-  parser.add_option("-o", "--outfile",
-      dest="outfile",
-      help="File to write output to (default=%s)" % default_outfile)
-
-  parser.add_option("--color-emits",
-      dest="color_emits",
-      help="Specify color for 'emits' lines (default=%s)." %
-      default_color_emits)
-
-  parser.add_option("--color-start-on",
-      dest="color_start_on",
-      help="Specify color for 'start on' lines (default=%s)." %
-      default_color_start_on)
-
-  parser.add_option("--color-stop-on",
-      dest="color_stop_on",
-      help="Specify color for 'stop on' lines (default=%s)." %
-      default_color_stop_on)
-
-  parser.add_option("--color-event",
-      dest="color_event",
-      help="Specify color for event boxes (default=%s)." %
-      default_color_event)
-
-  parser.add_option("--color-text",
-      dest="color_text",
-      help="Specify color for summary text (default=%s)." %
-      default_color_text)
-
-  parser.add_option("--color-bg",
-      dest="color_bg",
-      help="Specify background color for diagram (default=%s)." %
-      default_color_bg)
-
-  parser.add_option("--color-event-text",
-      dest="color_event_text",
-      help="Specify color for text in event boxes (default=%s)." %
-      default_color_text)
-
-  parser.add_option("--color-job-text",
-      dest="color_job_text",
-      help="Specify color for text in job boxes (default=%s)." %
-      default_color_text)
-
-  parser.add_option("--color-job",
-      dest="color_job",
-      help="Specify color for job boxes (default=%s)." %
-      default_color_job)
-
-  parser.set_defaults(color_emits=default_color_emits,
-  color_start_on=default_color_start_on,
-  color_stop_on=default_color_stop_on,
-  color_event=default_color_event,
-  color_job=default_color_job,
-  color_job_text=default_color_text,
-  color_event_text=default_color_text,
-  color_text=default_color_text,
-  color_bg=default_color_bg,
-  outfile=default_outfile)
-
-  (options, args) = parser.parse_args()
-
-  if options.outfile == '-':
-    ofh = sys.stdout
-  else:
-    try:
-      ofh = open(options.outfile, "w")
-    except:
-      sys.exit("ERROR: cannot open file %s for writing" % options.outfile)
-
-  if options.restrictions:
-    restrictions_list = options.restrictions.split(",")
-
-  read_data()
-
-  for job in restrictions_list:
-    if not job in jobs:
-      sys.exit("ERROR: unknown job %s" % job)
-
-  header(ofh)
-  show_events(ofh)
-  show_jobs(ofh)
-  show_edges(ofh)
-  footer(ofh)
+    global options
+    global restrictions_list
+
+    description = "Convert initctl(8) output to GraphViz dot(1) format."
+    epilog = "See http://www.graphviz.org/doc/info/colors.html " \
+             "for available colours."
+
+    parser = ArgumentParser(description=description, epilog=epilog)
+
+    parser.add_argument("-r", "--restrict-to-jobs",
+                        dest="restrictions",
+                        help="Limit display of 'start on' and 'stop on' "
+                        "conditions to specified jobs (comma-separated list).")
+
+    parser.add_argument("-f", "--infile",
+                        dest="infile",
+                        help="File to read '%s' output from. If not specified"
+                        ", initctl will be run automatically." % cmd)
+
+    parser.add_argument("-o", "--outfile",
+                        dest="outfile",
+                        help="File to write output to (default=%s)" %
+                             default_outfile)
+
+    parser.add_argument("--color-emits",
+                        dest="color_emits",
+                        help="Specify color for 'emits' lines (default=%s)." %
+                             default_color_emits)
+
+    parser.add_argument("--color-start-on",
+                        dest="color_start_on",
+                        help="Specify color for 'start on' lines "
+                             "(default=%s)." % default_color_start_on)
+
+    parser.add_argument("--color-stop-on",
+                        dest="color_stop_on",
+                        help="Specify color for 'stop on' lines "
+                             "(default=%s)." % default_color_stop_on)
+
+    parser.add_argument("--color-event",
+                        dest="color_event",
+                        help="Specify color for event boxes (default=%s)." %
+                             default_color_event)
+
+    parser.add_argument("--color-text",
+                        dest="color_text",
+                        help="Specify color for summary text (default=%s)." %
+                             default_color_text)
+
+    parser.add_argument("--color-bg",
+                        dest="color_bg",
+                        help="Specify background color for diagram "
+                             "(default=%s)." % default_color_bg)
+
+    parser.add_argument("--color-event-text",
+                        dest="color_event_text",
+                        help="Specify color for text in event boxes "
+                             "(default=%s)." % default_color_text)
+
+    parser.add_argument("--color-job-text",
+                        dest="color_job_text",
+                        help="Specify color for text in job boxes "
+                             "(default=%s)." % default_color_text)
+
+    parser.add_argument("--color-job",
+                        dest="color_job",
+                        help="Specify color for job boxes (default=%s)." %
+                             default_color_job)
+
+    parser.set_defaults(color_emits=default_color_emits,
+                        color_start_on=default_color_start_on,
+                        color_stop_on=default_color_stop_on,
+                        color_event=default_color_event,
+                        color_job=default_color_job,
+                        color_job_text=default_color_text,
+                        color_event_text=default_color_text,
+                        color_text=default_color_text,
+                        color_bg=default_color_bg,
+                        outfile=default_outfile)
+
+    options = parser.parse_args()
+
+    if options.outfile == '-':
+        ofh = sys.stdout
+    else:
+        try:
+            ofh = open(options.outfile, "w")
+        except:
+            sys.exit("ERROR: cannot open file %s for writing" %
+                     options.outfile)
+
+    if options.restrictions:
+        restrictions_list = options.restrictions.split(",")
+
+    read_data()
+
+    for job in restrictions_list:
+        if not job in jobs:
+            sys.exit("ERROR: unknown job %s" % job)
+
+    header(ofh)
+    show_events(ofh)
+    show_jobs(ofh)
+    show_edges(ofh)
+    footer(ofh)
 
 
 if __name__ == "__main__":
-  main()
+    main()