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

Switch install scheme backend to sysconfig #10358

Merged
merged 2 commits into from
Aug 21, 2021

Conversation

uranusjr
Copy link
Member

Now we’ve weeded out (almost) all the known distutils-sysconfig inompatibilities, let’s make the switch? Installations running on Python 3.9 or lower are not affected. Only Python 3.10 and up will use sysconfig.

Right now I’m targeting 21.3 although this is a bit awkward since CPython 3.10 is scheduled to be released slightly before that. But that’s probably a good thing since we can at least be sure the pip bundled in 3.10.0 is “guaranteed” to work? We can always bundle this switch in the first 3.10.x patch release anyway.

@uranusjr uranusjr added the type: deprecation Related to deprecation / removal. label Aug 13, 2021
@uranusjr uranusjr added this to the 21.3 milestone Aug 13, 2021
@uranusjr uranusjr force-pushed the sysconfig-switch-python-310 branch from 0d6c892 to 443533a Compare August 13, 2021 10:17
@uranusjr
Copy link
Member Author

Side note: CPython currently has an unresolved bug that installs user-site platform-dependent packages (platlib) into a wrong location on certain platforms e.g. Fedora 34 (bpo-44860). I’m pretty sure this will be fixed before 3.10.0 proper is released, but if it’s not, that’s CPython’s problem.

@uranusjr uranusjr force-pushed the sysconfig-switch-python-310 branch from 443533a to cb411c2 Compare August 13, 2021 11:01
@pradyunsg
Copy link
Member

Do we need to wait for python/cpython#27655 to be released then?

https://github.com/pypa/pip/runs/3321341848#step:5:2976 seems to be an esoteric error. :/

@uranusjr
Copy link
Member Author

pypa/pip/runs/3321341848#step:5:2976 seems to be an esoteric error. :/

Huh, what the hell? This also seems to be a pretty environment-specific thing—the Windows tests are all passing, including the 3.10 ones.

@pradyunsg
Copy link
Member

Gonna move the failure output to a comment, in case it's useful for future reference; and re-running the build.

_________________ TestInstallUnpackedWheel.test_install_prefix _________________
[gw0] linux -- Python 3.10.0 /home/runner/work/pip/pip/.tox/py/bin/python

self = <tests.unit.test_wheel.TestInstallUnpackedWheel object at 0x7f4125518df0>
data = <tests.lib.TestData object at 0x7f41253b71f0>
tmpdir = Path('/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0')

    def test_install_prefix(self, data, tmpdir):
        prefix = os.path.join(os.path.sep, 'some', 'path')
        self.prep(data, tmpdir)
        scheme = get_scheme(
            self.name,
            user=False,
            home=None,
            root=tmpdir,
            isolated=False,
            prefix=prefix,
        )
>       wheel.install_wheel(
            self.name,
            self.wheelpath,
            scheme=scheme,
            req_description=str(self.req),
        )

data       = <tests.lib.TestData object at 0x7f41253b71f0>
prefix     = '/some/path'
scheme     = <pip._internal.models.scheme.Scheme object at 0x7f41253d2d90>
self       = <tests.unit.test_wheel.TestInstallUnpackedWheel object at 0x7f4125518df0>
tmpdir     = Path('/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0')

tests/unit/test_wheel.py:411: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.tox/py/lib/python3.10/site-packages/pip/_internal/operations/install/wheel.py:762: in install_wheel
    _install_wheel(
        direct_url = None
        name       = 'sample'
        pycompile  = True
        req_description = 'sample'
        requested  = False
        scheme     = <pip._internal.models.scheme.Scheme object at 0x7f41253d2d90>
        warn_script_location = True
        wheel_path = '/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/sample-1.2.0-py2.py3-none-any.whl'
        z          = <zipfile.ZipFile [closed]>
.tox/py/lib/python3.10/site-packages/pip/_internal/operations/install/wheel.py:644: in _install_wheel
    success = compileall.compile_file(path, force=True, quiet=True)
        all_paths  = <function _install_wheel.<locals>.all_paths at 0x7f41253f4f70>
        assert_no_path_traversal = <function _install_wheel.<locals>.assert_no_path_traversal at 0x7f41253f4e50>
        changed    = set()
        console    = {'sample': 'sample:main'}
        data_scheme_file_maker = <function _install_wheel.<locals>.data_scheme_file_maker at 0x7f41253f4790>
        data_scheme_paths = <filter object at 0x7f41253b7c70>
        direct_url = None
        distribution = <pip._internal.metadata.pkg_resources.Distribution object at 0x7f41253b7430>
        file       = <pip._internal.operations.install.wheel.ZipBackedFile object at 0x7f41253b7160>
        file_paths = <itertools.filterfalse object at 0x7f41253b7ca0>
        files      = <itertools.chain object at 0x7f41253b7af0>
        generated  = []
        gui        = {'sample2': 'sample:main'}
        info_dir   = 'sample-1.2.0.dist-info'
        installed  = {'sample-1.2.0.data/data/my_data/data_file': '../../../my_data/data_file', 'sample-1.2.0.dist-info/DESCRIPTION.rst': '...fo/METADATA': 'sample-1.2.0.dist-info/METADATA', 'sample-1.2.0.dist-info/RECORD': 'sample-1.2.0.dist-info/RECORD', ...}
        is_data_scheme_path = <function _install_wheel.<locals>.is_data_scheme_path at 0x7f41253f48b0>
        is_dir_path = <function _install_wheel.<locals>.is_dir_path at 0x7f41253f4d30>
        is_entrypoint_wrapper = <function _install_wheel.<locals>.is_entrypoint_wrapper at 0x7f41253f4af0>
        is_script_scheme_path = <function _install_wheel.<locals>.is_script_scheme_path at 0x7f41253f4a60>
        lib_dir    = Path('/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages')
        make_data_scheme_file = <function _install_wheel.<locals>.data_scheme_file_maker.<locals>.make_data_scheme_file at 0x7f41253f4ca0>
        make_root_scheme_file = <function _install_wheel.<locals>.root_scheme_file_maker.<locals>.make_root_scheme_file at 0x7f41253f4160>
        metadata   = <email.message.Message object at 0x7f41253b6ec0>
        name       = 'sample'
        other_scheme_files = <map object at 0x7f41253b7e80>
        other_scheme_paths = <itertools.filterfalse object at 0x7f41253b7550>
        path       = Path('/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__init__.py')
        paths      = <generator object _install_wheel.<locals>.all_paths at 0x7f4125d21540>
        pyc_output_path = <function _install_wheel.<locals>.pyc_output_path at 0x7f41253f4040>
        pyc_source_file_paths = <function _install_wheel.<locals>.pyc_source_file_paths at 0x7f41253f41f0>
        pycompile  = True
        record_installed = <function _install_wheel.<locals>.record_installed at 0x7f41253f55a0>
        requested  = False
        root_scheme_file_maker = <function _install_wheel.<locals>.root_scheme_file_maker at 0x7f41253f4dc0>
        root_scheme_paths = <itertools.filterfalse object at 0x7f41253b6d10>
        scheme     = <pip._internal.models.scheme.Scheme object at 0x7f41253d2d90>
        script_scheme_files = <map object at 0x7f41253b7f10>
        script_scheme_paths = <filter object at 0x7f41253b79a0>
        stdout     = <pip._internal.utils.misc.StreamWrapper object at 0x7f41253f49d0>
        warn_script_location = True
        wheel_path = '/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/sample-1.2.0-py2.py3-none-any.whl'
        wheel_zip  = <zipfile.ZipFile [closed]>
/opt/hostedtoolcache/Python/3.10.0-rc.1/x64/lib/python3.10/compileall.py:240: in compile_file
    ok = py_compile.compile(fullname, cfile, dfile, True,
        cfile      = '/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__pycache__/__init__.cpython-310.pyc'
        ddir       = None
        dfile      = None
        force      = True
        fullname   = Path('/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__init__.py')
        hardlink_dupes = False
        head       = '__init__'
        index      = 0
        invalidation_mode = None
        legacy     = False
        limit_sl_dest = None
        name       = '__init__.py'
        opt_cfiles = {-1: '/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__pycache__/__init__.cpython-310.pyc'}
        opt_level  = -1
        optimize   = [-1]
        prependdir = None
        quiet      = True
        rx         = None
        stripdir   = None
        success    = True
        tail       = '.py'
/opt/hostedtoolcache/Python/3.10.0-rc.1/x64/lib/python3.10/py_compile.py:162: in compile
    bytecode = importlib._bootstrap_external._code_to_timestamp_pyc(
        cfile      = '/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__pycache__/__init__.cpython-310.pyc'
        code       = <code object <module> at 0x7f41253e3470, file "/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__init__.py", line 1>
        dfile      = None
        dirname    = '/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__pycache__'
        doraise    = True
        file       = Path('/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__init__.py')
        invalidation_mode = <PycInvalidationMode.TIMESTAMP: 1>
        loader     = <_frozen_importlib_external.SourceFileLoader object at 0x7f41253b7c40>
        optimize   = -1
        quiet      = 0
        source_bytes = b'\n__version__ = \'1.2.0\'\n\ndef main():\n    """Entry point for the application script"""\n    print("Call your main application code here")\n'
        source_stats = {'mtime': 1628852640.7727487, 'size': 135}
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

code = <code object <module> at 0x7f41253e3470, file "/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__init__.py", line 1>
mtime = 1628852640.7727487, source_size = 135

>   ???
E   ValueError: unmarshallable object

code       = <code object <module> at 0x7f41253e3470, file "/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__init__.py", line 1>
data       = bytearray(b'o\r\r\n\x00\x00\x00\x00\xa0Q\x16a\x87\x00\x00\x00')
mtime      = 1628852640.7727487
source_size = 135

<frozen importlib._bootstrap_external>:689: ValueError

@pradyunsg
Copy link
Member

pradyunsg commented Aug 13, 2021

Maybe @brettcannon has an idea of what's up?

CPython seems to be failing to compile this content to bytecode:

"sample/__init__.py": textwrap.dedent(
'''
__version__ = '1.2.0'
def main():
"""Entry point for the application script"""
print("Call your main application code here")
'''
),

@pradyunsg
Copy link
Member

re-running the build.

Definitely not a flaky failure. :/

@brettcannon
Copy link
Member

Is the code marshalled on the fly or somehow checked into the repo? If it's the former it may be a bug, if it's the latter then it's probably due to the marshal format being changed.

I know Guido and the perf team have been poking around in marshal lately but I don't know if any of their PRs landed upstream in 3.10.

@pradyunsg
Copy link
Member

There's no marshal'd contented checked in to this repository. This file is generated on each test run, and compiled using compileall.compile_file(path, force=True, quiet=True), where path is:

path       = Path('/tmp/pytest-of-runner/pytest-0/popen-gw0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__init__.py')

The contents are from the link above, and can be confirmed in the "showlocals" output from pytest, that's augumented the traceback.

The ValueError seems to bubble up from importlib._bootstrap_external._code_to_timestamp_pyc.

@brettcannon
Copy link
Member

@pradyunsg if you can get a small reproducer then please open a bug at bugs.python.org.

@uranusjr
Copy link
Member Author

So I tried compiling CPython from source (I’m not sure the exact version GitHub is using so tested with rc) and this test seems to run fine. Since all we do here is switching from distutils to sysconfig, and all that affects is potentially changing some path values pip uses to install things to, I’m suspecting this is an environment issue. Maybe GHA’s Python setup makes sysconfig point to a different location from distutils that somehow cannot be correctly pycompile-d 😟

@uranusjr uranusjr force-pushed the sysconfig-switch-python-310 branch from 5abbecb to 6f6c038 Compare August 19, 2021 15:22
@uranusjr
Copy link
Member Author

uranusjr commented Aug 19, 2021

Err, the paths are the same as far as I can tell?

https://github.com/pypa/pip/runs/3373309381#step:5:3012

@pradyunsg
Copy link
Member

pradyunsg commented Aug 20, 2021

Did a round of debugging by ssh'ing into the GHA runner. Turns out, somehow, tests.lib.path.Path objects are not marshallable in 3.10 + GHA?

(Pdb) code.co_consts[1].co_filename
Path('/tmp/pytest-of-runner/pytest-0/test_install_prefix0/some/path/lib/python3.10/site-packages/sample/__init__.py')
(Pdb) code.co_consts[1].co_filename.__class__
<class 'tests.lib.path.Path'>
(Pdb) marshal.dumps(code.co_consts[1].co_filename)
*** ValueError: unmarshallable object

This looks like it's super specific to pip's test suite. The fix for us, will be to change root=tmpdir, to root=str(tmpdir).

I'd say #9578 has became a bit more important now. :)

@pradyunsg
Copy link
Member

Specifically, the patch is:

diff --git a/tests/unit/test_wheel.py b/tests/unit/test_wheel.py
index 3b39f91b97c..863b4ddb011 100644
--- a/tests/unit/test_wheel.py
+++ b/tests/unit/test_wheel.py
@@ -404,7 +404,7 @@ def test_install_prefix(self, data, tmpdir):
             self.name,
             user=False,
             home=None,
-            root=tmpdir,
+            root=str(tmpdir),
             isolated=False,
             prefix=prefix,
         )

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Aug 20, 2021
@uranusjr
Copy link
Member Author

We really should switch to pathlib.Path.

@uranusjr
Copy link
Member Author

I do wonder why the class suddenly becomes unmarshallable though. Were we depending on some internals we shouldn’t (and 3.10 exposes our mistake), or is 3.10 breaking compatibility intentially? But that’s not pip-related so I’ll do it in my own free time (wait, but I’m doing pip in my free time anyway, what’s the difference).

@uranusjr uranusjr force-pushed the sysconfig-switch-python-310 branch from 41453b5 to 7ed5911 Compare August 20, 2021 16:53
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 20, 2021
This is only done for Python 3.10+ so we don't disrupt too many existing
pip installations.
@uranusjr uranusjr force-pushed the sysconfig-switch-python-310 branch from 7ed5911 to d1d9bc5 Compare August 20, 2021 16:54
@pradyunsg
Copy link
Member

wait, but I’m doing pip in my free time anyway, what’s the difference

LOL

@pradyunsg pradyunsg merged commit 3e640bc into pypa:main Aug 21, 2021
@encukou
Copy link
Contributor

encukou commented Sep 7, 2021

Whoa! How did you get a non-string co_filename on a code object? I don't think that should be possible. Perhaps the path was silently encoded before?

Path objects never were marshallable – the format doesn't support them. There's a list of supported types in marshal docs, and Path isn't on it.
(I don't think code objects should be allowed to contain arbitrary objects, but if so, they should be added to the list of “containers” in those docs.)

@pfmoore
Copy link
Member

pfmoore commented Sep 7, 2021

@encukou See the comment above - we apparently do compileall.compile_file(path, force=True, quiet=True) where path is a Path object.

Sounds like a bug in compile_file, which should probably ensure the filename is a string (via something like os.fspath?)

Edit: I can't easily find a reproducer, though - is it 3.10 only?

@uranusjr
Copy link
Member Author

uranusjr commented Sep 7, 2021

To clarify, the Path object here is not pathlib.Path; pip has its own Path class that’s a str subclass. But yeah this sounds like a bug in compile_file; maybe it’s only able to handle str but not a subclass?

@uranusjr uranusjr deleted the sysconfig-switch-python-310 branch September 7, 2021 13:22
@encukou
Copy link
Contributor

encukou commented Sep 7, 2021

pip has its own Path class that’s a str subclass

Oh! That's the missing link. I'll file a bpo. Edit: bpo-45127 filed
I guess there should be some changes to Python, but only regarding which exception is raised and where, so it won't be a high priority.

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

Successfully merging this pull request may close these issues.

7 participants