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
1=== modified file 'lib/lp/codehosting/__init__.py'
2--- lib/lp/codehosting/__init__.py 2009-11-17 23:15:11 +0000
3+++ lib/lp/codehosting/__init__.py 2010-02-19 19:41:19 +0000
4@@ -10,7 +10,7 @@
5 __metaclass__ = type
6 __all__ = [
7 'get_bzr_path',
8- 'get_bzr_plugins_path',
9+ 'get_BZR_PLUGIN_PATH_for_subprocess',
10 'iter_list_chunks',
11 'load_optional_plugin',
12 ]
13@@ -47,16 +47,28 @@
14 'bzr')
15
16
17-def get_bzr_plugins_path():
18+def _get_bzr_plugins_path():
19 """Find the path to the Bazaar plugins for this rocketfuel instance"""
20 return os.path.join(config.root, 'bzrplugins')
21
22
23-os.environ['BZR_PLUGIN_PATH'] = get_bzr_plugins_path()
24+def get_BZR_PLUGIN_PATH_for_subprocess():
25+ """Calculate the appropriate value for the BZR_PLUGIN_PATH environment
26+ variable for launching a Launchpad Bazaar subprocess.
27+
28+ The '-site' token tells bzrlib not to include the 'site specific plugins
29+ directory' (which is usually something like
30+ /usr/lib/pythonX.Y/dist-packages/bzrlib/plugins/) in the plugin search
31+ path, which would be inappropriate for Launchpad, which may be using a bzr
32+ egg of an incompatible version."""
33+ return ":".join((_get_bzr_plugins_path(), "-site"))
34+
35+
36+os.environ['BZR_PLUGIN_PATH'] = get_BZR_PLUGIN_PATH_for_subprocess()
37
38 # We want to have full access to Launchpad's Bazaar plugins throughout the
39 # codehosting package.
40-load_plugins([get_bzr_plugins_path()])
41+load_plugins([_get_bzr_plugins_path()])
42
43
44 def load_optional_plugin(plugin_name):
45
46=== modified file 'lib/lp/codehosting/sshserver/tests/test_session.py'
47--- lib/lp/codehosting/sshserver/tests/test_session.py 2009-06-25 04:06:00 +0000
48+++ lib/lp/codehosting/sshserver/tests/test_session.py 2010-02-19 19:41:19 +0000
49@@ -14,7 +14,7 @@
50 from twisted.internet.protocol import ProcessProtocol
51
52 from canonical.config import config
53-from lp.codehosting import get_bzr_path, get_bzr_plugins_path
54+from lp.codehosting import get_bzr_path, get_BZR_PLUGIN_PATH_for_subprocess
55 from lp.codehosting.sshserver.auth import LaunchpadAvatar
56 from lp.codehosting.sshserver.session import (
57 ExecOnlySession, ForbiddenCommand, RestrictedExecOnlySession)
58@@ -299,7 +299,7 @@
59 "ISession(avatar) doesn't adapt to ExecOnlySession. "
60 "Got %r instead." % (session,))
61 self.assertEqual(
62- os.path.abspath(get_bzr_plugins_path()),
63+ get_BZR_PLUGIN_PATH_for_subprocess(),
64 session.environment['BZR_PLUGIN_PATH'])
65 self.assertEqual(
66 '%s@bazaar.launchpad.dev' % self.avatar.username,
67
68=== modified file 'lib/lp/codehosting/tests/test_acceptance.py'
69--- lib/lp/codehosting/tests/test_acceptance.py 2010-01-21 22:14:31 +0000
70+++ lib/lp/codehosting/tests/test_acceptance.py 2010-02-19 19:41:19 +0000
71@@ -20,7 +20,7 @@
72 adapt_suite, LoomTestMixin)
73 from lp.codehosting.tests.servers import (
74 CodeHostingTac, set_up_test_user, SSHCodeHostingServer)
75-from lp.codehosting import get_bzr_path, get_bzr_plugins_path
76+from lp.codehosting import get_bzr_path, get_BZR_PLUGIN_PATH_for_subprocess
77 from lp.codehosting.vfs import branch_id_to_path
78 from lp.registry.model.person import Person
79 from lp.registry.model.product import Product
80@@ -136,8 +136,10 @@
81 (mainly so we can test the loom support).
82 """
83 return self.run_bzr_subprocess(
84- args, env_changes={'BZR_SSH': 'paramiko',
85- 'BZR_PLUGIN_PATH': get_bzr_plugins_path()},
86+ args, env_changes={
87+ 'BZR_SSH': 'paramiko',
88+ 'BZR_PLUGIN_PATH': get_BZR_PLUGIN_PATH_for_subprocess()
89+ },
90 allow_plugins=True, retcode=retcode)
91
92 def _run_bzr_error(self, args):
93
94=== modified file 'lib/lp/codehosting/tests/test_lpserve.py'
95--- lib/lp/codehosting/tests/test_lpserve.py 2009-10-16 01:54:41 +0000
96+++ lib/lp/codehosting/tests/test_lpserve.py 2010-02-19 19:41:19 +0000
97@@ -17,7 +17,7 @@
98
99 from canonical.config import config
100
101-from lp.codehosting import get_bzr_path, get_bzr_plugins_path
102+from lp.codehosting import get_bzr_path, get_BZR_PLUGIN_PATH_for_subprocess
103 from lp.codehosting.bzrutils import make_error_utility
104
105
106@@ -31,35 +31,7 @@
107
108 def assertFinishedCleanly(self, result):
109 """Assert that a server process finished cleanly."""
110- # XXX gary 2009-10-15 bug 316192
111- # Ideally, this method would only be this single line:
112- #
113- # self.assertEqual((0, '', ''), tuple(result))
114- #
115- # However, because of the bug above, stderr (the last value) can
116- # include complaints that bzr tried to import certain plugins but
117- # was unable to. This can make the last value into something like
118- # this (concatenate the strings):
119- #
120- # "No module named dbus.bus\n"
121- # "Unable to load plugin 'dbus' from "
122- # "'/usr/lib/python2.4/site-packages/bzrlib/plugins'\n"
123- # "No module named dbus.bus\n"
124- # "Unable to load plugin 'avahi' from "
125- # "'/usr/lib/python2.4/site-packages/bzrlib/plugins'\n"
126- #
127- # Therefore, for now, we allow stderr to have messages like
128- # that, with a regex. A fix for the bzr bug mentioned above has
129- # already been released, so hopefully soon this method can
130- # return to the single assert above, this module will no longer
131- # have to import re, and this comment can be consigned to
132- # history.
133- self.assertEqual(0, result[0]) # the return code
134- self.assertEqual('', result[1]) # stdout
135- self.failUnless(re.match(
136- r"(No module named \S+\n|"
137- "Unable to load plugin \S+ from '/usr/lib/python[^']+'\n)*$",
138- result[2]))
139+ self.assertEqual((0, '', ''), tuple(result))
140
141 def get_python_path(self):
142 """Return the path to the Python interpreter."""
143@@ -79,7 +51,7 @@
144 """
145 if env_changes is None:
146 env_changes = {}
147- env_changes['BZR_PLUGIN_PATH'] = get_bzr_plugins_path()
148+ env_changes['BZR_PLUGIN_PATH'] = get_BZR_PLUGIN_PATH_for_subprocess()
149 old_env = {}
150
151 def cleanup_environment():