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

TST: Explicitly pass NumPy path to cython during tests (also speed them up) #25141

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

seberg
Copy link
Member

@seberg seberg commented Nov 14, 2023

This mainly injects the -I into the cython build in meson by fetching the actual numpy directory during the build to paper over gh-25135.

It clearly isn't a fix, since it is probably a Cython issue (or maybe an issue with the meson in-place build setup).

I also modified the building fixture to only run once, the tests are ridiculously slow otherwise (there were much fewer tests not long ago).

This papers over numpygh-25135, it doesn't fix it, since we are finding
the right NumPy, Cython should be finding the right pyd files also
(whether this is a NumPy or a Cython issue).

# TODO: This is a hack due to gh-25135, where cython may not find the right
# __init__.pyd file.
add_project_arguments('-I', npy_path, language : 'cython')
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the best way I found, I tested it locally with an in-place build at least...

Copy link
Member

Choose a reason for hiding this comment

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

What if you unconditionally add the current working directory? For any other use case, I think the current Cython logic does the right thing.

Copy link
Member

Choose a reason for hiding this comment

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

OTOH, I see the lines above also use the cd .. trick in order to get the C include path, so this is reusing that hack.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a feeling on what is best here. Wasn't this run in a temporary folder though? Escaping that might be annoying.

@seberg seberg added the 09 - Backport-Candidate PRs tagged should be backported label Nov 14, 2023
@mattip mattip merged commit b3d71f7 into numpy:main Nov 15, 2023
61 checks passed
@mattip
Copy link
Member

mattip commented Nov 15, 2023

Thanks @seberg

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