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

Support PyPy 3.8 #2206

Merged
merged 7 commits into from
Oct 23, 2021
Merged

Support PyPy 3.8 #2206

merged 7 commits into from
Oct 23, 2021

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Oct 4, 2021

Thanks for contributing, make sure you address all the checklists (for details on how see

development documentation)!

  • ran the linter to address style issues (tox -e fix_lint)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

This builds on and replaces #2204. In an earlier version of portable PyPy (one built on manylinux 2010 and bundling dependent shared objects), it was necessary to copy the bundled shared objects from prefix/lib. This is no longer needed since now the libpypy3-c.so is symlinked not copied.

On non-portable PyPy installations, like those in distros or on conda-forge, this code was never used since there the dependent shared objects are provided outside the python tree.

Also fixed the stdlib which had python not pypy as the stem of the directory name.

I tested this locally to make sure it works as advertised.

Comment on lines 41 to 47
def sources(cls, interpreter):
for src in super(PyPy3Posix, cls).sources(interpreter):
yield src
host_lib = Path(interpreter.system_prefix) / "lib"
if host_lib.exists() and host_lib.is_dir():
for path in host_lib.iterdir():
yield PathRefToDest(path, dest=cls.to_lib)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fairly sure there was a pypy3 variant that worked like this (over a year ago when I wrote this). Doesn't this drop support for those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let's err on the side of caution and stick with @mgorny 's fix

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd actually prefer if we'd not do this 😊 but make it version-specific; if you can dig out from pypy when this change happened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before PyPy's reorganization of the directory layout for python3.8 there were no directories in prefix/lib.

It seems this code was added in #1503 in a blanket change to "support c-extensions inside virtual environments". The issue pointed to is #1448, but that issue only mentions include files, not lib files. It seems to relate to issue #1023, but that one is windows only and deals with the libs directory, not the lib one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, in that case I'm happy to go with your original proposal, sorry for the back and forth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, updating

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Seems the pypy3 test suite fails 🤔

@mattip
Copy link
Contributor Author

mattip commented Oct 4, 2021

Hmm. When I run ../pypy3.6-HEAD/bin/pypy -m pytest tests/unit/create I get an error error: unrecognized arguments: --no-success-flaky-report. This is with pytest 6.2.5. I needed to install flaky.

We should add a pypy3.7 run.

@gaborbernat
Copy link
Contributor

We should add a pypy3.7 run.

Yes, we should have both 3.6 and 3.7 there 😊

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Assuming the CI passes 😊

@pypa pypa deleted a comment from codecov bot Oct 4, 2021
@mattip
Copy link
Contributor Author

mattip commented Oct 4, 2021

Cool, thanks

.github/workflows/check.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat gaborbernat changed the title Pypy3 sym Support PyPy 3.8 Oct 4, 2021
@mattip
Copy link
Contributor Author

mattip commented Oct 4, 2021

I think the linux pypy3.8 tests will be fixed with rc2 which should be ready this week. I am not sure what is the problem with windows, is it missing python.exe ?

@gaborbernat
Copy link
Contributor

I think the linux pypy3.8 tests will be fixed with rc2 which should be ready this week. I am not sure what is the problem with windows, is it missing python.exe ?

Could be this tox-dev/filelock#101? Does pypy support https://docs.python.org/3/library/msvcrt.html#msvcrt.locking? Pypy on Windows is sometimes interesting 🤔 but don't have time to debug it...

@mattip
Copy link
Contributor Author

mattip commented Oct 4, 2021

Could be this tox-dev/filelock#101?

Maybe

Does pypy support https://docs.python.org/3/library/msvcrt.html#msvcrt.locking?

Nominally, but there may be bugs since it is implemented via CFFI, so maybe there are threading problems ...

@gaborbernat
Copy link
Contributor

Guess this is blocked until RC2 then? Ping me once that happens.

@mattip
Copy link
Contributor Author

mattip commented Oct 4, 2021

ok, but I would like to leave windows out for now, otherwise we won't have a virtualenv story for the 3.8 release.

@gaborbernat
Copy link
Contributor

gaborbernat commented Oct 4, 2021

Generally, we don't tend to do piecewise support 🤔 so if possible would prefer to fix Windows... 🤔 or at least mark the failing tests as skip explicitly 🤔

@mattip
Copy link
Contributor Author

mattip commented Oct 4, 2021

I need some help to debug the create_no_seed failures on windows. Although the source location has ['pypy.exe', `pypy3.exe`, `pypy3w.exe`, pypyw.exe`, `python.exe`, `pythonw.exe`], the last two are not copied to the destination, and the check for python.exe fails.

assert exe_path.exists(), "\n".join(str(i) for i in creator.bin_dir.iterdir())
.

But when I stop in a debugger where the exes are listed, python appears in the list.

def exe_names(cls, interpreter):
return super(PyPy3, cls).exe_names(interpreter) | {"pypy"}
.

Why isn't it being copied?

@gaborbernat
Copy link
Contributor

gaborbernat commented Oct 5, 2021

@mattip The tests failing are not the ones testing that our creator is correct, but rather than the built-in venv behaves equivalently to ours. And in this case seems pypy differs, as the python.exe is not created when using pypy builtin venv 🤔 the weird part here though is that neither 3.6 should pass... but for some reason does (only in the CI locally for me fails both). So guess the question is that should the venv module on Windows create python.exe? And if not we should patch the test to express such 😊 virtualenv does create this because people often expect python.exe to work even if one is in pypy... but seems venv from pypy does not.

@mattip
Copy link
Contributor Author

mattip commented Oct 5, 2021

the weird part here though is that neither 3.6 should pass

The 3.6 windows downloads were last updated for release 7.3.3, and have only pypy3.exe and pypy3w.exe. We started adding all the other exe variants three months ago, so only the 7.3.6rc1 downloads have pypy.exe, python.exe and python3.exe

@mattip
Copy link
Contributor Author

mattip commented Oct 5, 2021

Ahah. Found the problem with venv. In symlink_or_copy, it intercepts python.exe and replaces it with venvlauncher.exe, which does not exists on PyPy.

@gaborbernat
Copy link
Contributor

Ahah. Found the problem with venv. In symlink_or_copy, it intercepts python.exe and replaces it with venvlauncher.exe, which does not exists on PyPy.

Is that within pypy venv module?

@mattip
Copy link
Contributor Author

mattip commented Oct 5, 2021

Yes.

@mattip
Copy link
Contributor Author

mattip commented Oct 5, 2021

cool. Fixing venv makes the failures go away locally. I will put that in for rc2 and then we can try it here in CI.

@gaborbernat
Copy link
Contributor

👍

@mattip
Copy link
Contributor Author

mattip commented Oct 6, 2021

Something is off: bin/pypy3: error while loading shared libraries: libpypy3-c.so: cannot open shared object file: No such file or directory. :(

@mattip
Copy link
Contributor Author

mattip commented Oct 6, 2021

It seems I should pay better attention to stdlib venv test failures. The problem is there, not with virtualenv

@gaborbernat
Copy link
Contributor

virtualenv seems like a good place to test the venv module 😃

@mattip
Copy link
Contributor Author

mattip commented Oct 12, 2021

PyPy3.8 on windows hangs waiting for a lock in a __del__. Would it make sense to refactor this code to avoid doing anything in the destructor as long as _del_lock(lock) is called with None, and then in the __exit__ set self.lock to None once it is released?

def _del_lock(lock):
with _store_lock:
if lock is not None:
with lock.thread_safe:

refactored to

    def _del_lock(lock):
        if lock is not None:
            with _store_lock:
                with lock.thread_safe:

  File "D:\a\virtualenv\virtualenv\.tox\pypy3\lib\site-packages\virtualenv\app_data\via_disk_folder.py", line 99, in py_info_clear
    with py_info_folder.lock_for_key(filename.stem):
  File "C:\hostedtoolcache\windows\PyPy\3.8.12\x64\Lib\contextlib.py", line 113, in __enter__
    return next(self.gen)
  File "D:\a\virtualenv\virtualenv\.tox\pypy3\lib\site-packages\virtualenv\util\lock.py", line 132, in lock_for_key
    lock = self._create_lock(name)
  File "D:\a\virtualenv\virtualenv\.tox\pypy3\lib\site-packages\virtualenv\util\lock.py", line 88, in _create_lock
    _lock_store[lock_file] = _CountedFileLock(lock_file)
  File "D:\a\virtualenv\virtualenv\.tox\pypy3\lib\site-packages\virtualenv\util\lock.py", line 18, in __init__
    parent = os.path.dirname(lock_file)
  File "C:\hostedtoolcache\windows\PyPy\3.8.12\x64\Lib\ntpath.py", line 223, in dirname
    return split(p)[0]
  File "C:\hostedtoolcache\windows\PyPy\3.8.12\x64\Lib\ntpath.py", line 186, in split
    seps = _get_bothseps(p)
  File "C:\hostedtoolcache\windows\PyPy\3.8.12\x64\Lib\ntpath.py", line 38, in _get_bothseps
    return '\\/'
  File "D:\a\virtualenv\virtualenv\.tox\pypy3\lib\site-packages\coverage\pytracer.py", line 101, in _trace
    if THIS_FILE in frame.f_code.co_filename:
  File "D:\a\virtualenv\virtualenv\.tox\pypy3\lib\site-packages\virtualenv\util\lock.py", line 100, in __del__
    self._del_lock(self._lock)
  File "D:\a\virtualenv\virtualenv\.tox\pypy3\lib\site-packages\virtualenv\util\lock.py", line 93, in _del_lock
    with _store_lock:

@mattip
Copy link
Contributor Author

mattip commented Oct 12, 2021

Is coverage running for PyPy? That makes the code run without the JIT which significantly slows things down. Would it be OK to toggle coverage off for PyPy?

@mattip
Copy link
Contributor Author

mattip commented Oct 12, 2021

Additionally, macos PyPy 3.8 failed test_app_data_pinning with

ERROR: No matching distribution found for pip==19.3.1

A transient network error?

mgorny and others added 7 commits October 15, 2021 13:48
The PyPy3 logic creates symlinks for all files from the library
directory existing alongside the PyPy executable.  This is meant
to ensure that the bundled libraries to which PyPy is linked can also
be found from inside the virtualenv.  However, this logic also symlinks
all directories which is unnecessary and causes library directory
collisions with the new install layout.  Change to logic to symlink
non-directories only.

A similar fix has been applied to the internal venv module in PyPy3.8:
https://foss.heptapod.net/pypy/pypy/-/commit/713b2af9abd2b9453e12c60143e17431a1aefb33

Fixes pypa#2182
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
@mattip
Copy link
Contributor Author

mattip commented Oct 15, 2021

Rebased off main to get #2213. CI passed except codecov.

@mattip
Copy link
Contributor Author

mattip commented Oct 18, 2021

PyPy has released a python3.8 that requires this change.

@rodrigodc
Copy link

I confirm that this PR fixes the problem for PyPy 3.8, released yesterday.

@gaborbernat gaborbernat merged commit 4188ac6 into pypa:main Oct 23, 2021
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.

4 participants