Skip to content

Cython Workaround#2911

Merged
CoolCat467 merged 4 commits intopython-trio:masterfrom
CoolCat467:cython-workaround
Jan 5, 2024
Merged

Cython Workaround#2911
CoolCat467 merged 4 commits intopython-trio:masterfrom
CoolCat467:cython-workaround

Conversation

@CoolCat467
Copy link
Member

@CoolCat467 CoolCat467 commented Dec 21, 2023

This pull request partially completes changes that were noted in #2908.

Either added on to this pull request or in another pull request, we still need to make sure to test that cython will continue working and not find these sorts of issues after they are already a problem.

Excerpt from original comment in #2908:

But it would be good to also run a test for this in CI. We do have a test for the interpreter's built-in C coroutines (test_async_function_implemented_in_C) and I [njsmith] originally assumed that would be enough, but obviously it wasn't, so I guess we need to do the legwork to install cython in CI and build a tiny Cython module.

@codecov
Copy link

codecov bot commented Dec 21, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e317d9c) 99.67% compared to head (00c9be5) 99.67%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #2911   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files         115      115           
  Lines       17308    17308           
  Branches     3109     3109           
=======================================
  Hits        17252    17252           
  Misses         38       38           
  Partials       18       18
Files Coverage Δ
src/trio/_core/_run.py 100.00% <100.00%> (ø)

A5rocks
A5rocks approved these changes Jan 1, 2024
View reviewed changes
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Looks good, I think actually doing the CI work can be done later (just repurpose the already made issue).

@CoolCat467 CoolCat467 requested review from Zac-HD and njsmith January 1, 2024 19:33
@Zac-HD
Copy link
Member

Zac-HD commented Jan 3, 2024

I'd be OK with shipping this as a fix for the current problem, but I'm also keen to get some regression tests into our CI so we can tell that it's effective and notice if it breaks later. Maybe @jakkdl can help out?

@CoolCat467 CoolCat467 mentioned this pull request Jan 3, 2024
Closed
2 tasks
@jakkdl
Copy link
Member

jakkdl commented Jan 5, 2024

I'd be OK with shipping this as a fix for the current problem, but I'm also keen to get some regression tests into our CI so we can tell that it's effective and notice if it breaks later. Maybe @jakkdl can help out?

I'll take a look at it, seems quite doable. But agree on merging this and getting CI setup in a later PR.

@CoolCat467
Copy link
Member Author

CoolCat467 commented Jan 5, 2024

Ok, with two approvals I am going to merge this.

@CoolCat467 CoolCat467 merged commit eb32952 into python-trio:master Jan 5, 2024
@CoolCat467 CoolCat467 deleted the cython-workaround branch January 5, 2024 17:50
@jakkdl jakkdl mentioned this pull request Jan 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jakkdl jakkdl jakkdl approved these changes

@A5rocks A5rocks A5rocks approved these changes

@njsmith njsmith Awaiting requested review from njsmith

@Zac-HD Zac-HD Awaiting requested review from Zac-HD

Assignees

No one assigned

Labels

low-level

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@CoolCat467 @Zac-HD @jakkdl @A5rocks