-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
setup.py: Remove use of "imp" module #652
Conversation
Python 3.12 removed the "imp" module, so replace its use in devlib.
Remove code that was used for Python 2 only.
Re-add the "future" PyPI package since it actually contains the "past" Python package that devlib still uses.
vh_path = os.path.join(devlib_dir, 'utils', 'version.py') | ||
# can load this, as it does not have any devlib imports | ||
version_helper = imp.load_source('version_helper', vh_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a completely naïve question without looking into actual implementation details.
I was wondering if there would be any benefit in using the example replacement provided in the Python release notes [1] both in devlib and referring to this from WA?
Otherwise I've tested the PR and this looks like it does the job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no. The snippet they provide is:
import importlib.util
import importlib.machinery
def load_source(modname, filename):
loader = importlib.machinery.SourceFileLoader(modname, filename)
spec = importlib.util.spec_from_file_location(modname, filename, loader=loader)
module = importlib.util.module_from_spec(spec)
# The module is always executed and not cached in sys.modules.
# Uncomment the following line to cache the module.
# sys.modules[module.__name__] = module
loader.exec_module(module)
return module
That is closer to how an import using the import
statement works, but it's not even fully accurate:
- Relative imports in the module won't work (unless you uncomment the sys.modules assignment, despite the comment only stating something about caching, it's actually "mandatory caching")
- Circular dependencies won't work (ditto)
return module
is actually wrong, it should usereturn sys.modules[module.__name__]
as some modules manipulatesys.modules[__name__]
and theimport
statement does honor that.
So all that to say that if you want a properly "compliant" way of doing it, it can be quite tricky, and on top of that these APIs have changed approximately every 5 minutes, meaning that it's unlikely this snippet will stay "the best way to do it" for more than a couple years (look for "deprecated" in https://docs.python.org/3/library/importlib.html).
OTH, the exec()
will always stay wrong and simple, and if something breaks it will be easy to understand why for anyone without being familiar with the intricacies of Python import system (e.g. no __name__
defined when running the module, it can just be added in the globals like I did for __file__
).
referring to this from WA?
If you mean the plugin import system in WA, I wouldn't do that. Plugins expect to be proper Python modules so they should be imported the proper way. For version_helper.py, we have full control on what the module does, and we can easily tailor its import to its content if needed, we don't have this luxury with 3rd party modules.
Python 3.12 removed the "imp" module, so replace its use in devlib.
closes #644