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

BLD: Add Cython Coverage option #54453

Closed
wants to merge 5 commits into from
Closed

Conversation

lithomas1
Copy link
Member

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@lithomas1 lithomas1 added the Build Library building on various platforms label Aug 7, 2023
@lithomas1 lithomas1 added this to the 2.1 milestone Aug 7, 2023
@lithomas1 lithomas1 marked this pull request as ready for review August 8, 2023 13:52
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this! 🙌

Do I just set the environment variable CYTHON_COVERAGE=1 to use it?

@lithomas1
Copy link
Member Author

You need to specify it as a option.

For debug builds and Cython Coverage enabled you can do

pip install -ve . --config-settings=editable-verbose=true --config-settings=setup-args=-DCYTHON_COVERAGE=true --config-settings=setup-args=-Dbuildtype=debug --no-build-isolation

@@ -325,7 +325,7 @@ cdef double round_trip_wrapper(const char *p, char **q, char decimal,

cdef void* buffer_rd_bytes_wrapper(void *source, size_t nbytes,
size_t *bytes_read, int *status,
const char *encoding_errors) noexcept:
const char *encoding_errors) noexcept nogil:
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed to mark this as nogil or we would segfault since the tracing code would try to use GIL requiring code (and this function would be called in a nogil block).

I'm pretty sure the C code can acquire the GIL for itself when it needs it so nogil isn't needed?

cc @WillAyd

Copy link
Member

Choose a reason for hiding this comment

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

Yea it looks like the C code does ensure the GIL. I'm a little unclear then though - isn't marking this nogil incorrect since it actually uses the GIL?

@lithomas1 lithomas1 removed this from the 2.1 milestone Aug 23, 2023
@github-actions
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Oct 18, 2023
@lithomas1 lithomas1 removed the Stale label Dec 6, 2023
@mroeschke
Copy link
Member

Closing to clear the queue, but feel free to reopen if you have time to circle back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Library building on various platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants