getnikola / nikola

A static website and blog generator
https://getnikola.com/
MIT License
2.6k stars 444 forks source link

Handle change to plugin loading in recent yapsy (#3700) #3701

Closed AdamWill closed 1 year ago

AdamWill commented 1 year ago

https://github.com/tibonihoo/yapsy/pull/11 changes yapsy plugin loading to not use the deprecated imp module any more. However, as a side effect of that, it breaks this already-kinda-ugly hack, and we have to make it even uglier!

yapsy used to import the module like this:

imp.load_module(plugin_module_name,plugin_file...)

where plugin_module_name was the modified, "unique" name it creates early in loadPlugins. Interestingly, when you import a module like that, it gets added to sys.modules under both the modified name and its 'real' name, viz:

import sys import imp imp.load_module("someothername", None, "/usr/lib/python3.12/site-packages/yapsy/init.py", ("py", "r", imp.PKG_DIRECTORY)) <module 'someothername' from '/usr/lib/python3.12/site-packages/yapsy/init.py'> sys.modules["someothername"] <module 'someothername' from '/usr/lib/python3.12/site-packages/yapsy/init.py'> sys.modules["yapsy"] <module 'yapsy' from '/usr/lib/python3.12/site-packages/yapsy/init.py'>

That's why this hack worked. However, now yapsy imports the module using importlib, then adds it to sys.modules itself, only under the modified "unique" name, not under its original name. So sys.modules["unmodifiedpluginname"] is now a KeyError.

I can't think of a less ugly fix than this, unfortunately. We could try sending a patch for yapsy to add it under both the modified and unmodified names, but that would be somewhat tricky in yapsy's design, and I also suspect yapsy would consider it to actually be unwanted behavior.

Maybe what we really need is to send a patch for yapsy to just provide an interface to find a plugin's filesystem path...

Pull Request Checklist

Description

AdamWill commented 1 year ago

Revised to try two different possible 'unique' module names, since I found just doing one or the other wasn't enough in a real-world test with a real-world plugin...

AdamWill commented 1 year ago

I'm not actually sure which of the three possible names is 'most preferred' and which is 'least preferred', we can tweak it if anyone has ideas.

Kwpolska commented 1 year ago

Adding paths to yapsy would be preferable, but if we can still get the module ourselves in some way, that’s good enough.

AdamWill commented 1 year ago

I'll have a quick look at how viable it would be to add this to yapsy at some point.

AdamWill commented 1 year ago

Revised. I used mod_ not module_ because there is already mod_path and mod_dir.

Kwpolska commented 1 year ago

Thanks for fixing this! I’ll merge this, and if there’s any improvement in yapsy, let us know (or send another PR 😄)

AdamWill commented 1 year ago

that was quick! I think it may be possible to do this a 'better' way using yaspy's ability to set a callback for loadPlugins. I'm playing with that ATM. If it works out I'll send a follow-up PR.