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

Add pypy 7.2.0 #373

Closed
wants to merge 9 commits into from
Closed

Add pypy 7.2.0 #373

wants to merge 9 commits into from

Conversation

hyperair
Copy link

PyPy 7.3.1 has a new ABI version (73). We should support PyPy 7.2.0 as well,
which has ABI verison 41. This is consistent with the presence of both pypy
7.2.0 and 7.3.1 in the pypywheels/manylinux2010-pypy_x86_64 image.

PyPy 7.3.1 has a new ABI version (73). We should support PyPy 7.2.0 as well,
which has ABI verison 41. This is consistent with the presence of both pypy
7.2.0 and 7.3.1 in the pypywheels/manylinux2010-pypy_x86_64 image.
@YannickJadoul
Copy link
Member

We specifically had to wait for PyPy 7.3.0 before we could add PyPy support and merge #185, if I remember correctly: #185 (comment). Please read the rest of this PR for a full log of what we did.

Quite a few details were fixed (e.g., the ABI version naming was made more future-proof and fixes to pip for this; I don't fully remember all details to be honest), as we were working together with the PyPy developers to fit it into cibuildwheel and reporting minor differences to CPython that became apparent from using it in cibuildwheel.

I also think there's no real point in supporting the old 7.2.0? PyPy wheels are not fully spread on PyPI yet, and updating from PyPy 7.2.0 to PyPy 7.3.1 should be quite painless anyway. At any rate, the plan is to do support the last 2 or 3 releases of PyPy, when future new versions of PyPy come out. Still to be discussed with PyPy and their policy on supporting older releases.

That being said, if you manage to get all tests green, I'm OK with merging it. But it's going to take a lot more work than the few lines you've added, and I don't even know it's possible.

Match name from the directory name in the pypywheels/manylinux2010-pypy_x86_64
docker image.
@hyperair
Copy link
Author

hyperair commented Jun 10, 2020

To be honest, my motivation for this was the pypy2.7-41 ABI, because that spans all the way back to pypy 4.1(?) I think.

I have something that looks like it works right now, but I need to look a bit closer and see whether it's actually viable or not.

I'll have a look at the tests and see if I want to continue down this path.

@YannickJadoul
Copy link
Member

To be honest, my motivation for this was the pypy2.7-41 ABI, because that spans all the way back to pypy 4.1(?) I think.

Hmm, I don't really know, but that would be a good argument, indeed.

I have something that looks like it works right now, but I need to look a bit closer and see whether it's actually viable or not.

That's just Linux, though? Not sure how hard the rest will be.

I'll have a look at the tests and see if I want to continue down this path.

OK, good luck! If I can find some time, I'll see if reading #185 refreshes my mind on possible changes we needed in 7.3.0.

@hyperair
Copy link
Author

OK, good luck! If I can find some time, I'll see if reading #185 refreshes my mind on possible changes we needed in 7.3.0.

Thanks!

@hyperair
Copy link
Author

hyperair commented Jun 11, 2020

So macos and windows builds are failing, with the following errors respectively:

  ERROR: Command errored out with exit status 1:
   command: /tmp/cibw_bin/python -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/pip-req-build-rAkgSa/setup.py'"'"'; __file__='"'"'/private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/pip-req-build-rAkgSa/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/pip-wheel-KO2bV3
       cwd: /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/pip-req-build-rAkgSa/
  Complete output (8 lines):
  running bdist_wheel
  running build
  running build_ext
  building 'spam' extension
  creating build
  creating build/temp.macosx-10.14-x86_64-2.7
  cc -pthread -arch x86_64 -DNDEBUG -O2 -fPIC -I/tmp/pypy2.7-v7.2.0-osx64/include -c spam.c -o build/temp.macosx-10.14-x86_64-2.7/spam.o
  error: $MACOSX_DEPLOYMENT_TARGET mismatch: now "10.9" but "10.14" during configure

and

debug: OperationError:
debug:  operror-type: LookupError
debug:  operror-value: cp65001 encoding is only available on Windows
Traceback (most recent call last):
  File "C:\Python36\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "C:\Python36\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "c:\users\travis\build\joerick\cibuildwheel\cibuildwheel\__main__.py", line 314, in <module>
    main()
  File "c:\users\travis\build\joerick\cibuildwheel\cibuildwheel\__main__.py", line 231, in main
    cibuildwheel.windows.build(build_options)
  File "c:\users\travis\build\joerick\cibuildwheel\cibuildwheel\windows.py", line 174, in build
    env = setup_python(config, dependency_constraint_flags, options.environment)
  File "c:\users\travis\build\joerick\cibuildwheel\cibuildwheel\windows.py", line 132, in setup_python
    shell(['python', '--version'], env=env)
  File "c:\users\travis\build\joerick\cibuildwheel\cibuildwheel\windows.py", line 28, in shell
    return subprocess.check_call(' '.join(args), env=env, cwd=cwd, shell=True)
  File "C:\Python36\lib\subprocess.py", line 311, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command 'python --version' returned non-zero exit status 1.
============================== warnings summary ===============================

I'm not really sure how to go about debugging these. @YannickJadoul any ideas?

Edit: The Windows error might be from this bug, and suggests that pypy 7.2.0 might not be viable for Windows.

How open are you to me adding pypy 7.2.0 support only for Linux?

Edit 2: It works on macos now 🥳

@hyperair hyperair force-pushed the pypy41-abi branch 3 times, most recently from b018d7a to f395c7b Compare June 11, 2020 19:48
This should hopefully fix the following error:

 ERROR: Command errored out with exit status 1:
   command: /tmp/cibw_bin/python -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/pip-req-build-rAkgSa/setup.py'"'"'; __file__='"'"'/private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/pip-req-build-rAkgSa/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/pip-wheel-KO2bV3
       cwd: /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/pip-req-build-rAkgSa/
  Complete output (8 lines):
  running bdist_wheel
  running build
  running build_ext
  building 'spam' extension
  creating build
  creating build/temp.macosx-10.14-x86_64-2.7
  cc -pthread -arch x86_64 -DNDEBUG -O2 -fPIC -I/tmp/pypy2.7-v7.2.0-osx64/include -c spam.c -o build/temp.macosx-10.14-x86_64-2.7/spam.o
  error: $MACOSX_DEPLOYMENT_TARGET mismatch: now "10.9" but "10.14" during configure
@YannickJadoul
Copy link
Member

Hi @hyperair. Sorry for getting back to you so late :-/

Quite impressive you got things working on macOS, already! :-)

About Windows: did you see this? #185 (review)
If I remember this correctly, this is going to help

How open are you to me adding pypy 7.2.0 support only for Linux?

I don't think we'd be too happy about that, as cibuildwheel's goal is to support this across platforms and CIs. If it's just for Linux, I would almost claim that it might be better to manually run the docker, then, for the rare case when someone wants to produce older wheels; after all, once the docker image is running, there's hardly any setup to be done.

Let's also get @joerick involved, on what complexity price (in workaround/hacks around the code) we're willing to pay to support this older version of PyPy?

@YannickJadoul
Copy link
Member

And it might be interesting to as the opinion on the PyPy developers as well.

@mattip, is there any (implicit or explicit) policy on the support for older versions of PyPy? From your perspective, does it make sense that cibuildwheel supports older ABIs, or do want to push users to update to the latest PyPy version?

@mattip
Copy link
Contributor

mattip commented Jun 13, 2020

I would strongly suggest not to support older versions of PyPy. We do not backport bug-fixes, and do not have the capacity to try to think of work-arounds for known bugs.

@YannickJadoul
Copy link
Member

Thanks, @mattip!

@hyperair, not sure where to go from here, then, given that you've already put in quite some effort to get this working, by now, and seem to have almost succeeded? :-/ I don't know if there's a specific reason/use case why you wanted this support for the older PyPy ABI in cibuildwheel?

@hyperair
Copy link
Author

@hyperair, not sure where to go from here, then, given that you've already put in quite some effort to get this working, by now, and seem to have almost succeeded? :-/ I don't know if there's a specific reason/use case why you wanted this support for the older PyPy ABI in cibuildwheel?

Well, I have some things running on pypy 5.x that are difficult to migrate immediately, and they also depend on kafka-cffi wheels. The 41 abi would help tide us over until we're ready to upgrade pypy.

As for where to go from here, I'm not sure -- are you still open to merging this PR if the Windows build works? If not, I guess I could also stop here and just use my custom branch of cibuildwheel in kafka-cffi for the time being.

@Czaki
Copy link
Contributor

Czaki commented Jun 17, 2020

Well, I have some things running on pypy 5.x that are difficult to migrate immediately, and they also depend on kafka-cffi wheels. The 41 abi would help tide us over until we're ready to upgrade pypy.

I rather suggest that you can maintain your fork for such rare case. Maintaining python version which does not have support from creator could be consume enormous time.

pip gets installed into bin/ instead of Scripts/.
@YannickJadoul
Copy link
Member

As for where to go from here, I'm not sure -- are you still open to merging this PR if the Windows build works? If not, I guess I could also stop here and just use my custom branch of cibuildwheel in kafka-cffi for the time being.

I'm not too sure; it's not just my decision, after all. But given the feedback from PyPy, I'm not too enthusiast on providing it for all users of cibuildwheel. @joerick?

How realistic is it to keep these changes just in a local branch/fork, until you manage to make a transition, like @Czaki suggests? That wouldn't mean you're on your own; happy to provide help and feedback from the cibuildwheel perspective if you run into issues, but I'm not sure we want to have all cibuildwheel users build these wheels by default (unless there're suddenly more users that need this functionality, ofc).

@YannickJadoul
Copy link
Member

Btw: the solution seems to be working? It's now just an assertion failing on the produce wheel names:

E       AssertionError: assert {'spam-0.1.0-...d64.whl', ...} == {'spam-0.1.0-...d64.whl', ...}
E         Extra items in the left set:
E         'spam-0.1.0-pp36-pp36-win32.whl'
E         Extra items in the right set:
E         'spam-0.1.0-pp36-pypy3_72-win32.whl'
E         Use -v to get the full diff

@hyperair
Copy link
Author

Btw: the solution seems to be working? It's now just an assertion failing on the produce wheel names:

Yeah, getting there..

How realistic is it to keep these changes just in a local branch/fork, until you manage to make a transition, like @Czaki suggests? That wouldn't mean you're on your own; happy to provide help and feedback from the cibuildwheel perspective if you run into issues

Not too difficult, actually. I can probably get along fine with the occasional rebase. Thanks for the offer for help, that's much appreciated.

but I'm not sure we want to have all cibuildwheel users build these wheels by default (unless there're suddenly more users that need this functionality, ofc).

That's true. Would an opt-in environment variable make sense?

@YannickJadoul
Copy link
Member

YannickJadoul commented Jun 17, 2020

Yeah, getting there..

Great :-)

That's true. Would an opt-in environment variable make sense?

It could, yes (except that we don't have anything like it, for now). The main underlying issue is how to set up a policy for what PyPy versions we support.

This is a lot easier to determine for CPython, as the interpreter version and the language version are very much intertwined (contrary to PyPy), there is a clear release and deprecation schedule that we can follow (i.e., at some point, we just dropped 3.4, since it was EOL), and they frankly just have a lot more contributors to backport changes to old still-supported versions, etc.

From that perspective, following PyPy as closely as possible has always seemed the safest route for cibuildwheel to follow. I had provisionally planned to maybe support the last 2 or 3 ABI versions (to have some overlap) and then just make give up and have users update, just like they are forced to update from 3.4 at some point. So from that perspective, I'd have been open to support 7.2.x, but the reason for not having done so is because there were a few changes on ABI tags that were solved while we were adding support (see e.g. pypa/pip#7629) and PyPy 7.2.x predates these changes that prepared better pip and wheel support.

EDIT: See also pypa/wheel#328

@joerick
Copy link
Contributor

joerick commented Jun 17, 2020

Thanks all for this great discussion above! To be honest, @YannickJadoul has been doing the bulk of the work to run and maintain our Pypy support, so I'm happy to go along with his decision. But if my opinion is useful at all,

The main underlying issue is how to set up a policy for what PyPy versions we support.

Normally our philosophy is 'if it works, and it increases support, let's merge it' but I can't say I'm super eager to increase our Pypy testing matrix right now. For me it's a trade-off between developer time and number of users (maybe I'm a little swayed by problems caused by recent changes to Appveyor's image #371) but pypy tends to be something of a black sheep and probably requires a disproportionate amount of our time to support. Which I'm very happy to do! But supporting one ABI version seems like the right tradeoff, especially given the changes in the last ABI change that @YannickJadoul noted above.

If you're interested in maintaining a fork longer-term, I'd be happy to direct people there, perhaps from our README or FAQ. Also, it could serve as an experiment- if it turns out after a year or so that it's not much work for you to maintain (aside from the rebases), that's a pretty good signal that we could just merge it in mainline :)

@YannickJadoul
Copy link
Member

YannickJadoul commented Jun 17, 2020

I like that idea, actually, of keeping that fork and seeing where things go from there. PyPy wheels (and them being able/allowed to be uploaded to PyPI) are still quite young, so this would give us more perspective and see how fast things evolve in the future. Again, happy to help rebasing that fork, when necessary, just tag me in some issue or PR is necessary.

How does that sound, @hyperair?

@hyperair
Copy link
Author

How does that sound, @hyperair?

Sounds good to me. Shall I close this PR then?

@YannickJadoul
Copy link
Member

As you wish; if you prefer to use this PR to not set up all the CIs yourself, that's also fine with me.

I don't know how @joerick feels about that, but we could do something similar to #382, keeping a branch here such that you can take advantage of the CI build we have set up. But at the same time, it might also be more useful to you to have control over your own fork's CI services? So I'll leave this up to your judgement :-)

@joerick
Copy link
Contributor

joerick commented Jun 23, 2020

I'd be happy for you to use a long-running PR if it's useful @hyperair. Up to you :)

@joerick joerick closed this Jul 10, 2020
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.

5 participants