bpo-43307: sync sysconfig and site with PyPy to add an implementation key by mattip · Pull Request #24628 · python/cpython

mattip · 2021-02-23T17:08:10Z

PyPy has extended the INSTALL_SCHEMA to add handling multiple implementations. This is a backport of these changes. Additionally, tweak the license stanza in site.py (as long as I was syncing).

https://bugs.python.org/issue43307

merwok · 2021-02-24T16:54:48Z

            # sys.abiflags may not be defined on all platforms.
            _CONFIG_VARS['abiflags'] = ''
+        _CONFIG_VARS['implementation'] = _get_implementation()
+        _CONFIG_VARS['implementation_lower'] = _get_implementation().lower()


Not sure if this is the correct name; we do have sys.implementation with names like cpython, jython, etc. The usage here seems different, more like a directory name to find libraries.

What would be a better name? We need something that maps like CPython -> python, PyPy -> pypy. I don't know what Pyston/Jython/Graal python use, maybe some of them want to reuse the python name?

My point is that cpython is an implementation name, Python isn’t.

Better name: any idea if we don’t look at the values but the usage? Isn’t it the name for a directory containing libraries or modules?

I agree, implementation does not fit well. There is platlibdir. How about something like impllibdir for "implementation library directory"?

Adopted impllibdir

mattip · 2021-02-24T17:12:08Z

I think this can get a "skip news" label.

merwok · 2021-02-24T21:51:07Z

I would disagree, this change should be in what’s new so that interested parties can be notified.

tiran · 2021-02-28T09:29:45Z

            # sys.abiflags may not be defined on all platforms.
            _CONFIG_VARS['abiflags'] = ''
+        _CONFIG_VARS['implementation'] = _get_implementation()
+        _CONFIG_VARS['implementation_lower'] = _get_implementation().lower()


I agree, implementation does not fit well. There is platlibdir. How about something like impllibdir for "implementation library directory"?

tiran · 2021-02-28T09:31:00Z

+# NOTE: site.py has copy of this function.
+# Sync it when modify this function.
+def _get_implementation():
+    if '__pypy__' in sys.builtin_module_names:
+        return 'PyPy'
+    return 'Python'


Would it be possible to import site here and use the site._get_implementation()?

The functionality is only in site.py for a performance tweak.

sysconfig and it's dependencies are relatively large but site module needs very limited part of them. To speedup startup time, we have copy of them.

Maybe we should remove the performance tweak instead? site.py already imports os, so I wonder if the statement is still true.

mattip · 2021-03-01T06:35:06Z

CI is passing. This is ready for another review.

github-actions · 2021-04-01T00:10:42Z

This PR is stale because it has been open for 30 days with no activity.

mattip · 2021-04-01T05:46:46Z

@tiran, @merwok is there something I should do here?

tiran · 2021-04-22T17:36:17Z

+_is_pypy = '__pypy__' in sys.builtin_module_names
+
+# NOTE: site.py has copy of this function.
+# Sync it when modify this function.
+def _get_impllibdir(os_name):
+    if not _is_pypy and os_name != 'nt':
+        return 'python'
+    elif not _is_pypy and os_name == 'nt':
+        return 'Python'
+    elif _is_pypy and os_name != 'nt':
+        return 'pypy'
+    elif _is_pypy and os_name == 'nt':
+        return 'PyPy'
+


Is there any reason why you have duplicated the code instead of importing them from site? The site module is almost always imported at startup.

Suggested change

_is_pypy = '__pypy__' in sys.builtin_module_names

# NOTE: site.py has copy of this function.

# Sync it when modify this function.

def _get_impllibdir(os_name):

if not _is_pypy and os_name != 'nt':

return 'python'

elif not _is_pypy and os_name == 'nt':

return 'Python'

elif _is_pypy and os_name != 'nt':

return 'pypy'

elif _is_pypy and os_name == 'nt':

return 'PyPy'

_is_pypy = '__pypy__' in sys.builtin_module_names

# NOTE: site.py has copy of this function.

# Sync it when modify this function.

def _get_impllibdir(os_name):

if _is_pypy:

return 'PyPy' if os_name == 'nt' else 'pypy'

else:

return 'Python' if os_name == 'nt' else 'python'

I think the design is that the site module can be overridden by the user, so it cannot be the canonical source of information. I think the sysconfig code was originally copied to prevent site.py importing sysconfig.py during startup, which would slow things down. I was just following that usage pattern.

Do you have references for that? sitecustomize is for custom behaviour, but I don’t see that site is meant to be replaced.

I could see a bootstrap problem though, for example during Python build we may need sysconfig but can’t import site (that needs modules such as io and others).

bedevere-bot · 2021-04-22T17:39:51Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

brettcannon

Nothing to add beyond what Christian already asked about the code duplication.

merwok · 2021-04-28T15:40:05Z

A small question: is the PR title (which will become the commit message) still accurate?
With the new implibdir variable and the requested changes, is this still a sync/port, or is it a new version that could be used by CPython and PyPy?

mattip · 2021-06-15T06:58:10Z

Closing this, a PR is not the right place to get a consensus across

cpython, pypy, ...
distros: windows, debian, bsd, gentoo, darwin, ...
installers such as virtualenv/venv, conda, ...
desires for custom schema for performance and out-of-interpreter use in places like site.py, cross-compiling, conda installations, ...

I thought I could reduce the dimensionality of the problem by removing the implementation from the equation but it seems not.

the-knights-who-say-ni added the CLA signed label Feb 23, 2021

bedevere-bot added the awaiting review label Feb 23, 2021

uranusjr mentioned this pull request Feb 23, 2021

Implement sysconfig-based location inference pypa/pip#9626

Merged

sync sysconfig/site.py with PyPy to add an implementation key

9cbdac8

merwok reviewed Feb 24, 2021

View reviewed changes

Comment thread Lib/site.py Outdated

merwok reviewed Feb 24, 2021

View reviewed changes

redo setcopyright()

9ea9f2b

tiran reviewed Feb 28, 2021

View reviewed changes

blurb-it Bot and others added 3 commits February 28, 2021 18:32

📜🤖 Added by blurb_it.

da72a52

fixes from review

f57911c

more name substitutions

2f79469

merwok reviewed Feb 28, 2021

View reviewed changes

Comment thread Lib/site.py Outdated

mattip added 3 commits February 28, 2021 21:56

fixes from review

e74b9c1

add _is_pypy to sysconfig.py

60ac2b1

reorder

566b1a5

github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 1, 2021

github-actions Bot removed the stale Stale PR or inactive for long period of time. label Apr 2, 2021

tiran requested changes Apr 22, 2021

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Apr 22, 2021

tiran requested review from brettcannon and methane April 22, 2021 17:40

brettcannon reviewed Apr 23, 2021

View reviewed changes

merwok changed the title ~~bpo-43307: sync sysconfig/site.py with PyPy to add an implementation key~~ bpo-43307: sync sysconfig and site with PyPy to add an implementation key Apr 28, 2021

mattip mentioned this pull request Jun 13, 2021

initial win support conda-forge/pypy3.6-feedstock#41

Merged

5 tasks

mattip closed this Jun 15, 2021

mattip mentioned this pull request Apr 3, 2023

Sync site.py and sysconfig.py with PyPy #87473

Closed

Uh oh!

Uh oh!

Conversation

mattip commented Feb 23, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Feb 24, 2021

Uh oh!

merwok commented Feb 24, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattip commented Mar 1, 2021

Uh oh!

github-actions Bot commented Apr 1, 2021

Uh oh!

mattip commented Apr 1, 2021

Uh oh!

tiran Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Apr 22, 2021

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

merwok commented Apr 28, 2021

Uh oh!

mattip commented Jun 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mattip commented Feb 23, 2021 •

edited by bedevere-bot

Loading

tiran Apr 22, 2021 •

edited

Loading