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

Replace old_build_ext with new_build_ext #4498

Merged
merged 20 commits into from
Dec 20, 2021

Conversation

matusvalo
Copy link
Contributor

@matusvalo matusvalo commented Dec 15, 2021

This PR adds options from old_build_ext to new_build_ext. Moreover, now cython.Build.built_ext points to new_build_ext.

Fixes #3541

@matusvalo
Copy link
Contributor Author

For now it is work in progress.

@matusvalo matusvalo marked this pull request as ready for review December 17, 2021 14:14
@matusvalo
Copy link
Contributor Author

Ready for review. Maybe the question is about tests. I suppose they are needed.

Cython/Distutils/build_ext.py Outdated Show resolved Hide resolved
Cython/Distutils/build_ext.py Outdated Show resolved Hide resolved
Cython/Distutils/build_ext.py Outdated Show resolved Hide resolved
Cython/Distutils/build_ext.py Outdated Show resolved Hide resolved
Cython/Distutils/build_ext.py Outdated Show resolved Hide resolved
Cython/Distutils/build_ext.py Outdated Show resolved Hide resolved
Cython/Distutils/build_ext.py Outdated Show resolved Hide resolved
@matusvalo
Copy link
Contributor Author

Added some additional tests, updated docs, fixed some small issues. Please let me know what is missing in this PR to be accepted.

@matusvalo
Copy link
Contributor Author

The only CI fails seems unrelated to this PR, maybe connected to #4503? Is there anything else to be added to this PR?

@matusvalo
Copy link
Contributor Author

@scoder seems that last commit 5c1c2c4 broke the CI

@scoder
Copy link
Contributor

scoder commented Dec 20, 2021

@scoder seems that last commit 5c1c2c4 broke the CI

Yes, I'm already working on it. There is a circular import between setuptools and Cython that is rather difficult to resolve. I'll see what I can do.

@scoder
Copy link
Contributor

scoder commented Dec 20, 2021

The CI failures also apply to master and other branches, so I'll assume they are unrelated – although I had hoped that the changes would actually help with them.

@scoder scoder merged commit c6f5c5d into cython:master Dec 20, 2021
@scoder
Copy link
Contributor

scoder commented Dec 20, 2021

Thanks @matusvalo !

@nitzmahone
Copy link

nitzmahone commented Jan 13, 2022

@scoder @matusvalo This change killed some nasty dynamic build_ext subclassing that PyYAML's been doing since the dawn of time (see yaml/pyyaml#601 and https://github.com/yaml/pyyaml/blob/6.0/setup.py#L174-L238). The most immediate issue is just that the cython_sources method from old_build_ext that PyYAML's subclass was calling is no longer available- I tried a few different ways of patching it on with no success (various issues around missing instance data on either the command or the extension by the time get_source_files is called).

A couple of questions:

  1. Would you consider a reasonably simple change to emulate the cython_sources method from old_build_ext (to keep already-released PyYAML build_ext subclasses working on 3.0.0a10+ without modification)? That's assuming I can come up with one- so far, that's not looking good...
  2. Curious what the long-term plan is for old_build_ext? I can make things work again in future PyYAML releases without rewiring all the ancient build code just by swapping in old_build_ext (eg WIP keep PyYAML build working on Cython3.0.0a10+ yaml/pyyaml#602), but not sure how long it'll be around.

Thanks!

(I can open an issue for this, but if the answers to the above are "no" and "not very long", not much point 😉 )

@scoder
Copy link
Contributor

scoder commented Jan 13, 2022

I created #4568 to discuss this.

@Uzume
Copy link

Uzume commented Aug 1, 2023

@nitzmahone: pyyaml will likely have to move away from distutils with the advent of Python 3.12 later this year anyway. Why not just move from Cython.Distutils.old_build_ext or Cthon.Distutils.build_ext to Cython.Build.build_ext? Then pyyaml will not depend on cython_sources at all.

@jakirkham
Copy link
Contributor

Would it make sense to move that discussion to issue ( #4568 )

@nitzmahone
Copy link

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.

@jakirkham
Copy link
Contributor

Yeah the Python build story is definitely going through some upheaval 😅

Though do think we will be happier with those changes eventually (maybe after things settle a bit)

Was more wondering if this discussion made sense to track in an open issue (instead of a merged PR). Though maybe this info is already tracked in other places

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build_ext does not properly track dependencies (and should be deprecated / removed)
5 participants