Merge lp:~dobey/ubuntu/natty/logilab-common/namespace-packages into lp:ubuntu/natty/logilab-common

[dobey]

[Evan Broder (broder)]

I'm having a hard time understanding this patch. It would be helpful if there was a bug report that explained what you're trying to fix with a bit more detail.

That being said, as best as I *can* understand what's going on, this patch seems like it regresses behavior. The try/except block you introduce is completely included within the while loop you remove (with some extra code in the else block that you don't replicate).

Furthermore, the patch that you linked to appears to have been reported against 0.22.0 of logilab-astng. The version in Natty is *much* newer than that. 0.22.0 was released with Hardy 2 years ago. If you look at logilab-common/modutils.py from back then (http://bit.ly/g2MP3x) the patch seems more like it would do something.

review: Disapprove

[Sebastien Bacher (seb128)]

setting to work in progress so it's out of the sponsoring list until set back to "needs review"

[Natalia Bidart (nataliabidart)]

Hi,

Currently, I'm running

python-logilab-common version 0.50.3-1ubuntu1~maverick1

When running pylint over a project that has several subprojects under the same namespace, such as ubuntuone, pylint doesn't find the imported modules that are located in the ubuntuone package but physically in a module in the PYTHONPATH but not in the current working directory.

What I said above, in an example, would be:

Package ubuntuone-client (installed in the system) provides ubuntuone.clientdefs module in this way:

ubuntuone/
    __init__.py
    clientdefs.py

Package ubuntuone-control-panel imports ubuntuone.clientdefs in http://bazaar.launchpad.net/%7Eubuntuone-control-tower/ubuntuone-control-panel/trunk/annotate/head%3A/ubuntuone/controlpanel/dbus_client.py#L27

The module ubuntuone.clientdefs if available at PYTHONPATH level as you can confirm with:
nessita@dali:~$ python
Python 2.6.6 (r266:84292, Sep 15 2010, 16:22:56)
[GCC 4.4.5] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import ubuntuone.clientdefs
>>>

But when running pylint over the ubuntuone.controlpanel.dbus_client module, we get tons of:

nessita@dali:~/canonical/u1/cp/trunk$ pylint ubuntuone/controlpanel/dbus_client.py
************* Module ubuntuone.controlpanel.dbus_client
F0401: 27: Unable to import 'ubuntuone.clientdefs'
E0611: 27: No name 'clientdefs' in module 'ubuntuone'
F0401: 28: Unable to import 'ubuntuone.platform.linux'
E0611: 28: No name 'platform' in module 'ubuntuone'
F0401: 29: Unable to import 'ubuntuone.platform.linux.tools'
E0611: 29: No name 'platform' in module 'ubuntuone'
nessita@dali:~/canonical/u1/cp/trunk$

And all the required modules are available in the PYTHONPATH. We tried workarounding this by setting the PYTHONPATH is several ways, but pylint keeps failing. The patch proposed in the logilab ticket fixes this issue and as far as I can tell it is not a regression behavior but an enhancement.

Right now, pylint does not work in a certain amount of cases like this one. By applying the patch, we're making that amount a bit smaller.

I wrote to the logilab list a few weeks ago asking how to solve this issue and the only reply I got was:

"""
The pb is that pylint doesn't understand that yet... Nor direct __path__
manipulation.
"""

They suggested trying to workaroung the issue using the pylint's --init-hook swicth but that would not do the trick.

Is very common to have this division of the same namespace being provided by python packages (zope does that as well), so from my point of view it would be a huge gain to have pylint working correctly in those cases.

[Evan Broder (broder)]

Ok, I do appreciate the explanation. That was very helpful in understanding the issue.

That being said, it would still be *more* helpful in the future if the issue was explained in a bug report instead of a merge proposal history, which tends to be rather ephemeral. And equally importantly, it's helpful to reference that bug both in your changelog entry and in the patch you're adding (using http://dep.debian.net/deps/dep3/) so that people in the future can figure out the bug. Additionally, if you feel you've addressed concerns from the sponsorship team, please change the "Status" at the very top of the merge proposal page from "Work in progress" back to "Needs reviewing" so that it shows back up in the sponsorship queue.

That's all important process stuff for once the patch is good. However, I still don't like this patch. In particular, I noticed that the new code your patch adds calls imp.find_module(os.sep.join(modpath), path), which you're not supposed to do. imp.find_module's docstring says:

  to search for a submodule of a package, pass the submodule name and the package's __path__.

The actual issue here is line 648 of modutils.py:

            path = [mp_filename]

Imports of submodules are handled by looking at the __path__ attribute of the package module (This is sort of indirectly discussed in PEP-382 <http://www.python.org/dev/peps/pep-0382/> which is a new way of handling multiple namespace packages in Python 3). For instance,

  >>> import email
  >>> email.__path__
  ['/usr/lib/python2.6/email']

So instead of setting path = [mp_filename] before going to the next loop iteration, you need to find the module itself (check out imp.load_module for doing that), grab its __path__ attribute, and set path to that. That's what the mailing list reply about direct __path__ manipulation is referring to.

[lp:~dobey/ubuntu/natty/logilab-common/namespace-packages]

19. By dobey on 2010-12-20

Add DEP-3 information and series file

Unmerged revisions

19. By dobey on 2010-12-20

Add DEP-3 information and series file

18. By dobey on 2010-12-15

* 01_namespace_packages.patch: Work around for setuptools namespace packages
  - Pulled from http://www.logilab.org/ticket/8796

=== modified file 'debian/changelog'
--- debian/changelog	2010-06-11 23:49:43 +0000
+++ debian/changelog	2010-12-20 18:54:08 +0000
@@ -1,3 +1,11 @@
+logilab-common (0.50.3-1ubuntu1) natty; urgency=low
+
+  * 01_namespace_packages.patch: Work around for setuptools namespace packages
+    - Pulled from http://www.logilab.org/ticket/8796
+    - Add DEP-3 information and series file
+
+ -- Rodney Dawes <[e-mail]>  Wed, 15 Dec 2010 17:08:42 -0500
+
 logilab-common (0.50.3-1) unstable; urgency=low
 
   * New upstream release

=== added directory 'debian/patches'
=== added file 'debian/patches/01_namespace_packages.patch'
--- debian/patches/01_namespace_packages.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/01_namespace_packages.patch	2010-12-20 18:54:08 +0000
@@ -0,0 +1,49 @@
+Description: Handle namespace packages
+Origin: http://www.logilab.org/ticket/8796
+Bug: http://www.logilab.org/ticket/8796
+--- logilab-common/modutils.py	2010-05-25 05:14:29.000000000 -0300
++++ logliab-common/modutils.py	2010-11-10 13:17:51.000000000 -0300
+@@ -617,35 +617,13 @@
+     except AttributeError:
+         checkeggs = False
+     imported = []
+-    while modpath:
+-        try:
+-            _, mp_filename, mp_desc = find_module(modpath[0], path)
+-        except ImportError:
+-            if checkeggs:
+-                return _search_zip(modpath, pic)[:2]
+-            raise
+-        else:
+-            if checkeggs:
+-                fullabspath = [abspath(x) for x in _path]
+-                try:
+-                    pathindex = fullabspath.index(dirname(abspath(mp_filename)))
+-                    emtype, emp_filename, zippath = _search_zip(modpath, pic)
+-                    if pathindex > _path.index(zippath):
+-                        # an egg takes priority
+-                        return emtype, emp_filename
+-                except ValueError:
+-                    # XXX not in _path
+-                    pass
+-                except ImportError:
+-                    pass
+-                checkeggs = False
+-        imported.append(modpath.pop(0))
+-        mtype = mp_desc[2]
+-        if modpath:
+-            if mtype != PKG_DIRECTORY:
+-                raise ImportError('No module %s in %s' % ('.'.join(modpath),
+-                                                          '.'.join(imported)))
+-            path = [mp_filename]
++    try:
++        _, mp_filename, mp_desc = find_module(os.sep.join(modpath), path)
++    except ImportError:
++        if checkeggs:
++            return _search_zip(modpath, pic)[:2]
++        raise
++    mtype = mp_desc[2]
+     return mtype, mp_filename
+ 
+ def _is_python_file(filename):
+

=== added file 'debian/patches/series'
--- debian/patches/series	1970-01-01 00:00:00 +0000
+++ debian/patches/series	2010-12-20 18:54:08 +0000
@@ -0,0 +1,2 @@
+01_namespace_packages.patch
+