pyclbr.readmodule() and pyclbr.readmodule_ex() don't support namespace packages

[vstinner]

BPO	26569
Nosy	@brettcannon, @ncoghlan, @vstinner, @ericsnowcurrently
Files	pyclbr.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-03-17.18:22:24.445>
created_at = <Date 2016-03-15.22:56:21.372>
labels = ['interpreter-core', 'type-bug']
title = "pyclbr.readmodule() and pyclbr.readmodule_ex() don't support namespace packages"
updated_at = <Date 2016-03-17.18:22:55.728>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2016-03-17.18:22:55.728>
actor = 'eric.snow'
assignee = 'none'
closed = True
closed_date = <Date 2016-03-17.18:22:24.445>
closer = 'eric.snow'
components = ['Interpreter Core']
creation = <Date 2016-03-15.22:56:21.372>
creator = 'vstinner'
dependencies = []
files = ['42176']
hgrepos = []
issue_num = 26569
keywords = ['patch']
message_count = 12.0
messages = ['261832', '261868', '261869', '261876', '261877', '261878', '261879', '261880', '261900', '261901', '261925', '261926']
nosy_count = 5.0
nosy_names = ['brett.cannon', 'ncoghlan', 'vstinner', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue26569'
versions = ['Python 3.5', 'Python 3.6']

[vstinner]

While working on the issue bpo-26295 which converts the test module to a package, I noticed that pyclbr doesn't work with packages: test_pyclbr fails when test becomes a package.

Attached patch fixes pyclbr._readmodule():

• Replace "spec.loader.is_package(fullmodule)" test with "spec.submodule_search_locations is not None" to check is the module is a package
• for a package, use spec.submodule_search_locations to build __path__

I don't know importlib well enough to say if my usage of importlib spec is correct.

[ericsnowcurrently]

Hmm. These two should be equivalent:

  if spec.loader.is_package(fullmodule):
      dict['__path__'] = [os.path.dirname(fname)]

  if spec.submodule_search_locations is not None:
      dict['__path__'] = spec.submodule_search_locations

Do you mean that "python -m pyclbr XXX" doesn't work for packages? I'm guessing you mean something else because I was able to run it successfully for both "os" (sort of a package) and "importlib".

Also, I noticed that if I run pyclbr on the "test" package or "test.regrtest" I get back no output, not an error. I'd expect to get output though. Perhaps that's related?

If you are getting an error, what is the traceback you are getting?

Note that pyclbr fails with the following if you try to run it on a file or module that doesn't exist:

AttributeError: 'NoneType' object has no attribute 'loader'

[vstinner]

Hmm. These two should be equivalent:
if spec.loader.is_package(fullmodule): ...

The problem is that spec.loader is None when test is a package.

It's easy to reproduce the issue:

$ rm -f Lib/test/__init__.py Lib/test/__pycache__/__init__.cpython-36.pyc
$ ./python -m test -v -m test_decorators test_pyclbr
..

======================================================================
ERROR: test_decorators (test.test_pyclbr.PyclbrTest)
----------------------------------------------------------------------

Traceback (most recent call last):
  ...
  File "/home/haypo/prog/python/default/Lib/pyclbr.py", line 145, in _readmodule
    fname = spec.loader.get_filename(fullmodule)
AttributeError: 'NoneType' object has no attribute 'get_filename'

[ericsnowcurrently]

Ah, you're talking about deleting Lib/test/init.py. Doing so makes it a namespace package. The loader we use for namespace packages [1] does not have a get_filename() method. So the problem to solve is supporting namespace packages in Lib/pyclbr.py.

Regarding your patch, it's okay, but not the best option. Using spec.submodule_search_locations like you are isn't ideal, but works. However, you should be able to leave the is_package() call alone.

TBH, the better option is to use importlib.util.module_from_spec() instead, since it does the right thing for you, like setting __path__.

FWIW, I get the same as you by deleting those files and running the following:

./python Lib/pyclbr.py Lib/test

[1] https://hg.python.org/cpython/file/default/Lib/importlib/_bootstrap_external.py#l991

[vstinner]

However, you should be able to leave the is_package() call alone.

I don't really care how the issue is fixed. As I wrote, I don't know well the importlib module. Feel free to propose a better patch :-) My main concern is to fix test_pyclbr when test becomes a namespace.

[ericsnowcurrently]

Also, beside namespace packages, custom loaders may run into a similar problem with get_filename() and probably other code in there. It looks like the pyclbr module assumes that modules will be either builtin or loaded through SourceFileLoader. For example, you get a similar failure for frozen modules:

./python Lib/pyclbr.py _frozen_importlib

[ericsnowcurrently]

Oh, and spec.loader for namespace package is set to None.

[ericsnowcurrently]

Well, your patch is correct even if not ideal. :) Landing it as a short-term solution is fine. Just keep this issue open so we can address the problem more completely later.

[python-dev]

New changeset 700ae1bfc453 by Victor Stinner in branch '3.5':
Fix pyclbr to support importing packages
https://hg.python.org/cpython/rev/700ae1bfc453

[vstinner]

Well, your patch is correct even if not ideal. :) Landing it as a short-term solution is fine.

Ok, done. Sorry, I didn't write an unit test. I only tested manually by removing Lib/test/init.py.

Just keep this issue open so we can address the problem more completely later.

If it's a different problem, I prefer to open a new issue. But it's up to you. Can you please rephrase the title to put your expectations there? I understood that you also want to support frozen module?

[ericsnowcurrently]

Yeah, I've opened bpo-26584 to keep the matters separate. We can address a more comprehensive cleanup there.