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

Guard against missing setuptools #1100

Merged
merged 1 commit into from
Jul 19, 2021
Merged

Guard against missing setuptools #1100

merged 1 commit into from
Jul 19, 2021

Conversation

NeilGirdhar
Copy link
Contributor

Without the dependency on setuptools, poetry won't update setuptools when updating astroid. And if a user uninstalls setuptools, poetry won't reinstall it when updating. This causes the cryptic error "'pkg_resources' has no attribute '_namespace_packages' " when running pylint.

@Pierre-Sassoulas
Copy link
Member

Are you on the latest astroid ? We used scm_setuptools in very recent version but we removed it in #1030 If we still need setuptools after 1030, I'd rather do what's necessary to not depend on it.

@NeilGirdhar
Copy link
Contributor Author

@Pierre-Sassoulas I am on the latest. The trouble is this line in astroid/interpreter/_import/util.py:

return pkg_resources is not None and modname in pkg_resources._namespace_packages

I believe you can't access that without setuptools.

@Pierre-Sassoulas
Copy link
Member

The code should not fail though, it's guarded by a try catch:

try:
    import pkg_resources
except ImportError:
    pkg_resources = None


def is_namespace(modname):
    return pkg_resources is not None and modname in pkg_resources._namespace_packages

I don't know how it's possible that you had an error. Did you do something particular to get the error ? It also look like distutils is used in a lot of place, (too much to refactor easily), maybe that's what should be added (it's included in setuptools, I guess)

@NeilGirdhar
Copy link
Contributor Author

But that line doesn't guard against the missing setuptools?

Right now, I can't reproduce the error I got though. What I remember is that pylint was failing with that cryptic error, and then I installed setuptools and the error went away. I wish I could reproduce it.

@Pierre-Sassoulas
Copy link
Member

It's possible that it's worse than just a dependency to setuptools and that we're depending on a specific version of setuptools (pkg_resources._namespace_packages is private, so...). If we have the wrong version (the out-dated default version of the system ?) pkg_resources is not None and we get the error. To be frank I don't know what is the purpose of this code, I'd like to remove it if possible but this seems like a big retro-engineering and refactor (because of the distutils dependency).

@NeilGirdhar
Copy link
Contributor Author

NeilGirdhar commented Jul 18, 2021

It's possible that it's worse than just a dependency to setuptools and that we're depending on a specific version of setuptools (pkg_resources._namespace_packages is private, so...).

I actually tested this, and found that pylint fails with setuptools version 10, but it's mainly because setuptools 10 doesn't seem to work at all. Not sure if that's actually a problem.

To be frank I don't know what is the purpose of this code, I'd like to remove it if possible but this seems like a big retro-engineering and refactor (because of the distutils dependency).

Yes, that would be awesome if it's possible.

@NeilGirdhar
Copy link
Contributor Author

Maybe you could just add another guard?

return pkg_resources is not None and and hasattr(pkg_resources, '_namespace_packages') and modname in pkg_resources._namespace_packages

@Pierre-Sassoulas
Copy link
Member

Yeah you're right, let's do the fast fix and open another issue for the refactor. Do you want to add this to your MR ?

@NeilGirdhar
Copy link
Contributor Author

Okay I'll do that

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.6.3 milestone Jul 19, 2021
@NeilGirdhar NeilGirdhar changed the title Add dependency on setuptools Guard against missing setuptools Jul 19, 2021
@NeilGirdhar
Copy link
Contributor Author

Done

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank for the fix. I think we might still need to add setuptools to the requirements (because of distutils used elsewhere) and you should add yourself to the contributor. Maybe you could rebase this on your original changes if you still have the commit name ?

@NeilGirdhar
Copy link
Contributor Author

I overwrote it. It's just one extra line in your requirements, right? And I'll happily add my name to contributors.

@NeilGirdhar
Copy link
Contributor Author

Done, except I couldn't find the contributors list ?

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Ho nevermind for the contributors I'm confusing astroid with pylint where there is a Contributors.txt. git is tracking all this anyway :)

@Pierre-Sassoulas Pierre-Sassoulas merged commit 40629ba into pylint-dev:main Jul 19, 2021
@NeilGirdhar NeilGirdhar deleted the patch-1 branch July 19, 2021 19:01
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.

2 participants