Closed ahesford closed 11 months ago
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
The patch applies cleanly to the 5.4.1 tag.
Hi @ahesford @SoftwareApe do we have timeline when would this fix be merged? Thanks!
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?
@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.
@ingydotnet @nitzmahone @perlpunk Hello, anyone from the maintainers can help?
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.
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.
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.
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
@willofferfit: The issue seems to be a reluctance to touch 5.x branch releases, see https://github.com/yaml/pyyaml/pull/726#issuecomment-1640397938.
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.
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.
I don't give a hoot about PyYAML 5.4.x, but it would be good to fix the build of stuff on master
and all currently supported release branches in a way that can use Cython 3 but doesn't require it. Pending a complete rethinking of the installation process, this is the simplest solution.
@SoftwareApe if you can resurrect a GHA wheel build environment that will rebuild every 5.4.x native wheel we have now and prove that they function (including dumping all the Windows DLL loads under the old Pythons to ensure we haven't introduced multiple CRTs/heaps), I'm happy to ship them. But I'm not willing to sign up to do that work a second time. I don't see what Ubuntu 22.04 has to do with the discussion- there are any number of trivial ways that distro packagers can continue to ship PyYAML 5.4 until the heat death of the universe that don't require any support from us.
@ahesford master
on this project is ... I don't even know what- maybe @ingydotnet can explain. I guess it's supposed to track the last release from the newest release branch... It seems to just be an attractive nuisance, because the only time things get merged there directly is by mistake.
Beyond our (very limited) extension test suites, we don't really know if Cython3 breaks anything else, so until we're ready to do the proper packaging rewrite and stop supporting < 3 (or 3 supports a Python version/feature we need that <3 does not), I'd rather not increase the expected compatibility to basically everything.
I can merge the contents of the 6.0.1 tag to master if it makes anyone feel better.
This matter has already taken more attention than I intended to give it. In general, I abhor upper bound restrictions on dependencies unless they convey specifically known breaks in compatibility. Using them as a way to signify "we've tested up to this version" causes all sorts of headaches for people trying to make packages coexist. This is particularly true for distribution packagers like me.
I'll spare you my usual rant about how the Python packaging ecosystem fosters these kinds of versioning headaches (on both ends). Do what makes sense for your project, and I'll carry a patch for Void if we choose to bump Cython earlier than you wish to support it.
@ahesford oh yes, I'm painfully aware of the Python-ecosystem-wide issues :wink: and as I mentioned on other threads, we're going to have this problem x1000 when setuptools
starts killing off ancient deprecated build properties still used by tons of older Python projects with uncapped setuptools
build deps (including this one). Every Python project that doesn't cap its setuptools
will eventually be incapable of building without some sort of assistance, and they're starting to kill that stuff off in a couple months.
It is a sad world where it is more painful to deploy fixes than develop them and that is what holds them back. But I do agree with @nitzmahone that setuptools
will soon break everything that uses anything beyond the forward supported APIs.
It seems to me though that a 5.5 release that supports 5.x API/semantics but only supports Python 3.10+ (jettisoning Python 2 support and wheels, etc.) would be a fair middle ground.
It seems to me though that a 5.5 release that supports 5.x API/semantics but only supports Python 3.10+ (jettisoning Python 2 support and wheels, etc.) would be a fair middle ground.
If that's possible we'd be quite happy, since we're on 3.10 anyway.
I can merge the contents of the 6.0.1 tag to master if it makes anyone feel better.
Yes, that would actually be desirable, if it's easy to do.
(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.
@nitzmahone: I totally agree and I was just researching the best ways to accomplish such. To that end I posted https://github.com/cython/cython/issues/4568#issuecomment-1660788127 asking them to help improve the documentation for build-time only dependency usage of Cython.Build.build_ext
(vs. Cython.Build.cythonize
).
Thank you for this simple solution. It's a shame that https://github.com/yaml/pyyaml/pull/702 was a preferred "fix". Given the importance of this project it truly needs better management.
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...
Just tested this PR. On top of it it needs to be added
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,3 +1,3 @@
[build-system]
-requires = ["setuptools", "wheel", "Cython<3.0"]
+requires = ["setuptools", "wheel", "Cython"]
build-backend = "setuptools.build_meta"
With that patch I was able to build pyyaml
however looks like pytest is failing
Part of those errors are caused by something still missing in build env for that module (can someone hel what exactly it is? 🤔) but some are not.
It is yet another minor issue. pep517 based build shows some cython related warnings
+ /usr/bin/python3 -sBm build -w --no-isolation
* Getting build dependencies for wheel...
running egg_info
creating lib/PyYAML.egg-info
writing lib/PyYAML.egg-info/PKG-INFO
writing dependency_links to lib/PyYAML.egg-info/dependency_links.txt
writing top-level names to lib/PyYAML.egg-info/top_level.txt
writing manifest file 'lib/PyYAML.egg-info/SOURCES.txt'
cythoning yaml/_yaml.pyx to yaml/_yaml.c
/usr/lib64/python3.9/site-packages/Cython/Compiler/Main.py:381: FutureWarning: Cython directive 'language_level' not set, using '3str' for now (Py3). This has changed from earlier releases!
File: /home/tkloczko/rpmbuild/BUILD/pyyaml-6.0.1/yaml/_yaml.pxd
tree = Parsing.p_module(s, pxd, full_module_name)
@kloczek This pull request was closed several months ago. Please report your issues elsewhere.
@kloczek This pull request was closed several months ago. Please report your issues elsewhere.
Can you point where I should report that? 🤔
FYI: This was effective merged in #808.
This is a de minimis fix for building with Cython 3. Recent Cython<3 releases provided
Cython.Distutils.build_ext
as an alias toCython.Distutils.old_build_ext.old_build_ext
; Cython 3 drops this alias and instead uses a wholly newCython.Distutils.build_ext
that does not provide thecython_sources
function used insetup.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 ofold_build_ext
), the import falls back to justCython.Distutils.build_ext
.This PR supersedes #729 and should be preferred for reasons discussed in my comments there.