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

Migrate to Cython3 #813

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Migrate to Cython3 #813

wants to merge 5 commits into from

Conversation

ZipFile
Copy link

@ZipFile ZipFile commented Aug 13, 2024

As title says. Major incompatibility with v0.29 was new mandatory dunder mangling, so solution was simply to rename them. Otherwise migration was pretty much straightforward.

Notes:

  • Cython 3 no longer recommends commiting C files. I already have some drafts, will submit PR after this one is merged.
  • So far it builds and runs on CPython 3.7-3.12 (with coverage).
  • On PyPy it won't build with DEPENDENCY_INJECTOR_DEBUG_MODE (probably Cython issue), without debug mode looks like it builds and runs fine (no coverage ofc), but there are few failing tests, need investigate more if this is related to version of PyPy itself (I was testing with 7.3.16).
  • Not exactly sure about segfaults mentioned in Roadmap 2024 #812, but majority of tests were failing because Cython 3 started mangling attributes that were previously unmangled.

@rmk135
Copy link
Member

rmk135 commented Aug 13, 2024

@ZipFile wow, thanks for the PR!

@rmk135 rmk135 self-assigned this Aug 13, 2024
@coveralls
Copy link

Coverage Status

coverage: 91.623%. remained the same
when pulling cb81e56 on ZipFile:cython3
into 2c998b8 on ets-labs:master.

@ZipFile
Copy link
Author

ZipFile commented Aug 13, 2024

Looks like tox was ignoring DEPENDENCY_INJECTOR_DEBUG_MODE env var set from GHA pipeline, coverage supposed to be around 94% with *.pyx included. Pushed fix + extra logging.

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.

3 participants