launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05903
[Merge] lp:~jelmer/launchpad/load-foreign-plugins into lp:launchpad
Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/load-foreign-plugins into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #509016 in Launchpad itself: "Please load bzr-git, bzr-svn and bzr-hg in loggerhead"
https://bugs.launchpad.net/launchpad/+bug/509016
For more details, see:
https://code.launchpad.net/~jelmer/launchpad/load-foreign-plugins/+merge/85261
Change the foreign branch plugins to always be loaded in Launchpad,
rather than only in the code importers.
The foreign branch plugins were previously only loaded conditionally because we
don't want the branch scanner and (former) puller to open non-bzr branches:
they're not properly supported by the rest of Launchpad, and less well
audited for security.
Since then, Launchpad has been changed to only open possibly insecure branches
through a single class (SafeBranchOpener) and bzr's infrastructure for
format handling has been improved to deal with different control directories
better. In particular, bzr now uses a single "Prober" per control directory
format (".bzr", remote, ".svn", remote svn, ".git", ".hg", etc).
This branch makes the SafeBranchOpener by default only open branches using
native bzr probers - BzrProber and RemoteBzrProber. This way we get
the same effect as we had previously (not scanning foreign branch directories),
but we can use other functionality from the foreign plugins where necessary.
The code importer manually overrides the probers where it needs.
This change should make loggerhead show foreign revision identifiers
(bug #509016) and allows us to drop the launchpad-specific delta for bzr-git.
It will also make it easier to integrate with foreign formats in other ways in
the future.
--
https://code.launchpad.net/~jelmer/launchpad/load-foreign-plugins/+merge/85261
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jelmer/launchpad/load-foreign-plugins into lp:launchpad.
=== renamed symlink 'bzrplugins/optional/git' => 'bzrplugins/git'
=== target changed u'../../sourcecode/bzr-git/' => u'../sourcecode/bzr-git'
=== renamed symlink 'bzrplugins/optional/hg' => 'bzrplugins/hg'
=== target changed u'../../sourcecode/bzr-hg' => u'../sourcecode/bzr-hg'
=== removed directory 'bzrplugins/optional'
=== removed file 'bzrplugins/optional/README'
--- bzrplugins/optional/README 2011-02-18 18:43:16 +0000
+++ bzrplugins/optional/README 1970-01-01 00:00:00 +0000
@@ -1,4 +0,0 @@
-These are for plugins that we do *not* want loaded all of the time.
-
-Specifically, the code import system loads these plugins, but we do not want
-the branch puller or scanner loading them.
=== renamed symlink 'bzrplugins/optional/svn' => 'bzrplugins/svn'
=== target changed u'../../sourcecode/bzr-svn' => u'../sourcecode/bzr-svn'
=== modified file 'lib/lp/codehosting/__init__.py'
--- lib/lp/codehosting/__init__.py 2011-06-23 16:00:55 +0000
+++ lib/lp/codehosting/__init__.py 2011-12-11 22:15:33 +0000
@@ -11,14 +11,12 @@
__all__ = [
'get_bzr_path',
'get_BZR_PLUGIN_PATH_for_subprocess',
- 'load_optional_plugin',
]
import os
import bzrlib
-from bzrlib import hooks
from bzrlib.plugin import load_plugins
from canonical.config import config
@@ -61,15 +59,6 @@
load_plugins([_get_bzr_plugins_path()])
-def load_optional_plugin(plugin_name):
- """Load the plugin named `plugin_name` from optionalbzrplugins/."""
- from bzrlib import plugins
- optional_plugin_dir = os.path.join(config.root, 'bzrplugins/optional')
- if optional_plugin_dir not in plugins.__path__:
- plugins.__path__.append(optional_plugin_dir)
- __import__("bzrlib.plugins.%s" % plugin_name)
-
-
def load_bundled_plugin(plugin_name):
"""Load a plugin bundled with Bazaar."""
from bzrlib.plugin import get_core_plugin_path
=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py 2011-11-14 08:30:52 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py 2011-12-11 22:15:33 +0000
@@ -57,7 +57,7 @@
branch_id_alias,
compose_public_url,
)
-from lp.codehosting import load_optional_plugin
+import lp.codehosting
from lp.codehosting.codeimport.tarball import (
create_tarball,
extract_tarball,
@@ -1143,7 +1143,6 @@
def setUp(self):
super(TestGitImport, self).setUp()
- load_optional_plugin('git')
self.setUpImport()
def tearDown(self):
@@ -1220,7 +1219,6 @@
def setUp(self):
super(TestMercurialImport, self).setUp()
- load_optional_plugin('hg')
self.setUpImport()
def tearDown(self):
@@ -1302,7 +1300,6 @@
def setUp(self):
super(TestBzrSvnImport, self).setUp()
- load_optional_plugin('svn')
self.setUpImport()
def makeImportWorker(self, source_details, opener_policy):
=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py 2011-11-09 14:31:52 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py 2011-12-11 22:15:33 +0000
@@ -49,7 +49,6 @@
from lp.code.interfaces.codeimportjob import ICodeImportJobSet
from lp.code.model.codeimport import CodeImport
from lp.code.model.codeimportjob import CodeImportJob
-from lp.codehosting import load_optional_plugin
from lp.codehosting.codeimport.tests.servers import (
BzrServer,
CVSServer,
@@ -742,7 +741,6 @@
def makeGitCodeImport(self):
"""Make a `CodeImport` that points to a real Git repository."""
- load_optional_plugin('git')
self.git_server = GitServer(self.repo_path, use_server=False)
self.git_server.start_server()
self.addCleanup(self.git_server.stop_server)
@@ -755,7 +753,6 @@
def makeHgCodeImport(self):
"""Make a `CodeImport` that points to a real Mercurial repository."""
- load_optional_plugin('hg')
self.hg_server = MercurialServer(self.repo_path, use_server=False)
self.hg_server.start_server()
self.addCleanup(self.hg_server.stop_server)
=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py 2011-11-14 08:30:52 +0000
+++ lib/lp/codehosting/codeimport/worker.py 2011-12-11 22:15:33 +0000
@@ -40,7 +40,6 @@
TooManyRedirections,
)
from bzrlib.transport import (
- do_catching_redirections,
get_transport_from_path,
get_transport_from_url,
)
@@ -66,6 +65,7 @@
branch_id_alias,
compose_public_url,
)
+import lp.codehosting # for bzr plugins
from lp.codehosting.codeimport.foreigntree import (
CVSWorkingTree,
SubversionWorkingTree,
@@ -711,49 +711,17 @@
"""
return None
- def _open_dir(self, url):
- """Simple BzrDir.open clone that only uses self.probers.
-
- :param url: URL to open
- :return: ControlDir instance
- """
- def redirected(transport, e, redirection_notice):
- self._opener_policy.checkOneURL(e.target)
- redirected_transport = transport._redirected_to(
- e.source, e.target)
- if redirected_transport is None:
- raise NotBranchError(e.source)
- self._logger.info('%s is%s redirected to %s',
- transport.base, e.permanently, redirected_transport.base)
- return redirected_transport
-
- def find_format(transport):
- last_error = None
- for prober_kls in self.probers:
- prober = prober_kls()
- try:
- return transport, prober.probe_transport(transport)
- except NotBranchError, e:
- last_error = e
- else:
- raise last_error
- transport = get_transport_from_url(url)
- transport, format = do_catching_redirections(find_format, transport,
- redirected)
- return format.open(transport)
-
def _doImport(self):
self._logger.info("Starting job.")
saved_factory = bzrlib.ui.ui_factory
- opener = SafeBranchOpener(self._opener_policy)
+ opener = SafeBranchOpener(self._opener_policy, self.probers)
bzrlib.ui.ui_factory = LoggingUIFactory(logger=self._logger)
try:
self._logger.info(
"Getting exising bzr branch from central store.")
bazaar_branch = self.getBazaarBranch()
try:
- remote_branch = opener.open(
- self.source_details.url, self._open_dir)
+ remote_branch = opener.open(self.source_details.url)
except TooManyRedirections:
self._logger.info("Too many redirections.")
return CodeImportWorkerExitCode.FAILURE_INVALID
=== modified file 'lib/lp/codehosting/puller/worker.py'
--- lib/lp/codehosting/puller/worker.py 2011-10-25 04:43:25 +0000
+++ lib/lp/codehosting/puller/worker.py 2011-12-11 22:15:33 +0000
@@ -214,7 +214,7 @@
:return: The destination branch.
"""
return self.opener.runWithTransformFallbackLocationHookInstalled(
- BzrDir.open, self.policy.createDestinationBranch, source_branch,
+ self.policy.createDestinationBranch, source_branch,
destination_url)
def openDestinationBranch(self, source_branch, destination_url):
=== modified file 'lib/lp/codehosting/safe_open.py'
--- lib/lp/codehosting/safe_open.py 2011-09-15 11:57:20 +0000
+++ lib/lp/codehosting/safe_open.py 2011-12-11 22:15:33 +0000
@@ -7,10 +7,21 @@
import threading
-from bzrlib import urlutils
+from bzrlib import (
+ trace,
+ errors,
+ urlutils,
+ )
from bzrlib.branch import Branch
-from bzrlib.bzrdir import BzrDir
+from bzrlib.bzrdir import (
+ BzrProber,
+ RemoteBzrProber,
+ )
from lazr.uri import URI
+from bzrlib.transport import (
+ do_catching_redirections,
+ get_transport,
+ )
__all__ = [
@@ -29,6 +40,10 @@
# TODO JelmerVernooij 2011-08-06: This module is generic enough to be
# in bzrlib, and may be of use to others. bug=850843
+# These are the default probers that SafeBranchOpener will try to use,
+# unless a different set was specified.
+
+DEFAULT_PROBERS = [BzrProber, RemoteBzrProber]
class BadUrl(Exception):
"""Tried to access a branch from a bad URL."""
@@ -184,9 +199,19 @@
_threading_data = threading.local()
- def __init__(self, policy):
+ def __init__(self, policy, probers=None):
+ """Create a new SafeBranchOpener.
+
+ :param policy: opener policy to use
+ :param probers: Optional list of probers to allow.
+ Defaults to local and remote bzr probers.
+ """
self.policy = policy
self._seen_urls = set()
+ if probers is None:
+ self.probers = list(DEFAULT_PROBERS)
+ else:
+ self.probers = probers
@classmethod
def install_hook(cls):
@@ -205,7 +230,7 @@
cls.transformFallbackLocationHook,
'SafeBranchOpener.transformFallbackLocationHook')
- def checkAndFollowBranchReference(self, url, open_dir=None):
+ def checkAndFollowBranchReference(self, url):
"""Check URL (and possibly the referenced URL) for safety.
This method checks that `url` passes the policy's `checkOneURL`
@@ -214,8 +239,6 @@
also -- recursively, until a real branch is found.
:param url: URL to check
- :param open_dir: Optional function to use for opening control
- directories (defaults to BzrDir.open)
:raise BranchLoopError: If the branch references form a loop.
:raise BranchReferenceForbidden: If this opener forbids branch
references.
@@ -225,7 +248,7 @@
raise BranchLoopError()
self._seen_urls.add(url)
self.policy.checkOneURL(url)
- next_url = self.followReference(url, open_dir=open_dir)
+ next_url = self.followReference(url)
if next_url is None:
return url
url = next_url
@@ -246,54 +269,75 @@
return url
new_url, check = opener.policy.transformFallbackLocation(branch, url)
if check:
- return opener.checkAndFollowBranchReference(new_url,
- getattr(cls._threading_data, "open_dir"))
+ return opener.checkAndFollowBranchReference(new_url)
else:
return new_url
def runWithTransformFallbackLocationHookInstalled(
- self, open_dir, callable, *args, **kw):
+ self, callable, *args, **kw):
assert (self.transformFallbackLocationHook in
Branch.hooks['transform_fallback_location'])
self._threading_data.opener = self
- self._threading_data.open_dir = open_dir
try:
return callable(*args, **kw)
finally:
- del self._threading_data.open_dir
del self._threading_data.opener
# We reset _seen_urls here to avoid multiple calls to open giving
# spurious loop exceptions.
self._seen_urls = set()
- def followReference(self, url, open_dir=None):
+ def followReference(self, url):
"""Get the branch-reference value at the specified url.
This exists as a separate method only to be overriden in unit tests.
"""
- if open_dir is None:
- open_dir = BzrDir.open
- bzrdir = open_dir(url)
+ bzrdir = self._open_dir(url)
return bzrdir.get_branch_reference()
- def open(self, url, open_dir=None):
+ def _open_dir(self, url):
+ """Simple BzrDir.open clone that only uses specific probers.
+
+ :param url: URL to open
+ :return: ControlDir instance
+ """
+ def redirected(transport, e, redirection_notice):
+ self.policy.checkOneURL(e.target)
+ redirected_transport = transport._redirected_to(
+ e.source, e.target)
+ if redirected_transport is None:
+ raise errors.NotBranchError(e.source)
+ trace.note('%s is%s redirected to %s',
+ transport.base, e.permanently, redirected_transport.base)
+ return redirected_transport
+
+ def find_format(transport):
+ last_error = errors.NotBranchError(transport.base)
+ for prober_kls in self.probers:
+ prober = prober_kls()
+ try:
+ return transport, prober.probe_transport(transport)
+ except errors.NotBranchError, e:
+ last_error = e
+ else:
+ raise last_error
+ transport = get_transport(url)
+ transport, format = do_catching_redirections(find_format, transport,
+ redirected)
+ return format.open(transport)
+
+ def open(self, url):
"""Open the Bazaar branch at url, first checking for safety.
What safety means is defined by a subclasses `followReference` and
`checkOneURL` methods.
-
- :param open_dir: Optional function to use for opening control
- directories (defaults to BzrDir.open)
"""
- url = self.checkAndFollowBranchReference(url, open_dir=open_dir)
- if open_dir is None:
- open_dir = BzrDir.open
+ url = self.checkAndFollowBranchReference(url)
def open_branch(url):
- dir = open_dir(url)
+ dir = self._open_dir(url)
return dir.open_branch()
return self.runWithTransformFallbackLocationHookInstalled(
- open_dir, open_branch, url)
+ open_branch, url)
def safe_open(allowed_scheme, url):
=== modified file 'lib/lp/codehosting/tests/test_acceptance.py'
--- lib/lp/codehosting/tests/test_acceptance.py 2011-09-02 16:07:17 +0000
+++ lib/lp/codehosting/tests/test_acceptance.py 2011-12-11 22:15:33 +0000
@@ -756,6 +756,9 @@
'RepositoryFormat5',
'RepositoryFormat6',
'RepositoryFormat7',
+ 'HgRepositoryFormat',
+ 'GitRepositoryFormat',
+ 'SvnRepositoryFormat',
]
scenarios = all_repository_format_scenarios()
scenarios = [
=== modified file 'lib/lp/codehosting/tests/test_bzrutils.py'
--- lib/lp/codehosting/tests/test_bzrutils.py 2011-11-14 08:30:52 +0000
+++ lib/lp/codehosting/tests/test_bzrutils.py 2011-12-11 22:15:33 +0000
@@ -224,7 +224,9 @@
get_branch_stacked_on_url_tests = loader.loadTestsFromTestCase(
TestGetBranchStackedOnURL)
scenarios = [scenario for scenario in branch_scenarios()
- if scenario[0] != 'BranchReferenceFormat']
+ if scenario[0] not in (
+ 'BranchReferenceFormat', 'GitBranchFormat',
+ 'HgBranchFormat', 'SvnBranchFormat')]
multiply_tests(get_branch_stacked_on_url_tests, scenarios, result)
result.addTests(loader.loadTestsFromTestCase(TestIsBranchStackable))
=== modified file 'lib/lp/codehosting/tests/test_safe_open.py'
--- lib/lp/codehosting/tests/test_safe_open.py 2011-09-12 11:25:28 +0000
+++ lib/lp/codehosting/tests/test_safe_open.py 2011-12-11 22:15:33 +0000
@@ -14,7 +14,10 @@
from bzrlib.bzrdir import (
BzrDir,
BzrDirMetaFormat1,
+ BzrProber,
)
+from bzrlib.controldir import ControlDirFormat
+from bzrlib.errors import NotBranchError
from bzrlib.repofmt.knitpack_repo import RepositoryFormatKnitPack1
from bzrlib.tests import TestCaseWithTransport
from bzrlib.transport import chroot
@@ -55,7 +58,7 @@
self._reference_values[references[i]] = references[i + 1]
self.follow_reference_calls = []
- def followReference(self, url, open_dir=None):
+ def followReference(self, url):
self.follow_reference_calls.append(url)
return self._reference_values[url]
@@ -68,13 +71,15 @@
def testCheckInitialURL(self):
# checkSource rejects all URLs that are not allowed.
opener = self.makeBranchOpener(None, [], set(['a']))
- self.assertRaises(BadUrl, opener.checkAndFollowBranchReference, 'a')
+ self.assertRaises(
+ BadUrl, opener.checkAndFollowBranchReference, 'a')
def testNotReference(self):
# When branch references are forbidden, checkAndFollowBranchReference
# does not raise on non-references.
opener = self.makeBranchOpener(False, ['a', None])
- self.assertEquals('a', opener.checkAndFollowBranchReference('a'))
+ self.assertEquals(
+ 'a', opener.checkAndFollowBranchReference('a'))
self.assertEquals(['a'], opener.follow_reference_calls)
def testBranchReferenceForbidden(self):
@@ -92,7 +97,8 @@
# is allowed and the source URL points to a branch reference to a
# permitted location.
opener = self.makeBranchOpener(True, ['a', 'b', None])
- self.assertEquals('b', opener.checkAndFollowBranchReference('a'))
+ self.assertEquals(
+ 'b', opener.checkAndFollowBranchReference('a'))
self.assertEquals(['a', 'b'], opener.follow_reference_calls)
def testCheckReferencedURLs(self):
@@ -100,7 +106,8 @@
# to is safe.
opener = self.makeBranchOpener(
True, ['a', 'b', None], unsafe_urls=set('b'))
- self.assertRaises(BadUrl, opener.checkAndFollowBranchReference, 'a')
+ self.assertRaises(
+ BadUrl, opener.checkAndFollowBranchReference, 'a')
self.assertEquals(['a'], opener.follow_reference_calls)
def testSelfReferencingBranch(self):
@@ -129,9 +136,9 @@
super(TestSafeBranchOpenerStacking, self).setUp()
SafeBranchOpener.install_hook()
- def makeBranchOpener(self, allowed_urls):
+ def makeBranchOpener(self, allowed_urls, probers=None):
policy = WhitelistPolicy(True, allowed_urls, True)
- return SafeBranchOpener(policy)
+ return SafeBranchOpener(policy, probers)
def makeBranch(self, path, branch_format, repository_format):
"""Make a Bazaar branch at 'path' with the given formats."""
@@ -141,6 +148,47 @@
repository_format.initialize(bzrdir)
return bzrdir.create_branch()
+ def testProbers(self):
+ # Only the specified probers should be used
+ b = self.make_branch('branch')
+ opener = self.makeBranchOpener([b.base], probers=[])
+ self.assertRaises(NotBranchError, opener.open, b.base)
+ opener = self.makeBranchOpener([b.base], probers=[BzrProber])
+ self.assertEquals(b.base, opener.open(b.base).base)
+
+ def testDefaultProbers(self):
+ # If no probers are specified to the constructor
+ # of SafeBranchOpener, then a safe set will be used,
+ # rather than all probers registered in bzr.
+ class TrackingProber(BzrProber):
+
+ calls = []
+
+ @classmethod
+ def probe_transport(klass, transport):
+ klass.calls.append(transport)
+ return BzrProber.probe_transport(transport)
+
+ self.addCleanup(ControlDirFormat.unregister_prober, TrackingProber)
+ ControlDirFormat.register_prober(TrackingProber)
+ # Open a location without any branches, so that all probers are
+ # tried.
+ # First, check that the TrackingProber tracks correctly.
+ opener = self.makeBranchOpener(["."], probers=[TrackingProber])
+ self.assertRaises(NotBranchError, opener.open, ".")
+ self.assertEquals(1, len(TrackingProber.calls))
+ TrackingProber.calls = []
+ # And make sure it's registered in such a way that BzrDir.open would
+ # use it.
+ self.assertRaises(NotBranchError, BzrDir.open, ".")
+ self.assertEquals(1, len(TrackingProber.calls))
+ TrackingProber.calls = []
+ # Make sure that SafeBranchOpener doesn't use it if no
+ # probers were specified
+ opener = self.makeBranchOpener(["."])
+ self.assertRaises(NotBranchError, opener.open, ".")
+ self.assertEquals(0, len(TrackingProber.calls))
+
def testAllowedURL(self):
# checkSource does not raise an exception for branches stacked on
# branches with allowed URLs.
@@ -237,12 +285,16 @@
b.set_stacked_on_url(a.base)
seen_urls = set()
- def open_dir(url):
- seen_urls.add(url)
- return BzrDir.open(url)
-
- opener = self.makeBranchOpener([a.base, b.base])
- opener.open(b.base, open_dir=open_dir)
+ class TrackingProber(BzrProber):
+
+ @classmethod
+ def probe_transport(klass, transport):
+ seen_urls.add(transport.base)
+ return BzrProber.probe_transport(transport)
+
+ opener = self.makeBranchOpener(
+ [a.base, b.base], probers=[TrackingProber])
+ opener.open(b.base)
self.assertEquals(seen_urls, set([b.base, a.base]))
def testCustomOpenerWithBranchReference(self):
@@ -252,12 +304,16 @@
b = BranchReferenceFormat().initialize(b_dir, target_branch=a)
seen_urls = set()
- def open_dir(url):
- seen_urls.add(url)
- return BzrDir.open(url)
-
- opener = self.makeBranchOpener([a.base, b.base])
- opener.open(b.base, open_dir=open_dir)
+ class TrackingProber(BzrProber):
+
+ @classmethod
+ def probe_transport(klass, transport):
+ seen_urls.add(transport.base)
+ return BzrProber.probe_transport(transport)
+
+ opener = self.makeBranchOpener(
+ [a.base, b.base], probers=[TrackingProber])
+ opener.open(b.base)
self.assertEquals(seen_urls, set([b.base, a.base]))
=== modified file 'scripts/code-import-worker.py'
--- scripts/code-import-worker.py 2011-08-24 23:05:26 +0000
+++ scripts/code-import-worker.py 2011-12-11 22:15:33 +0000
@@ -24,7 +24,6 @@
from bzrlib.transport import get_transport
from canonical.config import config
-from lp.codehosting import load_optional_plugin
from lp.codehosting.codeimport.worker import (
BzrImportWorker, BzrSvnImportWorker, CSCVSImportWorker,
CodeImportBranchOpenPolicy, CodeImportSourceDetails, GitImportWorker,
@@ -69,16 +68,12 @@
force_bzr_to_use_urllib()
source_details = CodeImportSourceDetails.fromArguments(self.args)
if source_details.rcstype == 'git':
- load_optional_plugin('git')
import_worker_cls = GitImportWorker
elif source_details.rcstype == 'bzr-svn':
- load_optional_plugin('svn')
import_worker_cls = BzrSvnImportWorker
elif source_details.rcstype == 'hg':
- load_optional_plugin('hg')
import_worker_cls = HgImportWorker
elif source_details.rcstype == 'bzr':
- load_optional_plugin('loom')
import_worker_cls = BzrImportWorker
elif source_details.rcstype in ['cvs', 'svn']:
import_worker_cls = CSCVSImportWorker
