Launchpad itself

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

  1. bug-497731
  2. Merge into devel
Proposed by Max Bowsher
Status: Merged
Merged at revision: not available
Proposed branch: lp:~maxb/launchpad/bug-497731
Merge into: lp:launchpad
Diff against target: 151 lines (+26/-40)
4 files modified
lib/lp/codehosting/__init__.py (+16/-4)
lib/lp/codehosting/sshserver/tests/test_session.py (+2/-2)
lib/lp/codehosting/tests/test_acceptance.py (+5/-3)
lib/lp/codehosting/tests/test_lpserve.py (+3/-31)
To merge this branch: bzr merge lp:~maxb/launchpad/bug-497731
Reviewer Review Type Date Requested Status
Guilherme Salgado (community) code Approve
Review via email: mp+19732@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Max Bowsher (maxb) wrote :

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.

Revision history for this message
Guilherme Salgado (salgado) wrote :

<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)
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
10__metaclass__ = type10__metaclass__ = type
11__all__ = [11__all__ = [
12 'get_bzr_path',12 'get_bzr_path',
13 'get_bzr_plugins_path',13 'get_BZR_PLUGIN_PATH_for_subprocess',
14 'iter_list_chunks',14 'iter_list_chunks',
15 'load_optional_plugin',15 'load_optional_plugin',
16 ]16 ]
@@ -47,16 +47,28 @@
47 'bzr')47 'bzr')
4848
4949
50def get_bzr_plugins_path():50def _get_bzr_plugins_path():
51 """Find the path to the Bazaar plugins for this rocketfuel instance"""51 """Find the path to the Bazaar plugins for this rocketfuel instance"""
52 return os.path.join(config.root, 'bzrplugins')52 return os.path.join(config.root, 'bzrplugins')
5353
5454
55os.environ['BZR_PLUGIN_PATH'] = get_bzr_plugins_path()55def get_BZR_PLUGIN_PATH_for_subprocess():
56 """Calculate the appropriate value for the BZR_PLUGIN_PATH environment
57 variable for launching a Launchpad Bazaar subprocess.
58
59 The '-site' token tells bzrlib not to include the 'site specific plugins
60 directory' (which is usually something like
61 /usr/lib/pythonX.Y/dist-packages/bzrlib/plugins/) in the plugin search
62 path, which would be inappropriate for Launchpad, which may be using a bzr
63 egg of an incompatible version."""
64 return ":".join((_get_bzr_plugins_path(), "-site"))
65
66
67os.environ['BZR_PLUGIN_PATH'] = get_BZR_PLUGIN_PATH_for_subprocess()
5668
57# We want to have full access to Launchpad's Bazaar plugins throughout the69# We want to have full access to Launchpad's Bazaar plugins throughout the
58# codehosting package.70# codehosting package.
59load_plugins([get_bzr_plugins_path()])71load_plugins([_get_bzr_plugins_path()])
6072
6173
62def load_optional_plugin(plugin_name):74def load_optional_plugin(plugin_name):
6375
=== 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 @@
14from twisted.internet.protocol import ProcessProtocol14from twisted.internet.protocol import ProcessProtocol
1515
16from canonical.config import config16from canonical.config import config
17from lp.codehosting import get_bzr_path, get_bzr_plugins_path17from lp.codehosting import get_bzr_path, get_BZR_PLUGIN_PATH_for_subprocess
18from lp.codehosting.sshserver.auth import LaunchpadAvatar18from lp.codehosting.sshserver.auth import LaunchpadAvatar
19from lp.codehosting.sshserver.session import (19from lp.codehosting.sshserver.session import (
20 ExecOnlySession, ForbiddenCommand, RestrictedExecOnlySession)20 ExecOnlySession, ForbiddenCommand, RestrictedExecOnlySession)
@@ -299,7 +299,7 @@
299 "ISession(avatar) doesn't adapt to ExecOnlySession. "299 "ISession(avatar) doesn't adapt to ExecOnlySession. "
300 "Got %r instead." % (session,))300 "Got %r instead." % (session,))
301 self.assertEqual(301 self.assertEqual(
302 os.path.abspath(get_bzr_plugins_path()),302 get_BZR_PLUGIN_PATH_for_subprocess(),
303 session.environment['BZR_PLUGIN_PATH'])303 session.environment['BZR_PLUGIN_PATH'])
304 self.assertEqual(304 self.assertEqual(
305 '%s@bazaar.launchpad.dev' % self.avatar.username,305 '%s@bazaar.launchpad.dev' % self.avatar.username,
306306
=== 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 @@
20 adapt_suite, LoomTestMixin)20 adapt_suite, LoomTestMixin)
21from lp.codehosting.tests.servers import (21from lp.codehosting.tests.servers import (
22 CodeHostingTac, set_up_test_user, SSHCodeHostingServer)22 CodeHostingTac, set_up_test_user, SSHCodeHostingServer)
23from lp.codehosting import get_bzr_path, get_bzr_plugins_path23from lp.codehosting import get_bzr_path, get_BZR_PLUGIN_PATH_for_subprocess
24from lp.codehosting.vfs import branch_id_to_path24from lp.codehosting.vfs import branch_id_to_path
25from lp.registry.model.person import Person25from lp.registry.model.person import Person
26from lp.registry.model.product import Product26from lp.registry.model.product import Product
@@ -136,8 +136,10 @@
136 (mainly so we can test the loom support).136 (mainly so we can test the loom support).
137 """137 """
138 return self.run_bzr_subprocess(138 return self.run_bzr_subprocess(
139 args, env_changes={'BZR_SSH': 'paramiko',139 args, env_changes={
140 'BZR_PLUGIN_PATH': get_bzr_plugins_path()},140 'BZR_SSH': 'paramiko',
141 'BZR_PLUGIN_PATH': get_BZR_PLUGIN_PATH_for_subprocess()
142 },
141 allow_plugins=True, retcode=retcode)143 allow_plugins=True, retcode=retcode)
142144
143 def _run_bzr_error(self, args):145 def _run_bzr_error(self, args):
144146
=== 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 @@
1717
18from canonical.config import config18from canonical.config import config
1919
20from lp.codehosting import get_bzr_path, get_bzr_plugins_path20from lp.codehosting import get_bzr_path, get_BZR_PLUGIN_PATH_for_subprocess
21from lp.codehosting.bzrutils import make_error_utility21from lp.codehosting.bzrutils import make_error_utility
2222
2323
@@ -31,35 +31,7 @@
3131
32 def assertFinishedCleanly(self, result):32 def assertFinishedCleanly(self, result):
33 """Assert that a server process finished cleanly."""33 """Assert that a server process finished cleanly."""
34 # XXX gary 2009-10-15 bug 31619234 self.assertEqual((0, '', ''), tuple(result))
35 # Ideally, this method would only be this single line:
36 #
37 # self.assertEqual((0, '', ''), tuple(result))
38 #
39 # However, because of the bug above, stderr (the last value) can
40 # include complaints that bzr tried to import certain plugins but
41 # was unable to. This can make the last value into something like
42 # this (concatenate the strings):
43 #
44 # "No module named dbus.bus\n"
45 # "Unable to load plugin 'dbus' from "
46 # "'/usr/lib/python2.4/site-packages/bzrlib/plugins'\n"
47 # "No module named dbus.bus\n"
48 # "Unable to load plugin 'avahi' from "
49 # "'/usr/lib/python2.4/site-packages/bzrlib/plugins'\n"
50 #
51 # Therefore, for now, we allow stderr to have messages like
52 # that, with a regex. A fix for the bzr bug mentioned above has
53 # already been released, so hopefully soon this method can
54 # return to the single assert above, this module will no longer
55 # have to import re, and this comment can be consigned to
56 # history.
57 self.assertEqual(0, result[0]) # the return code
58 self.assertEqual('', result[1]) # stdout
59 self.failUnless(re.match(
60 r"(No module named \S+\n|"
61 "Unable to load plugin \S+ from '/usr/lib/python[^']+'\n)*$",
62 result[2]))
6335
64 def get_python_path(self):36 def get_python_path(self):
65 """Return the path to the Python interpreter."""37 """Return the path to the Python interpreter."""
@@ -79,7 +51,7 @@
79 """51 """
80 if env_changes is None:52 if env_changes is None:
81 env_changes = {}53 env_changes = {}
82 env_changes['BZR_PLUGIN_PATH'] = get_bzr_plugins_path()54 env_changes['BZR_PLUGIN_PATH'] = get_BZR_PLUGIN_PATH_for_subprocess()
83 old_env = {}55 old_env = {}
8456
85 def cleanup_environment():57 def cleanup_environment():