pyyaml icon indicating copy to clipboard operation
pyyaml copied to clipboard

WIP: Attempt to get pyyaml to build with cython 3.0

Open tacaswell opened this issue 2 years ago • 5 comments

This is a slightly different approach to fixing #601 than #602 which drops the sub-classes and attempts to simplify the build logic.

This is cribbed from h5py's setup_build.py https://github.com/h5py/h5py/blob/master/setup_build.py

While this works and the tests pass I think it is dropping some configuration control and support for other Pythons.

tacaswell avatar Jan 25 '22 03:01 tacaswell

Yeah, just getting the libyaml extension to build under Cython 3.0 is pretty straightforward, as you've shown here- our usage of Cython is fairly basic. However, changing this would break every packager that's still calling setup.py directly and passing pyyaml-custom args to it, and wouldn't provide a dynamic fallback to pure-Python pyyaml for users that aren't installing a wheel but don't have a working extension build env. I'm not saying that's the wrong approach, since setuptools has long deprecated directly calling setup.py, but we'd need to provide other methods for packagers and users to accomplish the same configurations (probably via envvars, given the current state of PEP517 config data), and preferably a deprecation period for packagers to direct them to use the new methods (optional, but kind).

Off the top of my head, the things we need to be able to support are:

  • setting custom defines
  • override of include/link dirs
  • static link options
  • Cythonize mode (force/optional/never)
  • extension build mode (force/optional/never)

I hacked envvar support for some of these into the existing setup.py code back when we added pyproject.toml, just so we could use a proper PEP517 build that wasn't pip for most of our own wheel build scenarios, but if we're completely eliminating the custom command extensions, we'll need that degree of flexibility first and make sure we've communicated the change in behavior to packagers somehow.

nitzmahone avatar Jan 31 '22 23:01 nitzmahone

However, changing this would break every packager

Fair, but in my experience with Matplotlib and h5py, down stream packagers adapt to even major changes in how to build a package so long as you provide clear examples of what they should do now.

wouldn't provide a dynamic fallback to pure-Python pyyaml for users that aren't installing a wheel but don't have a working extension build env

I agree that is a pretty show-stopping problem, as are the first 3 bullet points.

I think the cythonize mode + extension building should be grouped, getting cython is much easier than getting the needed complier and pre-cythonized c-code has a tendency to not be forward-compatible. A sdist that require cython is far more likely to be usable in the future than an sdist that has cythonized c-code and I am not convinced that the complexity of explaining / implementing how to force re-cythonization is worth the narrow case of someone who has the Python headers, libyaml, and a c-compiler but not cython (which can be pip installed!).

tacaswell avatar Feb 01 '22 02:02 tacaswell

A sdist that require cython is far more likely to be usable in the future than an sdist that has cythonized c-code

Agreed- we quit including Cython'd outputs in our sdists back when we added pyproject.toml and made Cython a build dep for those exact reasons. We'd gotten nailed a few times having to quick-turn new sdist releases to work with breaking changes to the Python APIs generated by older versions of Cython while trying to keep things working under pre-release Pythons.

nitzmahone avatar Feb 17 '22 16:02 nitzmahone

I'd like to target some form of this work, probably along with the more dynamic discovery of libyaml @karolyi has been working on in #618 for the next non-patch PyYAML release. I've been hoping the PEP517 --config-setting mess would converge among pip/build/setuptools before we tried to rip out all the old distutils custom command stuff, but it's not looking promising, so we may need to just go with a fully envvar-only or out-of-band config approach for now and retrofit it if/when there's a better mechanism in place.

nitzmahone avatar Feb 17 '22 17:02 nitzmahone

On the plus side, support for setuptools consuming PEP517/518 config settings has improved quite a bit since February- I haven't gone back to see if will cover all the things we need now, but I've used it for a couple little toy projects with success, so I'm much more optimistic that it could do what we need without having to manually patch everything with envvars...

nitzmahone avatar Nov 21 '22 22:11 nitzmahone