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

fix deprecated pkg_resources #480

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

trim21
Copy link

@trim21 trim21 commented May 22, 2024

pkg_resources is deprecated.

Replace pkg_resources with importlib.resources, which available on python>=3.10.

https://setuptools.pypa.io/en/latest/pkg_resources.html

image

@trim21 trim21 marked this pull request as ready for review May 22, 2024 18:36
@hodgestar
Copy link
Contributor

@trim21 Many tests failed. Any idea what we can do to fix them?

@trim21
Copy link
Author

trim21 commented May 22, 2024

@trim21 Many tests failed. Any idea what we can do to fix them?

I'm looking into it.

@trim21
Copy link
Author

trim21 commented May 22, 2024

this works fine in my project but doesn't work in hpy testing, not sure why. looks like generated py file is imported in a very strange way.

@hodgestar
Copy link
Contributor

this works fine in my project but doesn't work in hpy testing, not sure why. looks like generated py file is imported in a very strange way.

Many of the errors seem to come from import hpymod; print(hpymod.__doc__) which doesn't seem that strange.

@trim21
Copy link
Author

trim21 commented May 22, 2024

this works fine in my project but doesn't work in hpy testing, not sure why. looks like generated py file is imported in a very strange way.

Many of the errors seem to come from import hpymod; print(hpymod.__doc__) which doesn't seem that strange.

some tests just import package with python -m mod so the assumption of __package__ is not empty failed.

Just fixed

@trim21
Copy link
Author

trim21 commented May 22, 2024

should works fine

@trim21
Copy link
Author

trim21 commented May 25, 2024

ci is still broken, I'll fix this.

@trim21
Copy link
Author

trim21 commented Jun 1, 2024

should works fine now, just test in my repo

image

@trim21 trim21 changed the title fix deprecated pkg_resources and support zipapp fix deprecated pkg_resources Jun 2, 2024
@agronholm
Copy link

Couldn't you just use the backport on Python < 3.10?

@trim21
Copy link
Author

trim21 commented Jun 3, 2024

Couldn't you just use the backport on Python < 3.10?

In that way developers(hpy users) will need to add extra pypi requiremenets to make it work, right?

@GalaxySnail
Copy link

In that way developers(hpy users) will need to add extra pypi requiremenets to make it work, right?

However pkg_resources (which is provided by setuptools) has already been an extra requirement. We can't assume setuptools is installed everywhere.

@agronholm
Copy link

Yeah, this is just swapping one requirement for another, and you should even make it conditionally installed only on Python < 3.10.

@trim21
Copy link
Author

trim21 commented Jun 3, 2024

In that way developers(hpy users) will need to add extra pypi requiremenets to make it work, right?

However pkg_resources (which is provided by setuptools) has already been an extra requirement. We can't assume setuptools is installed everywhere.

make sense

@trim21
Copy link
Author

trim21 commented Jun 3, 2024

I'm not very familiar with hpy itself, where should I add importlib_resources requirements to make tests pass?

@agronholm
Copy link

I'm not very familiar with hpy itself, where should I add importlib_resources requirements to make tests pass?

Presumably in setup.py where it has a requirement on setuptools?

@@ -264,6 +264,9 @@ def build_libraries(self, libraries):
cmdclass={"build_clib": build_clib_hpy},
use_scm_version=get_scm_config,
setup_requires=['setuptools_scm'],
install_requires=['setuptools>=64.0'],
install_requires=[
'setuptools>=64.0',

Choose a reason for hiding this comment

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

Does hpy actually still need setuptools at run time after this?

Copy link
Author

Choose a reason for hiding this comment

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

I have no idea, should I remove this?

Choose a reason for hiding this comment

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

Does the code import either setuptools or pkg_resources anywhere in the code base after your changes? If not, you can remove the dependency.

Choose a reason for hiding this comment

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

Frankly setuptools should never be a run-time dependency of any project. Its only legitimate use at the moment is as a build back-end.

Copy link
Author

Choose a reason for hiding this comment

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

looks like hpy.devel still require setuptools

Choose a reason for hiding this comment

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

Is that a separate project?

Copy link
Author

@trim21 trim21 Jun 3, 2024

Choose a reason for hiding this comment

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

actually I have no idea what dependency should I have if I use hpy in universal mode........

it require hpy.universal to load dll, so it looks like I should install hpy.universal, and
indeed there is a hpy.universal package in pypi. but it doesn't look right, so I need to include whole hpy, including devel, trace and so on as runtime dependency for my package using hpy in universal mode?

Choose a reason for hiding this comment

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

Is hpy.universal a separate project or is it somehow packaged separately from this project?

Choose a reason for hiding this comment

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

Anyway, if setuptools is directly imported by this project, it should be kept as a run-time dependency. At best it could be moved to extras if it's only used in some edge cases, provided that it's properly documented that way.

Copy link
Author

Choose a reason for hiding this comment

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

Is hpy.universal a separate project or is it somehow packaged separately from this project?

I don't know 🧐, didn't even successfully pack my hpy universal mode wheel yet.

setup.py Outdated Show resolved Hide resolved
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.

5 participants