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

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

Closed

Conversation

SoftwareApe
Copy link

@SoftwareApe SoftwareApe commented Jul 21, 2023

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.

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

ahesford commented Jul 21, 2023

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
Copy link
Author

@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
Copy link

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
Copy link
Author

SoftwareApe commented Jul 21, 2023

@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
Copy link

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
Copy link
Author

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

Copy link

@Uzume Uzume left a comment

Choose a reason for hiding this comment

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

@SoftwareApe: This is sort of a cool solution as it totally gets around cython_sources (and the need for Cython.Distutils.old_build_ext or Cthon.Distutils.build_ext), however, it seems to always require Cython breaking with_cython which allowed for installing prebuilt wheels without needing Cython. It seems like this solution could be improved to fix that.

It would be nice to use this solution but only at build time and not at install time. For example doing something more like Distributing Cython modules from the Cython documentation (which uses USE_CYTHON vs. with_cython) seems like a better solution.

@Uzume
Copy link

Uzume commented Aug 1, 2023

@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.

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.

3 participants