-
-
Notifications
You must be signed in to change notification settings - Fork 163
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
Consider making cython a build time dependency #457
Comments
It's set up to be optional atm: https://github.com/aio-libs/yarl/blob/master/setup.py#L70-L74. |
That is based on if we are in cpython or not Lines 13 to 16 in d592009
If the extensions are going to be built, it is from the pre-cythonized c files Line 19 in d592009
so even if the user is using cpython and is using cython, there is no way to have it automatically re-cythonize the the pyx files via |
Ah, right I forgot. We had to move out cythonize. It was there earlier. But then pip introduced build isolation and it became impossible to rely on the user to install Cython in the same env. The current vision is that most folks will get wheels but those who won't, should be able to build things w/o having to bring cython in (because its installation may also fail). Besides that, it should be possible to opt-out of compiling c-extensions at all. I see your point but I'm not in a position to decide whether to implement it. |
P.S. One of the problems is that if you declare it a build dependency, this would mean failing to install unconditionally w/o any chance to fallback to pure Python. |
The motivation for the build isolation is largely driven to allow you to specify (and have pip automatically install) the build dependencies. If you put up wheels this will only affect people who are installing from sdist (because they don't trust your wheels or are running on a system you don't have wheels for (such as cpython master branch)) so I think it is OK if they don't have a pure-python option or drop the pre-generated c files all together and fall back to pure-python. Right now it in the worst of all worlds where it just does not compile :( |
I rather prefer publishing a new release with updated Cython-generated files. For non-released versions, I think the pure-python fallback ( |
I do not disagree that doing a new release can be easy, but it is still a step that has to be done by someone in a reasonable time period after a new CPython release. I don't see the downside of an option that works without any developer intervention. Could a compromise be to make it a build time dependency only if the Python version is newer than the Python version used to generate the sdist / cythonized files? |
The project used cythonizing-on-the-fly. |
yarl 1.5.0 will be published with generated C files made by Cython 0.29.21 |
The currently published sdists are incompatible with CPython 3.11. |
Sure. CPython 3.11 is in deep alpha now. |
The cython 3.0.x branch does. I am just re-registering my complaint that this repo (along with a bunch of them from this org) were annoying to get building again due to (what feel to me to be non-standard) build-process choices rather than any actual incompatibilities. |
Again, it is supported by Cython==3.0.0a9 only. |
The sdist currently on pypi will not build with py310 due to some changes to the c-api (python/cpython#20429). Cython has already been updated to account for this (cython/cython#3639), however because the yarl sdist includes the output of cython against an older combination of cpython / cython they are not guaranteed to work with future versions.
Because yarl does publish wheels, in most cases users will not need to have cython install and it will "just work" out of the box, however in cases where there are not wheels it can be broken in ways that are impossible to fix via the pip cli. If users are in a situation where they can not (or do not want to) use the wheels then they will also need a compiler (which is much harder to get installed than cython these days).
The datrie projects is a pretty clean example of how to set this up: https://github.com/pytries/datrie/blob/master/setup.py
The text was updated successfully, but these errors were encountered: