Skip to content
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

Replace Deprecated API's #11

Merged
merged 3 commits into from
Mar 25, 2023

Conversation

AmeyaVS
Copy link
Contributor

@AmeyaVS AmeyaVS commented Feb 6, 2023

  • Replaced packaging.version instead of distutils.version
  • Replaced imp module to importlib

Note: Probably Deprecates Python 2.7 supports, and maybe some initial versions of Python 3.x.

* Replaced packaging.version instead of distutils.version
* Replaced imp module to importlib

**Note:** Probably Deprecates Python 2.7 supports, and maybe some initial versions of Python 3.x.

Signed-off-by: Ameya Vikram Singh <ameya.v.singh@gmail.com>
@AmeyaVS AmeyaVS changed the title Remove Deprecated API's Replace Deprecated API's Feb 6, 2023
@AmeyaVS
Copy link
Contributor Author

AmeyaVS commented Feb 7, 2023

This PR actually fixes the deprecation fix tried earlier in the last PR #10.
Currently the imp module is still being used to import at runtime.

@AmeyaVS
Copy link
Contributor Author

AmeyaVS commented Mar 17, 2023

Ping, any comments on the changes?

Comment on lines 577 to 587
candidate_module = None
if os.path.isdir(candidate_filepath):
candidate_module = imp.load_module(plugin_module_name,None,candidate_filepath,("py","r",imp.PKG_DIRECTORY))
if (spec := importlib.util.spec_from_file_location(candidate_filepath.split('/')[-1], candidate_filepath + "/__init__.py")) is not None:
candidate_module = importlib.util.module_from_spec(spec)
sys.modules[plugin_module_name] = candidate_module
spec.loader.exec_module(candidate_module)
else:
with open(candidate_filepath+".py","r") as plugin_file:
candidate_module = imp.load_module(plugin_module_name,plugin_file,candidate_filepath+".py",("py","r",imp.PY_SOURCE))
if (spec := importlib.util.spec_from_file_location(candidate_filepath.split('/')[-1], candidate_filepath + ".py")) is not None:
candidate_module = importlib.util.module_from_spec(spec)
sys.modules[plugin_module_name] = candidate_module
spec.loader.exec_module(candidate_module)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legibility suggestion: this would be easier to understand and offer a cleaner traceback (if/upon error) if refactored into separate path munging and conditional spec assignment/exec. Does this seem logically equivalent?

candidate_module = None
filepath_base = candidate_filepath.split('/')[-1]
if os.path.isdir(candidate_filepath):
    location = candidate_filepath + '/__init__.py'
else:
    location = candidate_filepath + '.py'

if (spec := importlib.util.spec_from_file_location(filepath_base, location)):
    candidate_module = importlib.util.module_from_spec(spec)
    sys.modules[plugin_module_name] = candidate_module
    spec.loader.exec_module(candidate_module)

return candidate_module

Copy link
Contributor Author

@AmeyaVS AmeyaVS Mar 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, updated the PR with the suggestions.

@AmeyaVS
Copy link
Contributor Author

AmeyaVS commented Mar 24, 2023

Is there anything else I need to take care of e.g.: Updating the versioning details?
Breaking Python2 Compatibility.

@tibonihoo tibonihoo merged commit b7f8e8e into tibonihoo:master Mar 25, 2023
@tibonihoo
Copy link
Owner

Hello !
Thanks a lot for the pull request.

Nothing is missing in it but since this is definitely breaking the pyhon2 compatibility, I had to push a few commits to make the transition a bit more explicit and clean before merging the breaking change.

@AmeyaVS AmeyaVS deleted the fix/remove_deprecated_apis branch March 27, 2023 07:44
AdamWill added a commit to AdamWill/nikola that referenced this pull request Jul 14, 2023
tibonihoo/yapsy#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...

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/nikola that referenced this pull request Jul 14, 2023
tibonihoo/yapsy#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...

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/nikola that referenced this pull request Jul 14, 2023
tibonihoo/yapsy#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...

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/nikola that referenced this pull request Jul 15, 2023
tibonihoo/yapsy#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...

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/nikola that referenced this pull request Jul 15, 2023
tibonihoo/yapsy#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...

Signed-off-by: Adam Williamson <awilliam@redhat.com>
Kwpolska pushed a commit to getnikola/nikola that referenced this pull request Jul 15, 2023
tibonihoo/yapsy#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...

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@tdenewiler tdenewiler mentioned this pull request Oct 4, 2023
@tillea tillea mentioned this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants