replacing pkg_resources with importlib #3749

fondbcn · 2024-01-24T11:07:41Z

Pull Request Checklist

I’ve read the guidelines for contributing.
I updated AUTHORS.txt and CHANGES.txt (if the change is non-trivial) and documentation (if applicable).
I tested my changes.

Description

Sorry for the old error.
I updated the file with this PR instead.

Kwpolska · 2024-01-24T20:02:54Z

There are some more places where pkg_resources is used, could you fix them all?

felixfontein · 2024-01-24T20:15:11Z

nikola/nikola.py

@@ -1031,7 +1031,7 @@ def init_plugins(self, commands_only=False, load_all=False):
        extra_plugins_dirs = self.config['EXTRA_PLUGINS_DIRS']
        self._loading_commands_only = commands_only
        self._plugin_places = [
-            resource_filename('nikola', 'plugins'),
+            resources.files('nikola').joinpath('plugins'),


resources.files() has been added in Python 3.9 (https://docs.python.org/3/library/importlib.resources.html#importlib.resources.files), while this repo still supports Python 3.8 (https://github.com/getnikola/nikola/blob/master/setup.py#L142).

resources.files() has been added in Python 3.9 (https://docs.python.org/3/library/importlib.resources.html#importlib.resources.files), while this repo still supports Python 3.8 (https://github.com/getnikola/nikola/blob/master/setup.py#L142).

should we use importlib_resources instead of importlib.resources ?

No. Please use the built-in importlib.resources, but only the functions available in Python ~~3.7~~ 3.8: https://docs.python.org/3.8/library/importlib.html#module-importlib.resources

No. Please use the built-in importlib.resources, but only the functions available in Python ~~3.7~~ 3.8: https://docs.python.org/3.8/library/importlib.html#module-importlib.resources

I can't, with importlib.resources you can choose only between deprecated or newer methods! (all the available methods in 3.8 are not working with newer python releases)

You can always make code depend on sys.version_info.

fondbcn · 2024-01-24T20:48:34Z

There are some more places where pkg_resources is used, could you fix them all?

not in nikola.py ?

Kwpolska · 2024-01-24T20:50:27Z

There are some more places where pkg_resources is used, could you fix them all?
not in nikola.py ?

Yes, there are 6 other files.

fondbcn · 2024-01-24T20:58:12Z

There are some more places where pkg_resources is used, could you fix them all?
not in nikola.py ?

Yes, there are 6 other files.

how to deal with python 3.8 as felixfontein said ?

felixfontein · 2024-01-24T21:03:07Z

I would probably create a helper function (in utils.py maybe?) which uses the functions available in Python 3.8. The next step would be to make this function depend on the Python version, since Python 3.11 basically deprecated everything in importlib.resources that was available in Python 3.8...

felixfontein · 2024-01-24T21:07:16Z

Another alternative might be using https://docs.python.org/3/library/pkgutil.html#pkgutil.get_data instead.

fondbcn · 2024-01-25T07:44:26Z

Another alternative might be using https://docs.python.org/3/library/pkgutil.html#pkgutil.get_data instead.

will "pkgutil" be widely compatible ?

felixfontein · 2024-01-26T06:44:42Z

According to the docs it's been available since at least Python 2.6.

fondbcn · 2024-01-26T09:54:53Z

hoping this works.
strangely some files require str() !

Kwpolska · 2024-01-26T21:03:35Z

nikola/nikola.py

@@ -1031,7 +1031,7 @@ def init_plugins(self, commands_only=False, load_all=False):
        extra_plugins_dirs = self.config['EXTRA_PLUGINS_DIRS']
        self._loading_commands_only = commands_only
        self._plugin_places = [
-            resource_filename('nikola', 'plugins'),
+            str(resources.path('nikola', 'plugins')) if sys.version_info.minor == 8 else str(resources.files('nikola').joinpath('plugins')),


Instead of repeating the same version check everywhere, please instead make a helper function in utils.py.

Please make it so your version check won’t break for Python 4.8, and consider making it <= instead of == while doing that.

What do the functions return so that they need str? If it’s a pathlib.Path, it would be desirable to switch other things to Paths instead of working with strings.

Kwpolska · 2024-01-26T21:05:04Z

nikola/nikola.py

-        builtin_sc_dir = resource_filename(
-            'nikola',
-            os.path.join('data', 'shortcodes', utils.get_template_engine(self.THEMES)))
+        builtin_sc_dir = str(resources.path('nikola')) if sys.version_info.minor == 8 else str(resources.files('nikola').joinpath(os.path.join('data', 'shortcodes', utils.get_template_engine(self.THEMES))))


The two things are not equivalent, this is why a function in utils would be a better choice.

Kwpolska · 2024-01-27T12:30:52Z

Please fix the failing tests and linting errors.

fondbcn · 2024-01-27T19:42:12Z

Please fix the failing tests and linting errors.

I have no idea why resources.path() is not working!

felixfontein

Looks like I forgot to send this review. Here it is.

nikola/utils.py

Kwpolska · 2024-01-28T20:52:26Z

To understand the 3.8 test failures, you can try running the tests locally, but first changing the code so that the 3.8 version runs on all Python versions (or you could install 3.8, but it’s probably not worth it).
The test failures in 3.9+ are unrelated to your code (#3753).

fondbcn · 2024-01-29T10:16:43Z

To understand the 3.8 test failures, you can try running the tests locally, but first changing the code so that the 3.8 version runs on all Python versions (or you could install 3.8, but it’s probably not worth it). The test failures in 3.9+ are unrelated to your code (#3753).

I tried every method in https://docs.python.org/3.8/library/importlib.html#module-importlib.resources , but nothing is working on 3.8 !
how newer files().joinpath() avoid this issue ???

felixfontein · 2024-01-29T17:20:37Z

The documentation of importlib.resources.path() says:

Return the path to the resource as an actual file system path. This function returns a context manager for use in a with statement. The context manager provides a pathlib.Path object.

(Emphasis is mine.)

So it returns a context manager, not directly a path. Your code is treating it as a function which returns a path. That doesn't work. Instead of return str(resources.path(package, resource)), you have to do

        with resources.path(package, resource) as path:
            return str(path)

This only works for resources that are not part of an archive. For this to work with resources in an archive, you need to implement a caching solution. Which might be easier if not all uses of resource_filename would be routed through the same utils function call.

I guess one question is how much complexity we want to add for Python 3.8. @Kwpolska what do you think?

Kwpolska · 2024-01-29T18:03:11Z

This only works for resources that are not part of an archive. For this to work with resources in an archive, you need to implement a caching solution. Which might be easier if not all uses of resource_filename would be routed through the same utils function call.

What’s an archive? We don’t support .egg or zipfile.

If the with statement is enough to fix 3.8, we should do that. If it isn’t, we can use pkg_resources or a backport of importlib_resources on 3.8 (but only 3.8, with a conditional requirement)

felixfontein · 2024-01-29T19:00:22Z

If we don't support zipfiles or directly running from eggs, then yeah, just doing a return inside the with should be fine.

fondbcn · 2024-01-29T19:36:26Z

If we don't support zipfiles or directly running from eggs, then yeah, just doing a return inside the with should be fine.

I tried the "with" on path() but the test is still failing.
The documentation of importlib.resources (3.9+) says that path() is equivalent to as_file(files().joinpath()), however read() also didn't work.

fondbcn · 2024-01-30T12:31:20Z

I found a solution that works for all 3.8 and 3.9+ (at least for me), just adding some init.py in data/ and removing os.path.join .
However the test time is longer.

nikola/data/__init__.py

nikola/data/samplesite/files/__init__.py

nikola/utils.py

nikola/nikola.py

Kwpolska · 2024-02-01T19:58:53Z

Please fix the test failures.

Co-authored-by: Felix Fontein <[email protected]>

Co-authored-by: Chris Warrick <[email protected]>

Kwpolska · 2024-02-01T22:18:00Z

Thanks for your contribution!

felixfontein reviewed Jan 24, 2024

View reviewed changes

Kwpolska reviewed Jan 26, 2024

View reviewed changes

felixfontein reviewed Jan 27, 2024

View reviewed changes

nikola/utils.py Outdated Show resolved Hide resolved

nikola/utils.py Outdated Show resolved Hide resolved

fondbcn force-pushed the master branch from fd8dfff to 6431f31 Compare January 30, 2024 12:28

Kwpolska requested changes Jan 30, 2024

View reviewed changes

nikola/data/__init__.py Outdated Show resolved Hide resolved

Kwpolska reviewed Jan 31, 2024

View reviewed changes

nikola/data/samplesite/files/__init__.py Outdated Show resolved Hide resolved

nikola/utils.py Outdated Show resolved Hide resolved

nikola/utils.py Outdated Show resolved Hide resolved

Kwpolska reviewed Jan 31, 2024

View reviewed changes

nikola/nikola.py Outdated Show resolved Hide resolved

Kwpolska linked an issue Feb 1, 2024 that may be closed by this pull request

Replace use of pkg_resources with importlib.resources #3743

Closed

3 tasks

Kwpolska approved these changes Feb 1, 2024

View reviewed changes

fondbcn added 2 commits February 1, 2024 23:12

replacing pkg_resources with importlib

3d73252

replacing all pkg_resources with importlib

6f5cb46

fondbcn and others added 10 commits February 1, 2024 23:12

resolving deprecated functions with utils.py

2f9a06f

applying str to paths

5aa00d8

solution of pkg resources for 3.8 and newer

c107acd

Update nikola/utils.py

470c89f

Co-authored-by: Felix Fontein <[email protected]>

removing type hint

3a96327

alternative solution for pkg

74346d3

Update nikola/utils.py

71c8a69

Co-authored-by: Chris Warrick <[email protected]>

Update nikola/nikola.py

4afe5b8

Co-authored-by: Chris Warrick <[email protected]>

final touches

100e958

Update utils.py

a2af6d5

Kwpolska force-pushed the master branch from d7a0a4f to a2af6d5 Compare February 1, 2024 22:12

Kwpolska enabled auto-merge (squash) February 1, 2024 22:12

Kwpolska merged commit 4541149 into getnikola:master Feb 1, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replacing pkg_resources with importlib #3749

replacing pkg_resources with importlib #3749

fondbcn commented Jan 24, 2024

Kwpolska commented Jan 24, 2024

felixfontein Jan 24, 2024

fondbcn Jan 24, 2024

Kwpolska Jan 24, 2024 •

edited

Loading

fondbcn Jan 25, 2024

felixfontein Jan 26, 2024 •

edited

Loading

fondbcn commented Jan 24, 2024

Kwpolska commented Jan 24, 2024

fondbcn commented Jan 24, 2024

felixfontein commented Jan 24, 2024

felixfontein commented Jan 24, 2024

fondbcn commented Jan 25, 2024

felixfontein commented Jan 26, 2024

fondbcn commented Jan 26, 2024

Kwpolska Jan 26, 2024

Kwpolska Jan 26, 2024

Kwpolska commented Jan 27, 2024

fondbcn commented Jan 27, 2024

felixfontein left a comment

Kwpolska commented Jan 28, 2024

fondbcn commented Jan 29, 2024

felixfontein commented Jan 29, 2024

Kwpolska commented Jan 29, 2024

felixfontein commented Jan 29, 2024

fondbcn commented Jan 29, 2024

fondbcn commented Jan 30, 2024

Kwpolska commented Feb 1, 2024

Kwpolska commented Feb 1, 2024

replacing pkg_resources with importlib #3749

replacing pkg_resources with importlib #3749

Conversation

fondbcn commented Jan 24, 2024

Pull Request Checklist

Description

Kwpolska commented Jan 24, 2024

felixfontein Jan 24, 2024

Choose a reason for hiding this comment

fondbcn Jan 24, 2024

Choose a reason for hiding this comment

Kwpolska Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

fondbcn Jan 25, 2024

Choose a reason for hiding this comment

felixfontein Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

fondbcn commented Jan 24, 2024

Kwpolska commented Jan 24, 2024

fondbcn commented Jan 24, 2024

felixfontein commented Jan 24, 2024

felixfontein commented Jan 24, 2024

fondbcn commented Jan 25, 2024

felixfontein commented Jan 26, 2024

fondbcn commented Jan 26, 2024

Kwpolska Jan 26, 2024

Choose a reason for hiding this comment

Kwpolska Jan 26, 2024

Choose a reason for hiding this comment

Kwpolska commented Jan 27, 2024

fondbcn commented Jan 27, 2024

felixfontein left a comment

Choose a reason for hiding this comment

Kwpolska commented Jan 28, 2024

fondbcn commented Jan 29, 2024

felixfontein commented Jan 29, 2024

Kwpolska commented Jan 29, 2024

felixfontein commented Jan 29, 2024

fondbcn commented Jan 29, 2024

fondbcn commented Jan 30, 2024

Kwpolska commented Feb 1, 2024

Kwpolska commented Feb 1, 2024

Kwpolska Jan 24, 2024 •

edited

Loading

felixfontein Jan 26, 2024 •

edited

Loading