Merge lp:~vila/bzr/552922-plugins-at into lp:bzr

[Vincent Ladeuil]

Since https://code.edge.launchpad.net/~vila/bzr/552922-plugins-at/+merge/22694
was broken, I'm proposing against bzr.dev to get a reasonable diff,
but the intent is still to land in 2.2 when it will be updated.

This patch fixes the BZR_PLUGINS_AT handling to not depend on os.listdir() order.
The automated tests couldn't catch the problem and I was lucky enough to not encounter
it before (and neither was Gary).

Unlike _find_module_plugin() which scan the 'plugins' directories and deduce the plugin
names from the loadable files, BZR_PLUGINS_AT *knows* the plugin names and can (and should)
try some file names explicitly.

Tests added.

[Gordon Tyler (doxxx)]

This fixes the problem I was having in bug 552922.

review: Approve

[Ian Clatworthy (ian-clatworthy)]

Some requested tweaks ...

> name = fullname[len('bzrlib.plugins.'):]

This variable is never used. Perhaps you wanted to use it in the ImportError? Otherwise, just delete this line please.

> def test_loading_from___init__(self):

Given this test doesn't actual load from __init__ but fails because that file is not there (after an explicit rename), I'd prefer a better test name, e.g. test_loading_from___init__only() say.

review: Approve

=== modified file 'NEWS'
--- NEWS	2010-04-06 06:59:03 +0000
+++ NEWS	2010-04-06 07:24:38 +0000
@@ -205,6 +205,10 @@
   deletion is involved.
   (Vincent Ladeuil, #531967)
 
+* Loading a plugin from a given path with ``BZR_PLUGINS_AT`` doesn't depend
+  on os.lisdir() order and is now reliable.
+  (Vincent Ladeuil, #552922).
+
 * Network transfer amounts and rates are now displayed in SI units according
   to the Ubuntu Units Policy <https://wiki.ubuntu.com/UnitsPolicy>.
   (Gordon Tyler, #514399)
 
=== modified file 'bzrlib/plugin.py'
--- bzrlib/plugin.py	2010-03-24 13:57:35 +0000
+++ bzrlib/plugin.py	2010-04-06 07:24:38 +0000
@@ -303,7 +303,7 @@
 
 
 def _load_plugin_module(name, dir):
-    """Load plugine name from dir.
+    """Load plugin name from dir.
 
     :param name: The plugin name in the bzrlib.plugins namespace.
     :param dir: The directory the plugin is loaded from for error messages.
@@ -560,32 +560,34 @@
     def load_module(self, fullname):
         """Load a plugin from a specific directory."""
         # We are called only for specific paths
-        plugin_dir = self.specific_paths[fullname]
-        candidate = None
-        maybe_package = False
-        for p in os.listdir(plugin_dir):
-            if os.path.isdir(osutils.pathjoin(plugin_dir, p)):
-                # We're searching for files only and don't want submodules to
-                # be recognized as plugins (they are submodules inside the
-                # plugin).
-                continue
-            name, path, (
-                suffix, mode, kind) = _find_plugin_module(plugin_dir, p)
-            if name is not None:
-                candidate = (name, path, suffix, mode, kind)
-                if kind == imp.PY_SOURCE:
-                    # We favour imp.PY_SOURCE (which will use the compiled
-                    # version if available) over imp.PY_COMPILED (which is used
-                    # only if the source is not available)
-                    break
-        if candidate is None:
+        plugin_path = self.specific_paths[fullname]
+        loading_path = None
+        package = False
+        if os.path.isdir(plugin_path):
+            for suffix, mode, kind in imp.get_suffixes():
+                if kind not in (imp.PY_SOURCE, imp.PY_COMPILED):
+                    # We don't recognize compiled modules (.so, .dll, etc)
+                    continue
+                init_path = osutils.pathjoin(plugin_path, '__init__' + suffix)
+                if os.path.isfile(init_path):
+                    loading_path = init_path
+                    package = True
+                    break
+        else:
+            for suffix, mode, kind in imp.get_suffixes():
+                if plugin_path.endswith(suffix):
+                    loading_path = plugin_path
+                    break
+        if loading_path is None:
             raise ImportError('%s cannot be loaded from %s'
-                              % (fullname, plugin_dir))
-        f = open(path, mode)
+                              % (fullname, plugin_path))
+        f = open(loading_path, mode)
         try:
-            mod = imp.load_module(fullname, f, path, (suffix, mode, kind))
-            # The plugin can contain modules, so be ready
-            mod.__path__ = [plugin_dir]
+            mod = imp.load_module(fullname, f, loading_path,
+                                  (suffix, mode, kind))
+            if package:
+                # The plugin can contain modules, so be ready
+                mod.__path__ = [plugin_path]
             mod.__package__ = fullname
             return mod
         finally:
 
=== modified file 'bzrlib/tests/test_plugins.py'
--- bzrlib/tests/test_plugins.py	2010-03-24 11:55:49 +0000
+++ bzrlib/tests/test_plugins.py	2010-04-06 07:24:38 +0000
@@ -810,21 +810,23 @@
         self.overrideAttr(plugin, '_loaded', False)
         # Create the same plugin in two directories
         self.create_plugin_package('test_foo', dir='non-standard-dir')
-        self.create_plugin_package('test_foo', dir='b/test_foo')
+        # The "normal" directory, we use 'standard' instead of 'plugins' to
+        # avoid depending on the precise naming.
+        self.create_plugin_package('test_foo', dir='standard/test_foo')
 
-    def assertTestFooLoadedFrom(self, dir):
+    def assertTestFooLoadedFrom(self, path):
         self.assertPluginKnown('test_foo')
         self.assertEqual('This is the doc for test_foo',
                          bzrlib.plugins.test_foo.__doc__)
-        self.assertEqual(dir, bzrlib.plugins.test_foo.dir_source)
+        self.assertEqual(path, bzrlib.plugins.test_foo.dir_source)
 
     def test_regular_load(self):
-        plugin.load_plugins(['b'])
-        self.assertTestFooLoadedFrom('b/test_foo')
+        plugin.load_plugins(['standard'])
+        self.assertTestFooLoadedFrom('standard/test_foo')
 
     def test_import(self):
         osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
-        plugin.set_plugins_path(['b'])
+        plugin.set_plugins_path(['standard'])
         try:
             import bzrlib.plugins.test_foo
         except ImportError:
@@ -833,12 +835,12 @@
 
     def test_loading(self):
         osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
-        plugin.load_plugins(['b'])
+        plugin.load_plugins(['standard'])
         self.assertTestFooLoadedFrom('non-standard-dir')
 
     def test_compiled_loaded(self):
         osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
-        plugin.load_plugins(['b'])
+        plugin.load_plugins(['standard'])
         self.assertTestFooLoadedFrom('non-standard-dir')
         self.assertEqual('non-standard-dir/__init__.py',
                          bzrlib.plugins.test_foo.__file__)
@@ -846,7 +848,7 @@
         # Try importing again now that the source has been compiled
         self._unregister_plugin('test_foo')
         plugin._loaded = False
-        plugin.load_plugins(['b'])
+        plugin.load_plugins(['standard'])
         self.assertTestFooLoadedFrom('non-standard-dir')
         if __debug__:
             suffix = 'pyc'
@@ -859,10 +861,35 @@
         # We create an additional directory under the one for test_foo
         self.create_plugin_package('test_bar', dir='non-standard-dir/test_bar')
         osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
-        plugin.set_plugins_path(['b'])
+        plugin.set_plugins_path(['standard'])
         import bzrlib.plugins.test_foo
         self.assertEqual('bzrlib.plugins.test_foo',
                          bzrlib.plugins.test_foo.__package__)
         import bzrlib.plugins.test_foo.test_bar
         self.assertEqual('non-standard-dir/test_bar/__init__.py',
                          bzrlib.plugins.test_foo.test_bar.__file__)
+
+    def test_loading_from___init__only(self):
+        # We rename the existing __init__.py file to ensure that we don't load
+        # a random file
+        init = 'non-standard-dir/__init__.py'
+        random = 'non-standard-dir/setup.py'
+        os.rename(init, random)
+        self.addCleanup(os.rename, random, init)
+        osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
+        plugin.load_plugins(['standard'])
+        self.assertPluginUnknown('test_foo')
+
+    def test_loading_from_specific_file(self):
+        plugin_dir = 'non-standard-dir'
+        plugin_file_name = 'iamtestfoo.py'
+        plugin_path = osutils.pathjoin(plugin_dir, plugin_file_name)
+        source = '''\
+"""This is the doc for %s"""
+dir_source = '%s'
+''' % ('test_foo', plugin_path)
+        self.create_plugin('test_foo', source=source,
+                           dir=plugin_dir, file_name=plugin_file_name)
+        osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@%s' % plugin_path)
+        plugin.load_plugins(['standard'])
+        self.assertTestFooLoadedFrom(plugin_path)