classification
Title: pyclbr.readmodule() and pyclbr.readmodule_ex() don't support namespace packages
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, eric.snow, ncoghlan, python-dev, vstinner
Priority: normal Keywords: patch

Created on 2016-03-15 22:56 by vstinner, last changed 2016-03-17 18:22 by eric.snow. This issue is now closed.

Files
File name Uploaded Description Edit
pyclbr.patch vstinner, 2016-03-15 22:56 review
Messages (12)
msg261832 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-15 22:56
While working on the issue #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.
msg261868 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-03-16 23:05
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'
msg261869 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-16 23:16
>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'
msg261876 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-03-17 00:48
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
msg261877 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-17 00:51
> 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.
msg261878 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-03-17 00:52
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
msg261879 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-03-17 00:56
Oh, and spec.loader for namespace package is set to None.
msg261880 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-03-17 01:01
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.
msg261900 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-03-17 08:12
New changeset 700ae1bfc453 by Victor Stinner in branch '3.5':
Fix pyclbr to support importing packages
https://hg.python.org/cpython/rev/700ae1bfc453
msg261901 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-17 08:15
> 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?
msg261925 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-03-17 18:22
Yeah, I've opened issue26584 to keep the matters separate.  We can address a more comprehensive cleanup there.
msg261926 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2016-03-17 18:22
And thanks for addressing this here. :)
History
Date User Action Args
2016-03-17 18:22:55eric.snowsetmessages: + msg261926
2016-03-17 18:22:24eric.snowsetstatus: open -> closed
resolution: fixed
messages: + msg261925

stage: commit review -> resolved
2016-03-17 08:15:39vstinnersetmessages: + msg261901
versions: + Python 3.5
2016-03-17 08:12:36python-devsetnosy: + python-dev
messages: + msg261900
2016-03-17 01:01:02eric.snowsettype: behavior
messages: + msg261880
components: + Interpreter Core
stage: commit review
2016-03-17 00:56:13eric.snowsetmessages: + msg261879
2016-03-17 00:52:54eric.snowsetmessages: + msg261878
2016-03-17 00:51:16vstinnersetmessages: + msg261877
2016-03-17 00:48:10eric.snowsetmessages: + msg261876
title: pyclbr.readmodule() and pyclbr.readmodule_ex() don't support packages -> pyclbr.readmodule() and pyclbr.readmodule_ex() don't support namespace packages
2016-03-16 23:16:55vstinnersetmessages: + msg261869
2016-03-16 23:05:51eric.snowsetnosy: + ncoghlan, eric.snow
messages: + msg261868
2016-03-15 22:56:47vstinnersettitle: pyclbr.readmodule() and pyclbr.readmodule_ex() doens't support packages -> pyclbr.readmodule() and pyclbr.readmodule_ex() don't support packages
2016-03-15 22:56:21vstinnercreate