Fix builds with Cython 3 #731

ahesford · 2023-07-21T14:14:11Z

This is a de minimis fix for building with Cython 3. Recent Cython<3 releases provided Cython.Distutils.build_ext as an alias to Cython.Distutils.old_build_ext.old_build_ext; Cython 3 drops this alias and instead uses a wholly new Cython.Distutils.build_ext that does not provide the cython_sources function used in setup.py.

Explicitly importing old_build_ext preserves the existing behavior for recent Cython<3 and uses the correct behavior for Cython 3. Should the import fail (e.g., because the version of Cython available predates the availability of old_build_ext), the import falls back to just Cython.Distutils.build_ext.

This PR supersedes #729 and should be preferred for reasons discussed in my comments there.

This is a *de minimis* fix for building with Cython 3. Recent Cython<3 releases provided `Cython.Distutils.build_ext` as an alias to `Cython.Distutils.old_build_ext.old_build_ext`; Cython 3 drops this alias and instead uses a wholly new `Cython.Distutils.build_ext` that does not provide the `cython_sources` function used in `setup.py`. Explicitly importing `old_build_ext` preserves the existing behavior for recent Cython<3 and uses the correct behavior for Cython 3. Should the import fail (*e.g.*, because the version of Cython available predates the availability of `old_build_ext`), the import falls back to just `Cython.Distutils.build_ext`. Signed-off-by: Andrew J. Hesford <[email protected]>

SoftwareApe · 2023-07-21T14:20:52Z

Thank you for providing a proper fix for the cython issue. Can this be applied to 5.4.1 as well? We desperately need a proper fix there, since we can't switch our OS to use 6.0.1

ahesford · 2023-07-21T14:28:11Z

The patch applies cleanly to the 5.4.1 tag.

git clean -xdf tar zcvf ../python-yaml_5.4.1.orig.tar.gz --exclude=.git . debuild -uc -us cp python-yaml.spec ../python-yaml_5.4.1-1.spec mv ../python*-yaml*5.4.1*.{gz,xz,spec,dsc} /osc/home\:alvistack/yaml-pyyaml-5.4.1/ rm -rf ../python*-yaml*5.4.1*.* See yaml#731 Signed-off-by: Wong Hoi Sing Edison <[email protected]>

git clean -xdf tar zcvf ../python-yaml_6.0.orig.tar.gz --exclude=.git . debuild -uc -us cp python-yaml.spec ../python-yaml_6.0-1.spec mv ../python*-yaml*6.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/yaml-pyyaml-6.0/ rm -rf ../python*-yaml*6.0*.* See yaml#731 Signed-off-by: Wong Hoi Sing Edison <[email protected]>

git clean -xdf tar zcvf ../python-yaml_6.0.1.orig.tar.gz --exclude=.git . debuild -uc -us cp python-yaml.spec ../python-yaml_6.0.1-1.spec mv ../python*-yaml*6.0.1*.{gz,xz,spec,dsc} /osc/home\:alvistack/yaml-pyyaml-6.0.1/ rm -rf ../python*-yaml*6.0.1*.* See yaml#731 Signed-off-by: Wong Hoi Sing Edison <[email protected]>

shuyangwu-dd · 2023-07-24T22:03:47Z

Hi @ahesford @SoftwareApe do we have timeline when would this fix be merged? Thanks!

praysml · 2023-07-25T04:21:16Z

Hi, we also need this fix as our docker image get broken and we can't use 6.0.1, is there any timeline when it will be merged?

SoftwareApe · 2023-07-25T06:08:23Z

@shuyangwu-dd @praysml we're in the same boat as you. I don't think either @ahesford or I are pyyaml maintainers. We're just hoping it's soon.

praysml · 2023-07-25T06:57:25Z

@ingydotnet @nitzmahone @perlpunk Hello, anyone from the maintainers can help?

Uzume · 2023-07-26T04:38:26Z

I find it interesting this PR basically revives the behavioral fix of #602.

I look forward to a new PR with this fix in it for a 5.x branch (fixing #724 and replacing #726) as well as this PR incorporating a reversion of #702 so that it will apply cleanly here on 6.x.

ahesford · 2023-07-26T04:42:11Z

I didn't see the prior attempt because it's a 1.5-year-old WIP that also hard requires Cython 3 when it shouldn't.

Uzume · 2023-07-26T04:51:22Z

I didn't see the prior attempt because it's a 1.5-year-old WIP that also hard requires Cython 3 when it shouldn't.

@ahesford: That is exactly why I made that comment. After reading issue #601 and its PRs #602, #702, #729 and this one as well as issue #724 and the duplicate issues #723 and #728 and the #702 backport PR #726, I thought a little overview might be useful here.

willofferfit · 2023-07-26T05:20:22Z

This PR fixed the issue for me in v5.4.1. It would be nice to get this merged officially and have a v5.4.1.post1 released

Uzume · 2023-07-26T07:11:36Z

@willofferfit: The issue seems to be a reluctance to touch 5.x branch releases, see #726 (comment).

nitzmahone · 2023-07-26T15:44:26Z

As I've mentioned several other places: getting a functional vaniilla Cython3 PEP517 build working on any branch from the past several years isn't the hard part, it's rebuilding all the existing wheels on 5.4.x- if we released this change without that, the majority of folks that use the wheels will suddenly break. The biggest (known) problem there is the Windows Python 2.x wheels, since Microsoft has killed off the downloads of the necessary SDK bits and related VS versions, as well as the Windows GHA runner versions that had (most) of those bits preinstalled. It took me a lot of screwing around to get all that stuff working properly a few years ago (eg, not loading multiple CRT versions and heaps in older Pythons that weren't built against the Universal CRT), and I'm not interested in doing it again. It's all possible if one spends enough time on it, but I'd much rather spend the limited time I have to donate to this project making it work better for the future instead of patching up long-dead releases for projects unwilling/unable to make the one-line code change to work with >=6.0.

SoftwareApe · 2023-07-26T15:48:49Z

long-dead releases for projects unwilling/unable to make the one-line code change to work with >=6.0

I'd argue Ubuntu 22.04 has a big enough install base not to consider 5.x long dead. And the problem with Python is not our unwillingness to change our code, but our inability to fix everybody's code.

Uzume

I think this is a far better solution than the applied #702 workaround in 6.0.1, however, pyyaml will soon have to move to an entirely different solution soon enough since this is all dependent on distutils (and Cython.Distutils) which according to PEP 632 has been deprecated since Python 3.10 and is removed from Python 3.12 which is due out 2023-10-02 (according to PEP 693).

A new solution that moves from the deprecated distutils and Cython.Distutils (including Cython.Distutils.old_build_ext or Cthon.Distutils.build_ext and cython_sources) will need to be developed. This means either:

moving to Cython.Build.build_ext
directly calling cythonize as per a Make setup.py work with Cython 3 and Cython 0.29 #729 style solution

Currently pyyaml intentionally makes Cython a build-time only dependency so that one need not be concerned about Cython during installs of prebuilt wheels, etc. It accomplishes that via with_cython.

Cython documents a similar method at Distributing Cython modules (which uses USE_CYTHON instead of with_cython) but only for the direct cythonize call method and not for Cython.Build.build_ext method (which is documented but only as a dependency all the time).

I believe a build-time only dependency on Cython.Build.build_ext could be developed by porting from the current build-time only dependency on Cython.Distutils.old_build_ext or Cthon.Distutils.build_ext (and cython_sources).

nitzmahone · 2023-08-01T17:17:24Z

(cross-posting my comment from the Cython issue):

I don't think we want to move the discussion back to Cython- this is all on us at this point to just redo PyYAML's build from a clean sheet of paper. That's been exactly the plan all along, but due to the sheer number of "knobs" provided by the old distutils mechanism to affect the build that had no way to be passed through (eg preprocessor defines, include dirs, linker directives, "just give me pure Python"), plus all the "organically-grown" logic that's crept in over the years (much of which is incomplete or plain broken today) we've been kinda stuck.

Now that pip and setuptools have (somewhat recently) agreed on passing through options via config_settings, it's "only" a matter of completely obliterating most of setup.py and coming up with the right set of knobs that can be expressed via config_settings to do everything that's needed, and rewriting it to (hopefully) just call Cythonize directly without the need to subclass anything. I've poked at it a couple times, and it still kinda sucks from a UX perspective, but not nearly as much as doing it all via envvars (which is what we've been doing since adding PEP517 support a few years back).

The other thing that keeps me in "analysis paralysis" mode on this project is that some folks are poking at a libfyaml extension, which would be awesome to integrate, but that probably needs to be accounted for in the build design as well. The current distutils stuff sorta supports dynamic selection/build of multiple extensions (though it's been just libyaml for so long that in reality it's probably busted anyway). Doing libfyaml support as a separate top-level package or a namespace package causes all sorts of cross-pollination problems with OS packaged libs and stuff, so IMO if we're going to do that, it really needs to happen all in the main package build, with the extensions being embedded as proper subpackages.

Uzume · 2023-08-01T17:51:21Z

@nitzmahone: I totally agree and I was just researching the best ways to accomplish such. To that end I posted cython/cython#4568 (comment) asking them to help improve the documentation for build-time only dependency usage of Cython.Build.build_ext (vs. Cython.Build.cythonize).

asavah · 2023-09-11T20:27:25Z

Thank you for this simple solution.
It's a shame that #702 was a preferred "fix".
Given the importance of this project it truly needs better management.

dimpase · 2023-09-29T12:40:06Z

As I've mentioned several other places: getting a functional vaniilla Cython3 PEP517 build working on any branch from the past several years isn't the hard part, it's rebuilding all the existing wheels on 5.4.x- if we released this change without that, the majority of folks that use the wheels will suddenly break.

Have you given promises to never release an incompatible version? Then you'd stay on version 5.4.x- forever...

git clean -xdf tar zcvf ../python-yaml_6.0.1.orig.tar.gz --exclude=.git . debuild -uc -us cp python-yaml.spec ../python-yaml_6.0.1-1.spec cp ../python*-yaml*6.0.1*.{gz,xz,spec,dsc} /osc/home\:alvistack/yaml-pyyaml-6.0.1/ rm -rf ../python*-yaml*6.0.1*.* See yaml#731 Signed-off-by: Wong Hoi Sing Edison <[email protected]>

git clean -xdf tar zcvf ../python-yaml_5.4.1.orig.tar.gz --exclude=.git . debuild -uc -us cp python-yaml.spec ../python-yaml_5.4.1-1.spec cp ../python*-yaml*5.4.1*.{gz,xz,spec,dsc} /osc/home\:alvistack/yaml-pyyaml-5.4.1/ rm -rf ../python*-yaml*5.4.1*.* See yaml#731 Signed-off-by: Wong Hoi Sing Edison <[email protected]>

git clean -xdf tar zcvf ../python-yaml_6.0.orig.tar.gz --exclude=.git . debuild -uc -us cp python-yaml.spec ../python-yaml_6.0-1.spec cp ../python*-yaml*6.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/yaml-pyyaml-6.0/ rm -rf ../python*-yaml*6.0*.* See yaml#731 Signed-off-by: Wong Hoi Sing Edison <[email protected]>