Skip to content

Drop the re-exports of PyMem_Raw* calls as those are not available using the limited API. #283

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

Merged
merged 3 commits into from
Mar 3, 2022

Conversation

adamreichold
Copy link
Member

Noticed this when preparing to upgrade an internal project at work. I opted to remove the re-exports for now as I do not see an easy option to make their presence dependent on whether PyO3 was built against the limited API or not.

@adamreichold
Copy link
Member Author

This must definitely be resolved before releasing 0.16 as the limited API is otherwise unusable with rust-numpy.

@davidhewitt
Copy link
Member

You can use pyo3_build_config::use_pyo3_cfgs in a build script to get the same cfgs we use in pyo3_ffi.

It should be publicly visible, looks like we have a docs bug.

@adamreichold
Copy link
Member Author

You can use pyo3_build_config::use_pyo3_cfgs in a build script to get the same cfgs we use in pyo3_ffi.

We can do that, but I am not sure this is the right thing to do: This will add the build time costs of a build script to this crate to enable re-exports which were made mainly for consistency with the C headers, not because of a concrete use case. (And if that use case has emerged in the mean time, it will have problems working with the limited API even if we make the re-exports available conditionally.)

@adamreichold
Copy link
Member Author

It should be publicly visible, looks like we have a docs bug.

I think the problem is that the resolve-config feature of pyo3-build-config is not enable for builds on docs.rs?

@adamreichold
Copy link
Member Author

You can use pyo3_build_config::use_pyo3_cfgs in a build script to get the same cfgs we use in pyo3_ffi.

I added a commit implementing this, but as written above I would prefer to not merge it due to the unclear use case of these re-exports. (Someone who does really need these can still use them directly from PyO3 after looking into the NumPy headers.)

@davidhewitt
Copy link
Member

Ok, I understand now. Sorry for my short response before - let's go back to removing these re-exports (we can always leave a comment in npyffi/array.rs explaining why we don't re-export them).

@adamreichold
Copy link
Member Author

we can always leave a comment in npyffi/array.rs explaining why we don't re-export them

Will do an explicit revert to leave a paper trail and add such a comment.

@adamreichold adamreichold force-pushed the fix-limited-api branch 2 times, most recently from da23e3e to 489aa26 Compare March 3, 2022 20:48
…t where to find the underlying PyO3 functions.

This reverts commit 712626b.
@adamreichold adamreichold merged commit 4b14d1d into main Mar 3, 2022
@adamreichold adamreichold deleted the fix-limited-api branch March 3, 2022 21:24
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.

2 participants