Bug 286453 – [webkitpy] sys.modules[__name__] = __import__('twisted') breaks imports from within twisted

• Status: NEW
• Component: Tools / Tests

[Sam Sneddon [:gsnedders]]

We have a couple of places where we do things like this:

```
WebKit/Tools/Scripts/webkitpy/autoinstalled/buildbot.py
73:sys.modules[__name__] = __import__('buildbot')

WebKit/Tools/Scripts/webkitpy/autoinstalled/twisted.py
55:sys.modules[__name__] = __import__('twisted')
```

However, this breaks if you do something like:

```
from webkitpy.autoinstalled.twisted.internet import reactor
```

This ends up with us trying to load a spec:

```
(Pdb) pp spec
ModuleSpec(name='webkitpy.autoinstalled.twisted.internet.reactor', loader=<_frozen_importlib_external.SourceFileLoader object at 0x104ce9f50>, origin='/Volumes/gsnedders/projects/Safari/WebKit/Tools/Scripts/libraries/autoinstalled/python-3-arm64/twisted/internet/reactor.py')
(Pdb) pp parent_spec
ModuleSpec(name='webkitpy.autoinstalled.twisted.internet', loader=<_frozen_importlib_external.SourceFileLoader object at 0x104ce9050>, origin='/Volumes/gsnedders/projects/Safari/WebKit/Tools/Scripts/libraries/autoinstalled/python-3-arm64/twisted/internet/__init__.py', submodule_search_locations=['/Volumes/gsnedders/projects/Safari/WebKit/Tools/Scripts/libraries/autoinstalled/python-3-arm64/twisted/internet'])
```

Which ultimately results in:

```
Traceback (most recent call last):
  File "/opt/homebrew/Cellar/python@3.11/3.11.11/Frameworks/Python.framework/Versions/3.11/lib/python3.11/pdb.py", line 1777, in main
    pdb._run(target)
  File "/opt/homebrew/Cellar/python@3.11/3.11.11/Frameworks/Python.framework/Versions/3.11/lib/python3.11/pdb.py", line 1643, in _run
    self.run(target.code)
  File "/opt/homebrew/Cellar/python@3.11/3.11.11/Frameworks/Python.framework/Versions/3.11/lib/python3.11/bdb.py", line 600, in run
    exec(cmd, globals, locals)
  File "<string>", line 1, in <module>
  File "/Volumes/gsnedders/projects/Safari/WebKit/Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py", line 16, in <module>
    from webkitpy.autoinstalled.twisted.internet import reactor
  File "/Volumes/gsnedders/projects/Safari/WebKit/Tools/Scripts/libraries/autoinstalled/python-3-arm64/twisted/internet/reactor.py", line 37, in <module>
    del sys.modules["twisted.internet.reactor"]
        ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: 'twisted.internet.reactor'
```

…because we've ended up with `webkitpy.autoinstalled.twisted.internet.reactor` in `sys.modules`.

Twisted very much doesn't expect to end up in this state, because this very much isn't what the package is called.

I'm tempted to suggest that trying to alias stuff is fundamentally flawed, and almost certainly impossible to get right. We're gonna end up with multiple copies of each module, because otherwise their `__name__` and `__spec__` will be wrong, and in certain cases this will cause problems.

It's kinda problematic because we don't have a good way to detect when people are importing from `webkitpy.autoinstall.twisted.*`, short of installing our own `MetaPathFinder` and raising an exception from within `find_spec`.

This kinda makes me want to remove the `sys.modules[__name__] = __import__('twisted')` and similar, avoiding the entire problem here.

This is the fundamental cause of bug 270561, for the record, which changed some twisted imports to things like `from webkitpy.autoinstalled.twisted.internet import reactor` to ensure we'd run `webkitpy.autoinstalled.twisted`.

Maybe the real problem here is the existence of modules like `webkitpy.autoinstalled.twisted` in the first place — they exist to ensure all of Twisted's dependencies are already installed, and arguably we should solve this by just matching pip and guaranteeing installation order, to a limited extent: https://pip.pypa.io/en/stable/cli/pip_install/#installation-order:

> As of v6.1.0, pip installs dependencies before their dependents, i.e. in “topological order.” This is the only commitment pip currently makes related to order.

(I thought I'd filed another bug on that, but I can't find it.)

[Radar WebKit Bug Importer]

<rdar://problem/143536201>

[Sam Sneddon [:gsnedders]]

(In reply to Sam Sneddon [:gsnedders] from comment #0)
> I'm tempted to suggest that trying to alias stuff is fundamentally flawed,
> and almost certainly impossible to get right. We're gonna end up with
> multiple copies of each module, because otherwise their `__name__` and
> `__spec__` will be wrong, and in certain cases this will cause problems.
> 
> It's kinda problematic because we don't have a good way to detect when
> people are importing from `webkitpy.autoinstall.twisted.*`, short of
> installing our own `MetaPathFinder` and raising an exception from within
> `find_spec`.

The nested module case isn't the only one that's problematic; as soon as we have `webkitpy.autoinstaller.foobar` returning the same module as `foobar`, if `foobar.py` then does `del sys.modules['foobar']; sys.modules['foobar'] = __import__("something_else")` then we still have problems.

We could go down a path of writing a custom finder and loader combination which deal with this, but we're really looking at a lot of work to make this work correctly, and there will always be ways in which you can make this fail.

I'm increasingly inclined to say that for packages we don't want to unconditionally register (e.g., for when we have mutually exclusive requirements between different packages) we should just add something like a `webkitpy.autoinstaller.register` function, taking the name of the project to register, which simply registers it and its dependencies, returning nothing, and then it can be imported like normal. This also avoids the current oddity of `import webkitpy.autoinstaller.foobar` which you then don't need to ever touch, you simply need the side effect of the module being executed.