BZR_PLUGIN_PATH should ignore trailiing slashes

[bwinton]

While getting Bug #129298 to work, I set my BZR_PLUGIN_PATH to C:\Program Files\Bazaar\Plugins\, which lead to the following hard-to-interpret error:
V:\Personal\Snippets>bzr plugins
Unable to load plugin 'push_and_update' from 'C:\\Program Files\\Bazaar\\Plugins
\\'
Unable to load plugin 'bzrtools' from 'C:\\Program Files\\Bazaar\\Plugins\\'
C:\Program Files\Bazaar\lib\library.zip\bzrlib\plugins\launchpad
        Launchpad.net integration plugin for Bazaar
It's not immediately obvious to me what's wrong there.

It would be nice if bzr ignored trailing slashes in the BZR_PLUGIN_PATH environment variable. Or, if we wanted to enfore correctness, which I'm fine with, it would be nice to print out the full path to the plugin we weren't able to load. i.e.
V:\Personal\Snippets>bzr plugins
Unable to load plugin 'push_and_update' from 'C:\\Program Files\\Bazaar\\Plugins
\\\\push_and_update'
Unable to load plugin 'bzrtools' from 'C:\\Program Files\\Bazaar\\Plugins\\\\bzrtools'
C:\Program Files\Bazaar\lib\library.zip\bzrlib\plugins\launchpad
        Launchpad.net integration plugin for Bazaar
In that case, the quadrupled(!) slashes would have been a hint that I needed the remove the trailing slash from my environment variable.

[bwinton (bwinton+bzr)]

Here's a sample patch. I know it's not good enough to check in, but I don't really know how to write an automated test for it.
Anyways, with this patch, I can set each of the elements of my BZR_PLUGIN_PATH to end with any number of trailing slashes, and running "python bzr plugins" will always succeed.

[Martin Pool (mbp)]

Unfortunately there do not seem to be any specific tests for BZR_PLUGIN_PATH at the moment. We should add some. It would probably need to use run_bzr_subprocess() since the plugins are already loaded into the test process at this point.

The patch looks ok but I'd like to get to the bottom of why trailing slashes are a problem.

[bwinton (bwinton+bzr)]

I traced it down to line 183 raising an exception. (ImportError, I believe...) But there wasn't anything useful

lines 181-193 look like:
    for name in plugin_names:
        try:
            exec "import bzrlib.plugins.%s" % name in {}
        except KeyboardInterrupt:
            raise
        except Exception, e:
            ## import pdb; pdb.set_trace()
            if re.search('\.|-| ', name):
                warning('Unable to load plugin %r from %r: '
                    'It is not a valid python module name.' % (name, d))
            else:
                warning('Unable to load plugin %r from %r' % (name, d))
            log_exception_quietly()

I can get the same error by adding a trailing slash to one of the entries in sys.path, and trying to load a file from that entry.

Z:\>dir c:\progra~1\Python\lib\bdb*
 Volume in drive C has no label.
 Volume Serial Number is 204B-2F2E

 Directory of c:\progra~1\Python\lib

12/05/2006 02:17 PM 20,756 bdb.py
11/07/2007 10:52 AM 18,353 bdb.pyc
11/07/2007 10:52 AM 18,353 bdb.pyo
               3 File(s) 57,462 bytes
               0 Dir(s) 126,196,838,400 bytes free

Z:\>python
Python 2.5.1 (r251:54863, Apr 18 2007, 08:51:08) [MSC v.1310 32 bit (Intel)] on
win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path
['', 'C:\\WINDOWS\\system32\\python25.zip', 'c:\\progra~1\\Python\\DLLs', 'c:\\p
rogra~1\\Python\\lib', 'c:\\progra~1\\Python\\lib\\plat-win', 'c:\\progra~1\\Pyt
hon\\lib\\lib-tk', 'c:\\progra~1\\Python', 'c:\\progra~1\\Python\\lib\\site-pack
ages', 'c:\\progra~1\\Python\\lib\\site-packages\\PIL', 'c:\\progra~1\\Python\\l
ib\\site-packages\\win32', 'c:\\progra~1\\Python\\lib\\site-packages\\win32\\lib
', 'c:\\progra~1\\Python\\lib\\site-packages\\Pythonwin']
>>> sys.path = ['', 'C:\\WINDOWS\\system32\\python25.zip', 'c:\\progra~1\\Python
\\DLLs', 'c:\\progra~1\\Python\\lib\\', 'c:\\progra~1\\Python\\lib\\plat-win', '
c:\\progra~1\\Python\\lib\\lib-tk', 'c:\\progra~1\\Python', 'c:\\progra~1\\Pytho
n\\lib\\site-packages', 'c:\\progra~1\\Python\\lib\\site-packages\\PIL', 'c:\\pr
ogra~1\\Python\\ib\\site-packages\\win32', 'c:\\progra~1\\Python\\lib\\site-pack
ages\\win32\\lib', 'c:\\progra~1\\Python\\lib\\site-packages\\Pythonwin']
>>> sys.path
['', 'C:\\WINDOWS\\system32\\python25.zip', 'c:\\progra~1\\Python\\DLLs', 'c:\\p
rogra~1\\Python\\lib\\', 'c:\\progra~1\\Python\\lib\\plat-win', 'c:\\progra~1\\P
ython\\lib\\lib-tk', 'c:\\progra~1\\Python', 'c:\\progra~1\\Python\\lib\\site-pa
ckages', 'c:\\progra~1\\Python\\lib\\site-packages\\PIL', 'c:\\progra~1\\Python\
\ib\\site-packages\\win32', 'c:\\progra~1\\Python\\lib\\site-packages\\win32\\li
b', 'c:\\progra~1\\Python\\lib\\site-packages\\Pythonwin']
>>> import bdb
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named bdb
>>> import cgi
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named cgi

etc, etc...

http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/52662 suggests that you should call os.path.abspath and os.path.exists on the arguments befo...

[Robert Collins (lifeless)]

Hmm, there are explicit tests for plugin.load_from_path, which is what is failing.

See for instance bzrlib.tests.test_plugins.TestLoadingPlugins

we can either test that BZR_PLUGIN_PATH when split gets rid of trailing slashes, or we can test that load_from_path works given a path like ['foo/'].

No need for blackbox or subprocess voodoo.

[bwinton (bwinton+bzr)]

Okay, I took one of the tests in TestLoadingPlugins, and copied it, and it turned out that I needed to strip trailing slashes in two different places, so I did that, and then while I was there, I couldn't decide whether I should be testing plugin.load_from_path or set_plugins_path, and so I just wrote a test for each. It should all be in the attached patch.

I tried to follow the format of the code as best I could, please let me know if I missed something. Specifically, you might notice that one of my tests has a doc-comment, whereas the other has a doc-string. That's because the other tests in TestLoadingPlugins had doc-comments, whereas the other tests in TestSetPluginsPath had doc-strings...

And I suppose I should mention the reason why I needed to strip trailing slashes twice. I started off only stripping them in set_plugins_path, but load_from_path takes a list of directories, and manually sets plugins.__path__, so to test load_from_path, I needed it to also strip trailing slashes. I did a search through the code to see if anyone called set_plugins_path without passing the return value into load_from_path (so that I could just do the check in load_from_path), but there seemed to be such a call in bzrlib/__init__.py, so it looks like we need to strip the trailing slashes twice.

[Martin Pool (mbp)]

On 7/31/07, Robert Collins <email address hidden> wrote:
> Hmm, there are explicit tests for plugin.load_from_path, which is what
> is failing.
>
> See for instance bzrlib.tests.test_plugins.TestLoadingPlugins
>
> we can either test that BZR_PLUGIN_PATH when split gets rid of trailing
> slashes, or we can test that load_from_path works given a path like
> ['foo/'].
>
> No need for blackbox or subprocess voodoo.

At the moment there's no test that mentions BZR_PLUGIN_PATH at all. That
can't be right. I agree it can be tested separately from actually loading the
plugins, but there should be something that uses it.

I wouldn't call subprocess 'voodoo' - just a tool for a specific
situation.

--
Martin