yaml / pyyaml

Canonical source repository for PyYAML
MIT License
2.47k stars 507 forks source link

Make setup.py work with Cython 3 and Cython 0.29 #729

Closed SoftwareApe closed 11 months ago

SoftwareApe commented 11 months ago

By calling Cythonize directly we can make setup.py work for both versions of Cython.

This fixes #601 for PyYAML 5.4.1, it probably also works with PyYAML 6.0.1. But please apply this patch to version 5. We desperately need a fix here, since we can't just change all of Ubuntu Jammy to be compatible with PyYAML 6.

ahesford commented 11 months ago

This change will likely break existing workflows because it elevates Cython to a hard build dependency. In its current form, Cython is optional. It also injects the hard dependency in a way that makes automatic dependency resolution impossible; users who don't already have Cython installed will just have their builds fail.

Making this change also essentially obviates all if the custom build and extension stuff altogether, so a fix like this ought to just remove all of that extra complexity.

If this is meant as a quick hack, just do this instead. This preserves the full spirit of the original flow and just forces Cython 3 to use what was the default for build_ext in Cython 0.29 anyway. (To make this more robust, you should probably trap an import error on old_build_ext and, if that fails, try importing Cython's build_ext to fall back to existing behavior.)

SoftwareApe commented 11 months ago

@ahesford the pyproject.toml already puts a hard dependency on Cython, so it doesn't add additional requirements here. Also the setup.py is even simplified by this change. Your solution looks fine to me, too, though, as long as it's applied to pyyaml 5.4.x.

ahesford commented 11 months ago

The fact that it's in pyproject.toml means that modern PEP517 workflows will properly resolve dependencies, but legacy builds using setup.py directly will have dependency resolution broken without more care. And setup.py isn't simplified by your change, because it has all of the extra complexity around opportunistic Cython handling and automatic cythonization that is rendered obsolete. If you're going to move the cythonization into the main routine, you should strip out all of the unnecessary vestiges that are left behind.

SoftwareApe commented 11 months ago

@ahesford ok, didn't know about this. Could you be more specific about the vestiges? Otherwise I think a try-catch would do here to only cythonize if possible.

But like I said if your solution works for Cython 3, I'm not against using that instead. Is Cython guaranteeing that it will keep this old_build_ext or will that break on the next change?

Can you open a PR with your changes or add them here?

ahesford commented 11 months ago

Almost all of setup.py is built around optional use of Cython; if Cython is made mandatory, most of the file can just be thrown away.

731 is my minimal fix that is backwards compatible and will continue to work with subsequent Cython 3 releases until they decide to remove old_build_ext.

SoftwareApe commented 11 months ago

Closing in favor of #731, please apply the fix to the 5.4.x version range as well, that's where we need it.

Uzume commented 11 months ago

@nitzmahone: It seems like the real solution is to avoid the deprecated distutils and Cython.Distutils so another possibility is to switch from Cython.Distutils.old_build_ext or Cthon.Distutils.build_ext and cython_sources to Cython.Build.build_ext. As per PEP 632, distutils has been deprecated since Python 3.10 and is removed from Python 3.12. According to PEP 693, Python 3.12 is due to be released later this year on 2023-10-02 so this will have to be fixed quite soon anyway.