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

Use sysconfig to determine stdlib paths for PyPy #1324

Merged
merged 5 commits into from Feb 1, 2022

Conversation

DanielNoord
Copy link
Collaborator

Steps

  • Write a good description on what the PR does.

Description

3/3 of PRs mentioned in #1322

This was a little more difficult to fix. See the structure of their repository here.

Basically on PyPy sysconfig.get_path is having trouble getting to the correct folders. It returns '{installed_base}/lib-{implementation_lower}' (this can be seen by passing expand=False). However, on PyPy the stdlib is spread over two different folders.

It finds the first folder with sysconfig.get_path. I believe this is because sys.prefix/lib_pypy is symlinked to sys.prefix/lib-pypy. Note that I think we can use only sys.prefix (the standard for sysconfig.get_path) as the issue with sys.prefix overwriting in virtualenvs is the same as mentioned in #1323 and seems to be fixed from my testing. This allows to remove the try...except block.

For the lib-python/3 folder we can't seem to get sysconfig.get_path to work. I don't think the function is intended to be able to resolve a stdlib spread over two different directories. I think there was something strange going on in distutils that made get_python_lib(standard_lib=True, prefix=sys.base_prefix) return the lib-python one, as sys.base_prefix is still ~/.pyenv/versions/pypy3.7-7.3.7. Not sure why distutils resolves this into the lib-python directory with get_python_lib but it worked... To fix this we can simply tell sysconfig that it should look for another implementation_lower and then it will find the correct directory.
Note that I have not added the 2.7 version as I think we don't support it?

I'm not sure what the order of review will be, but in the last PR of these three we should also add a changelog and remove all remaining imports. That doesn't necessarily need to be this PR though.

Type of Changes

Type
🔨 Refactoring

Related Issue

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining astroid or the dev workflow label Dec 31, 2021
@DanielNoord DanielNoord added this to the 2.10.0 milestone Dec 31, 2021
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.

We do not support python 2.7 anymore the last version supporting it is pylint 1.9.5 (I don't know for astroid).

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Left some comments. Although we might get away without adding lib_pypy, the safe thing to do would be to match all paths to the current implementation (even if some aren't strictly necessary). As a side note: It shouldn't be necessary to remove lib-pypy.

This is STD_LIB_DIRS on Github actions (for pypy):

{'/home/runner/work/astroid/astroid/venv/lib-python/3',
'/opt/hostedtoolcache/PyPy/3.6.12/x64/lib-python/3',
'/home/runner/work/astroid/astroid/venv/lib_pypy',
'/opt/hostedtoolcache/PyPy/3.6.12/x64/lib_pypy',
'/home/runner/work/astroid/astroid/venv/lib/python3.6',
'/home/runner/work/astroid/astroid/venv/lib64/python3.6'}

--
Regarding the merge order. I think it would make sense to do this one next and close with #1323 where the changelog entry could be added.

astroid/modutils.py Outdated Show resolved Hide resolved
astroid/modutils.py Outdated Show resolved Hide resolved
# sys.prefix/lib-pypy resolves into sys.prefix/lib_pypy
STD_LIB_DIRS.add(sysconfig.get_path("stdlib"))
STD_LIB_DIRS.add(
sysconfig.get_path("stdlib", vars={"implementation_lower": "python/3"})
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, but we should also do that for platstdlib.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are there actually any differences? The example in the docs doesn't show a difference:
https://docs.python.org/3/library/sysconfig.html
In the removed code we also don't seem to get platstdlib?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added!

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

LGTM! Let's do it then!

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 you @DanielNoord !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining astroid or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants