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

WIP: Attempt to get pyyaml to build with cython 3.0 #607

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tacaswell
Copy link

@tacaswell tacaswell commented Jan 25, 2022

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.

@nitzmahone
Copy link
Member

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.

@tacaswell
Copy link
Author

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!).

@nitzmahone
Copy link
Member

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

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

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

hswong3i added a commit to alvistack/yaml-pyyaml that referenced this pull request Jul 18, 2023
    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#607

Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>
This is cribbed from h5py's setup_build.py.

While this works and the tests pass, it drops a lot of the configuration that
used to be possible / support for other Pythons / setting the include path /
...
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.

None yet

2 participants