-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
DNM: Ensure manual builds use at least the same minimum Cython version used to build our wheels #9876
base: master
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #9876 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #9876 +/- ##
=======================================
Coverage 98.71% 98.71%
=======================================
Files 118 118
Lines 36294 36309 +15
Branches 4315 4316 +1
=======================================
+ Hits 35826 35841 +15
Misses 315 315
Partials 153 153
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -1,6 +1,7 @@ | |||
[build-system] | |||
requires = [ | |||
"setuptools >= 46.4.0", | |||
"Cython >= 3.0.11", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't do anything since the current packaging implementation in aiohttp differs from the other libs. setuptools is configured to build from C-files. And those C-files have to exist. So the contributors have to call cythonize. And we call it when publishing. The end-users either get pre-built wheels or hit the sdist that contains those C-files.
So everything is dependent on whatever https://github.com/aio-libs/aiohttp/blob/9e3a328/requirements/cython.txt outputs at the time of release (this is also known to cause problems with older Cython-produced C-files not being compatible with bleeding-edge CPythons).
At some point, I'd like to change the build process but this is how things are right now.
Hence, we shouldn't add this here as it's never getting invoked in the current process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I wonder how build will go on Python distro where Cython is not available?
python for java, .net, whatever...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cython is not the problem. It can convert PYX into C with its pure-python code. I'd worry about the following step (calling GCC/CLANG).
#9871 (reply in thread) showed some warnings/errors that are fixed in newer
Cython
. We currently don't specify a minimumCython
version which means manual builds can result in broken builds if theCython
version being used for the build is too old.This PR should not merge until we confirm this is the issue in the discussion