Opened 6 years ago

Closed 4 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: exarkun Owned by:
Priority: high Milestone:
Component: core Keywords:
Cc: jessica.mckellar@…, thijs Branch: branches/walkModulesNone-3419
(diff, github, buildbot, log)
Author: glyph Launchpad Bug:

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 6 years ago by exarkun

  • Type changed from enhancement to defect

comment:2 Changed 4 years ago by glyph

  • Status changed from new to assigned

comment:3 Changed 4 years ago by glyph

  • Summary changed from twisted.python.modules.walkModules fails completely if there is an unimportable module anywhere in the path to twisted.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 4 years ago by glyph

  • Keywords review added
  • Owner glyph deleted
  • Status changed from assigned to new

comment:5 Changed 4 years ago by glyph

  • Author set to glyph
  • Branch set to branches/walkModulesNone-3419

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

comment:6 Changed 4 years ago by jesstess

  • Owner set to jesstess

comment:7 Changed 4 years ago by jesstess

  • Cc jessica.mckellar@… 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 4 years ago by thijs

  • Cc thijs added

comment:9 Changed 4 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 4 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 4 years ago by glyph

  • Owner glyph deleted

Build results

I think that should be it.

comment:12 Changed 4 years ago by glyph

  • Keywords review added

comment:13 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to glyph

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

comment:14 Changed 4 years ago by glyph

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by <automation>

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