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

Implement sysconfig-based location inference #9626

Merged
merged 18 commits into from
Feb 28, 2021
Merged

Conversation

uranusjr
Copy link
Member

This detects location differences between distutils and sysconfig, and tell users to comment in #9617.

@pypa-bot
Copy link

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@pypa-bot pypa-bot added the needs rebase or merge PR has conflicts with current master label Feb 19, 2021
@uranusjr uranusjr force-pushed the sysconfig branch 2 times, most recently from 755de34 to 9da8b15 Compare February 19, 2021 10:00
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 19, 2021
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 20, 2021
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 22, 2021
This also helps catch bugs in the logic, which are also fixed in this
commit.
@uranusjr
Copy link
Member Author

uranusjr commented Feb 23, 2021

Okay, I think I’ve caught all the pip-specific edge cases. The only remaining failures are all PyPy. Its purelib and platlib values differ:

distutils: {sys.prefix}/site-packages
sysconfig: {sys.prefix}/lib/pypy3.6/site-packages

and this is arguably exactly the kind of mismatches we want to catch and help implementations fix. Who should we reach out to discuss this with PyPy folks?

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

This is really great and looks good to me. The only thought I have is we should probably add more "proper unit tests" for the newly introduced sysconfig-related code, instead of only patching the existing ones.

But, eh.

@pradyunsg
Copy link
Member

pradyunsg commented Feb 23, 2021

/cc @mattip for help with PyPy's sysconfig vs distutils.sysconfig stuff. :)

@mattip
Copy link
Contributor

mattip commented Feb 23, 2021

Thanks for pinging me. Please test the latest PyPy3.6 and 3.7, which is available on github actions via the actions/setup-python@v2 action, this will also help you move yet another few builds off travis.

As for distutils.sysconfig vs. sysconfig: where is there an _INSTALL_SCHEME in distutils.sysconfig? PyPy intentionally sets up site-packages to be in a different location than CPython, to prevent confusion, so the _INSTALL_SCHEME in sysconfig is changed on purpose. Could you help me understand what is actually failing in the tests?

@uranusjr
Copy link
Member Author

uranusjr commented Feb 23, 2021

The distutils-based implementation uses distutils.command.install.INSTALL_SCHEME to get the site-packages directory.

I checked PyPy’s implementation, it seems like the mismatch happens because our sysconfig implementation does not use the pypy key in sysconfig._INSTALL_SCHEMES, but posix_home when the “home” scheme is requested. Should we use pypy here instead?


This also seems to surface another issue in sysconfig: There is no way or a tools to ask the interpreter what are their preferred scheme names. We have _get_default_scheme() for the “prefix” schemes, but no equivalents for “user” and “home”, so pip can only guess the scheme names when it needs “the current platform/implementation’s user scheme name” for pip install --user (and likewise for “home” on --target), and implementations like PyPy has no way to communicate to tools messages like “please use pypy instead of posix_home”. This should probably be raised in Discourse for CPython core devs to consider.

@mattip
Copy link
Contributor

mattip commented Feb 23, 2021

Hmm. It seems there are a few problems. One is that PyPy never bothered to sync up distutils.command.install with the changes it made to sysconfig. Another is the difference in schemes, that it never backported to CPython. But +- bugs, there should be no need for pip to adjust posix_name or posix_user, this should be provided by the sysconfig implementation, that each implementation can modify to fit its needs.

If we want a single source for sysconfig across implementations, I think the best course would be to modify sysconfig's scheme layout and add a implementation key, that could get its value from platform.python_implementation(). This would look something like what PyPy did. Note that this also impacts site.py since they copy out parts of sysconfig.

@mattip
Copy link
Contributor

mattip commented Feb 23, 2021

We have _get_default_scheme() for the “prefix” schemes ...

It seems that is a private interface, the public one is sysconfig.get_path()

@mattip
Copy link
Contributor

mattip commented Feb 23, 2021

Yeah, this is all very emperical work, and on top of what the implementations do, the OS distributors do their thing too. Anyhow, thanks for pointing out the distutils install command, I have already made a stab at fixing that in PyPy. Could you point me to the tests that discovered the discrepancy?

@pradyunsg
Copy link
Member

I mean, part of why I want distutils out of the standard library is this stuff, but I’m glad that @uranusjr filed this PR, because this has always felt like a field of thorns - it touches legacy defacto stuff + new stuff and alternate Python implementations. :)

@uranusjr
Copy link
Member Author

I’ve raised this in bpo-43312.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@pradyunsg
Copy link
Member

@uranusjr Let's get this up to date, mark the tests as xfail(strict=True) on PyPy, and merge? The longer we let this linger, the slower our progress would be here. :)

@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 27, 2021
@uranusjr
Copy link
Member Author

Let me try fixing PyPy instead. I’ll do the xfails if this does not work…

@uranusjr uranusjr force-pushed the sysconfig branch 2 times, most recently from 78fcbeb to a51faf4 Compare February 27, 2021 23:56
@uranusjr
Copy link
Member Author

🔔 THIS WORKS 🔔

@pradyunsg pradyunsg merged commit f4f664b into pypa:master Feb 28, 2021
@pradyunsg
Copy link
Member

AHAHAHAHA. Let's do this! :)

@pradyunsg pradyunsg added the type: maintenance Related to Development and Maintenance Processes label Feb 28, 2021
@uranusjr uranusjr deleted the sysconfig branch February 28, 2021 08:28
@pradyunsg
Copy link
Member

FWIW, I'm seeing that this fails with GitHub's pypy3:

https://github.com/pradyunsg/pip/pull/9/checks?check_run_id=2250267986#step:6:4862

@mattip
Copy link
Contributor

mattip commented Apr 2, 2021

I don't see any failures there. Could you copy-paste the failing test or otherwise point out what is failing on pypy3 ?

@pradyunsg
Copy link
Member

https://github.com/pradyunsg/pip/runs/2250246469?check_suite_focus=true

Whoops, I think what was the wrong tab's URL. Sorry!

@mattip
Copy link
Contributor

mattip commented Apr 2, 2021

If I understand the log correctly, it seems there is a mismatch between distutils and sysconfig?

WARNING: Value for scheme.platlib does not match.  \
    Please report this to <https://github.com/pypa/pip/issues/9617>
distutils: <workspace>/venv/user/lib/python3.6/site-packages
sysconfig: <workspace>/venv/user/lib/pypy3.6/site-packages
 WARNING: Value for scheme.purelib does not match. \
    Please report this to <https://github.com/pypa/pip/issues/9617>
distutils: <workspace>/venv/user/lib/python3.6/site-packages
sysconfig: <workspace>/venv/user/lib/pypy3.6/site-packages
WARNING: Additional context:
        user = True
        home = None
        root = None
        prefix = None

... and going over to issue #9617 I see you already reached the same conclusion :). I will continue there.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants