launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:twisted-job-runner-virtualenv into launchpad:master.

Commit message:
Make TwistedJobRunner enter the virtualenv correctly

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/382819

ampoule's normal child process startup mechanisms don't quite enter the Launchpad virtualenv correctly.  I'd previously tried to deal with this by adding `import _pythonpath` to its bootstrap code, but overlooked the fact that it ran the system Python without the -S option, and so retained the system site-packages directory in `sys.path`.  As of zope.schema 6.0.0, this matters because the child process tries to import zope.interface and finds the system one which is no longer compatible enough with zope.schema in the virtualenv.

Instead, start the child processes in a way that's conceptually simpler although it requires more code here: just run them with `env/bin/python`.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:twisted-job-runner-virtualenv into launchpad:master.

diff --git a/lib/lp/services/job/runner.py b/lib/lp/services/job/runner.py
index cdcf604..fd393dd 100644
--- a/lib/lp/services/job/runner.py
+++ b/lib/lp/services/job/runner.py
@@ -61,6 +61,7 @@ from twisted.python import (
     )
 from zope.security.proxy import removeSecurityProxy
 
+import lp
 from lp.services import scripts
 from lp.services.config import (
     config,
@@ -481,6 +482,37 @@ class JobRunnerProcess(child.AMPChild):
         return {'success': len(runner.completed_jobs), 'oops_id': oops_id}
 
 
+class VirtualEnvProcessStarter(main.ProcessStarter):
+    """A `ProcessStarter` that sets up Launchpad's virtualenv correctly.
+
+    `ampoule.main` doesn't make it very easy to use `env/bin/python` rather
+    than the bare `sys.executable`; we have to clone-and-hack the innards of
+    `ProcessStarter.startPythonProcess`.  (The alternative would be to use
+    `sys.executable` with the `-S` option and then `import _pythonpath`, but
+    `ampoule.main` also makes it hard to insert `-S` at the right place in
+    the command line.)
+
+    On the other hand, the cloned-and-hacked version can be much simpler,
+    since we don't need to worry about PYTHONPATH; entering the virtualenv
+    correctly will deal with everything that we care about.
+    """
+
+    def _getExecutable(self):
+        top = os.path.dirname(os.path.dirname(os.path.dirname(lp.__file__)))
+        return os.path.join(top, 'env', 'bin', 'python')
+
+    def startPythonProcess(self, prot, *args):
+        env = self.env.copy()
+        args = (self._getExecutable(), '-c', self.bootstrap) + self.args + args
+        # The childFDs variable is needed because sometimes child processes
+        # misbehave and use stdout to output stuff that should really go to
+        # stderr.
+        reactor.spawnProcess(
+            prot, sys.executable, args, env, self.path, self.uid, self.gid,
+            self.usePTY, childFDs={0: "w", 1: "r", 2: "r", 3: "w", 4: "r"})
+        return prot.amp, prot.finished
+
+
 class QuietAMPConnector(main.AMPConnector):
     """An `AMPConnector` that logs stderr output more quietly."""
 
@@ -502,9 +534,7 @@ class TwistedJobRunner(BaseJobRunner):
         env = {'PATH': os.environ['PATH']}
         if 'LPCONFIG' in os.environ:
             env['LPCONFIG'] = os.environ['LPCONFIG']
-        env['PYTHONPATH'] = os.pathsep.join(sys.path)
-        starter = main.ProcessStarter(
-            bootstrap="import _pythonpath\n" + main.BOOTSTRAP, env=env)
+        starter = VirtualEnvProcessStarter(env=env)
         starter.connectorFactory = QuietAMPConnector
         super(TwistedJobRunner, self).__init__(logger, error_utility)
         self.job_source = job_source