Skip to content

No cythonize take 2#394

Closed
ocefpaf wants to merge 8 commits intoUnidata:masterfrom
ocefpaf:no_cythonize_take_2
Closed

No cythonize take 2#394
ocefpaf wants to merge 8 commits intoUnidata:masterfrom
ocefpaf:no_cythonize_take_2

Conversation

@ocefpaf
Copy link
Contributor

@ocefpaf ocefpaf commented Feb 24, 2026

Hopefully fix #379 and unlocks #372.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ ocefpaf
❌ jswhit2


jswhit2 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Feb 24, 2026

FYI this did not help #379. See my test where I tried both changes in https://github.com/ocefpaf/cftime/actions/runs/22368151216/job/64739567532

Comment on lines 12 to 16
# Cython 3.0.0 changes the default of the c_api_binop_methods directive to
# False, resulting in errors in datetime and timedelta arithmetic:
# https://github.com/Unidata/cftime/issues/271. We explicitly set it to
# True to retain Cython 0.x behavior for future Cython versions. This
# directive was added in Cython version 0.29.20.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most if not all of this comment could be removed if you're requiring Cython>=3.0 now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are merging this I'll fix that, but this did not fix what we hoped and #372 still fails with this change. I'm not sure if there are other reasons for this change.

@jswhit what do you think? Should we close or finalize this one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this doesn't fix #372, I don't think there is any reason to merge

@ocefpaf ocefpaf closed this Feb 26, 2026
@ocefpaf ocefpaf deleted the no_cythonize_take_2 branch February 26, 2026 10:23
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.

4 participants