Merge lp:~maxb/launchpad/bug-497731 into lp:launchpad

[Max Bowsher]

Keep the Python dist-packages directory off the bzr plugin path, thus fixing testsuite failures when incompatible bzr plugins are installed there.

[Max Bowsher (maxb)]

I observed some test failures which turned out to be because of unclean stderr, when a Launchpad bzr subprocess tried to load plugins from my /usr/lib/python2.5/dist-packages/bzrlib/plugins/ directory, when those plugins required bzr 2.0 (my system bzrlib) but Launchpad's bzrlib is an incompatible eggified 2.1.

The root of the problem is that bzr shouldn't be trying to load plugins from /usr/lib/python2.5/dist-packages/bzrlib/plugins/ at all, in a Launchpad scenario. Happily, bzr 2.1 adds extra syntax to BZR_PLUGIN_PATH which allows us to tell it not to: we need to add "-site" as a component of BZR_PLUGIN_PATH.

So, here's a simple patch to do just that. In doing so, we get to clean up a big block of code which *tried* (but failed because its regexps were not quite right) to tolerate these warnings in stderr.

To QA this without running the entire testsuite, you should run bin/test -t lp.codehosting.tests.test_lpserve.

NB: You need to have some .deb-packaged plugins which are not compatible with bzrlib 2.1 installed to manifest the actual test failures.

[Guilherme Salgado (salgado)]

<salgado> no, just remove get_BZR_PLUGIN_PATH_for_subprocess() and change get_bzr_plugins_path() to append a "-site" to its return value
<maxb> That's not acceptable because the bare directory is required for the load_plugins([...]) call ~10 lines below
<salgado> oh, I missed that
<salgado> why is it required there?
<maxb> The way it works is that if you're calling that bzrlib call directly, bzrlib uses *exactly* the directories you pass it. If you're setting the environment variable, bzrlib augments it with the standard directories unless you use the magic tokens to tell it not to
<salgado> I see
<salgado> maxb, then we can rename the existing one to _get_bzr_plugins_path() and remove it from __all__
<maxb> Sounds good
<salgado> maxb, also, it'd be nice to state in the new docstring why we use the "-site" magic token

review: Approve (code)

[Michael Hudson-Doyle (mwhudson)]

I'm going to tweak a docstring formatting wise and then I'll land this.

=== modified file 'lib/lp/codehosting/__init__.py'
--- lib/lp/codehosting/__init__.py	2009-11-17 23:15:11 +0000
+++ lib/lp/codehosting/__init__.py	2010-02-19 19:41:19 +0000
@@ -10,7 +10,7 @@
 __metaclass__ = type
 __all__ = [
     'get_bzr_path',
-    'get_bzr_plugins_path',
+    'get_BZR_PLUGIN_PATH_for_subprocess',
     'iter_list_chunks',
     'load_optional_plugin',
     ]
@@ -47,16 +47,28 @@
             'bzr')
 
 
-def get_bzr_plugins_path():
+def _get_bzr_plugins_path():
     """Find the path to the Bazaar plugins for this rocketfuel instance"""
     return os.path.join(config.root, 'bzrplugins')
 
 
-os.environ['BZR_PLUGIN_PATH'] = get_bzr_plugins_path()
+def get_BZR_PLUGIN_PATH_for_subprocess():
+    """Calculate the appropriate value for the BZR_PLUGIN_PATH environment
+    variable for launching a Launchpad Bazaar subprocess.
+    
+    The '-site' token tells bzrlib not to include the 'site specific plugins
+    directory' (which is usually something like
+    /usr/lib/pythonX.Y/dist-packages/bzrlib/plugins/) in the plugin search
+    path, which would be inappropriate for Launchpad, which may be using a bzr
+    egg of an incompatible version."""
+    return ":".join((_get_bzr_plugins_path(), "-site"))
+
+
+os.environ['BZR_PLUGIN_PATH'] = get_BZR_PLUGIN_PATH_for_subprocess()
 
 # We want to have full access to Launchpad's Bazaar plugins throughout the
 # codehosting package.
-load_plugins([get_bzr_plugins_path()])
+load_plugins([_get_bzr_plugins_path()])
 
 
 def load_optional_plugin(plugin_name):

=== modified file 'lib/lp/codehosting/sshserver/tests/test_session.py'
--- lib/lp/codehosting/sshserver/tests/test_session.py	2009-06-25 04:06:00 +0000
+++ lib/lp/codehosting/sshserver/tests/test_session.py	2010-02-19 19:41:19 +0000
@@ -14,7 +14,7 @@
 from twisted.internet.protocol import ProcessProtocol
 
 from canonical.config import config
-from lp.codehosting import get_bzr_path, get_bzr_plugins_path
+from lp.codehosting import get_bzr_path, get_BZR_PLUGIN_PATH_for_subprocess
 from lp.codehosting.sshserver.auth import LaunchpadAvatar
 from lp.codehosting.sshserver.session import (
     ExecOnlySession, ForbiddenCommand, RestrictedExecOnlySession)
@@ -299,7 +299,7 @@
             "ISession(avatar) doesn't adapt to ExecOnlySession. "
             "Got %r instead." % (session,))
         self.assertEqual(
-            os.path.abspath(get_bzr_plugins_path()),
+            get_BZR_PLUGIN_PATH_for_subprocess(),
             session.environment['BZR_PLUGIN_PATH'])
         self.assertEqual(
             '[e-mail]' % self.avatar.username,

=== modified file 'lib/lp/codehosting/tests/test_acceptance.py'
--- lib/lp/codehosting/tests/test_acceptance.py	2010-01-21 22:14:31 +0000
+++ lib/lp/codehosting/tests/test_acceptance.py	2010-02-19 19:41:19 +0000
@@ -20,7 +20,7 @@
     adapt_suite, LoomTestMixin)
 from lp.codehosting.tests.servers import (
     CodeHostingTac, set_up_test_user, SSHCodeHostingServer)
-from lp.codehosting import get_bzr_path, get_bzr_plugins_path
+from lp.codehosting import get_bzr_path, get_BZR_PLUGIN_PATH_for_subprocess
 from lp.codehosting.vfs import branch_id_to_path
 from lp.registry.model.person import Person
 from lp.registry.model.product import Product
@@ -136,8 +136,10 @@
         (mainly so we can test the loom support).
         """
         return self.run_bzr_subprocess(
-            args, env_changes={'BZR_SSH': 'paramiko',
-                               'BZR_PLUGIN_PATH': get_bzr_plugins_path()},
+            args, env_changes={
+                'BZR_SSH': 'paramiko',
+                'BZR_PLUGIN_PATH': get_BZR_PLUGIN_PATH_for_subprocess()
+            },
             allow_plugins=True, retcode=retcode)
 
     def _run_bzr_error(self, args):

=== modified file 'lib/lp/codehosting/tests/test_lpserve.py'
--- lib/lp/codehosting/tests/test_lpserve.py	2009-10-16 01:54:41 +0000
+++ lib/lp/codehosting/tests/test_lpserve.py	2010-02-19 19:41:19 +0000
@@ -17,7 +17,7 @@
 
 from canonical.config import config
 
-from lp.codehosting import get_bzr_path, get_bzr_plugins_path
+from lp.codehosting import get_bzr_path, get_BZR_PLUGIN_PATH_for_subprocess
 from lp.codehosting.bzrutils import make_error_utility
 
 
@@ -31,35 +31,7 @@
 
     def assertFinishedCleanly(self, result):
         """Assert that a server process finished cleanly."""
-        # XXX gary 2009-10-15 bug 316192
-        # Ideally, this method would only be this single line:
-        #
-        # self.assertEqual((0, '', ''), tuple(result))
-        #
-        # However, because of the bug above, stderr (the last value) can
-        # include complaints that bzr tried to import certain plugins but
-        # was unable to.  This can make the last value into something like
-        # this (concatenate the strings):
-        #
-        #  "No module named dbus.bus\n"
-        #  "Unable to load plugin 'dbus' from "
-        #    "'/usr/lib/python2.4/site-packages/bzrlib/plugins'\n"
-        #  "No module named dbus.bus\n"
-        #  "Unable to load plugin 'avahi' from "
-        #    "'/usr/lib/python2.4/site-packages/bzrlib/plugins'\n"
-        #
-        # Therefore, for now, we allow stderr to have messages like
-        # that, with a regex.  A fix for the bzr bug mentioned above has
-        # already been released, so hopefully soon this method can
-        # return to the single assert above, this module will no longer
-        # have to import re, and this comment can be consigned to
-        # history.
-        self.assertEqual(0, result[0]) # the return code
-        self.assertEqual('', result[1]) # stdout
-        self.failUnless(re.match(
-            r"(No module named \S+\n|"
-               "Unable to load plugin \S+ from '/usr/lib/python[^']+'\n)*$",
-            result[2]))
+        self.assertEqual((0, '', ''), tuple(result))
 
     def get_python_path(self):
         """Return the path to the Python interpreter."""
@@ -79,7 +51,7 @@
         """
         if env_changes is None:
             env_changes = {}
-        env_changes['BZR_PLUGIN_PATH'] = get_bzr_plugins_path()
+        env_changes['BZR_PLUGIN_PATH'] = get_BZR_PLUGIN_PATH_for_subprocess()
         old_env = {}
 
         def cleanup_environment():