Opened 9 years ago

Closed 8 years ago

#3419 defect closed fixed (fixed)

twisted.python.modules.walkModules fails completely if there is an unimportable package anywhere in the path

Reported by: Jean-Paul Calderone Owned by:
Priority: high Milestone:
Component: core Keywords:
Cc: jesstess, Thijs Triemstra Branch: branches/walkModulesNone-3419
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

exarkun@boson:/tmp/modules$ python
Python 2.5.2 (r252:60911, Jul 31 2008, 17:28:52) 
[GCC 4.2.3 (Ubuntu 4.2.3-2ubuntu7)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from sys import modules
>>> modules['OpenSSL'] = None
>>> from twisted.python.modules import walkModules
>>> list(walkModules())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/modules.py", line 715, in walkModules
    for module in package.walkModules(importPackages=False):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/modules.py", line 175, in walkModules
    for package in self.iterModules():
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/modules.py", line 127, in iterModules
    for placeToLook in self._packagePaths():
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/modules.py", line 420, in _packagePaths
    load = self.load()
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/modules.py", line 380, in load
    return self.pathEntry.pythonPath.moduleLoader(self.name)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/reflect.py", line 460, in namedAny
    raise ModuleNotFound("No module named %r" % (name,))
twisted.python.reflect.ModuleNotFound: No module named 'OpenSSL'
>>> 

Specifically, this makes the unit test for walkModules rather fragile. In trying to resolve #3111, it is not clear how to make this test pass.

Generally, it's kind of crummy for any one bad module in the path to completely disable this API.

Also, it's kind of crummy that the twisted.python.modules tests load such a huge quantity of Python code, since this makes them rather slow.

Change History (15)

comment:1 Changed 9 years ago by Jean-Paul Calderone

Type: enhancementdefect

comment:2 Changed 8 years ago by Glyph

Status: newassigned

comment:3 Changed 8 years ago by Glyph

Summary: twisted.python.modules.walkModules fails completely if there is an unimportable module anywhere in the pathtwisted.python.modules.walkModules fails completely if there is an unimportable package anywhere in the path

Updating the summary because it's actually only un-importable packages which cause problems.

Also, I tried to make a package with a syntactically invalid __init__.py and that didn't trigger the problem; it's something to do with the case which involves the presence of a None in sys.modules.

comment:4 Changed 8 years ago by Glyph

Keywords: review added
Owner: Glyph deleted
Status: assignednew

comment:5 Changed 8 years ago by Glyph

Author: glyph
Branch: branches/walkModulesNone-3419

(In [28722]) Branching to 'walkModulesNone-3419'

comment:6 Changed 8 years ago by jesstess

Owner: set to jesstess

comment:7 Changed 8 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: changed from jesstess to Glyph

test_modules.py

  • needs a copyright bump
  • There are some assertEqual in test_nonexistentPaths and test_nonDirectoryPaths that should be assertEquals
  • In the docstrings for the two new tests: "If a module has been blacklisted by setting a C{None} key in sys.modules" sounds like it means the same things as "If a package has been explicitly forbidden from importing by setting it to C{None} in sys.modules". If they do mean the same thing, sticking to one description would be clearer.
  • test_unimportablePackage says "attempting to walk over it should not load it". Does the existing assertion take care of checking if test_package was loaded, or does there need to be another assert?
  • os is imported but unused

modules.py

  • needs a copyright bump
  • in PythonPath.getitem, the modname parameter needs epytext markup
  • in PythonPath.getitem, the @raises statement looks like it's missing a negation.

builds look good.

comment:8 Changed 8 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

comment:9 Changed 8 years ago by Glyph

(In [28991]) Review feedback for test_modules.

  • copyright bump
  • assertEqual -> assertEquals
  • Fixed docstrings, logic, and names for the two unimportable-package tests to be consistent with each other, and make it clear that they're testing similar functionality of two different APIs.
  • added 'is loaded' assertion to test_unimportablePackageWalkModules
  • removed unused os import

Refs: #3419

comment:10 Changed 8 years ago by Glyph

(In [28992]) Review feedback for twisted.python.modules.

  • copyright bump
  • PythonPath.__getitem__: a better docstring, with
    • @param & @type for modname
    • @raise with a more helpful message (and corrected epytext syntax)
    • separated @return and @rtype

Refs: #3419

comment:11 Changed 8 years ago by Glyph

Owner: Glyph deleted

Build results

I think that should be it.

comment:12 Changed 8 years ago by Glyph

Keywords: review added

comment:13 Changed 8 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Glyph

Needs a news fragment. Otherwise looks good, please add that and merge.

comment:14 Changed 8 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [29007]) Merge walkModulesNone-3419

Author: glyph

Reviewer: exarkun, jesstess

Fixes: #3419

twisted.python.modules.walkModules will now handle packages explicitly precluded from importing by a None placed in sys.modules.

comment:15 Changed 7 years ago by <automation>

Owner: Glyph deleted
Note: See TracTickets for help on using tickets.