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

Makes PathBuf FromPyObject implementation work on all os.PathLike #3374

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Aug 8, 2023

PyOS_FSPath is in abi3-py36

@Tpt Tpt force-pushed the pathlike branch 2 times, most recently from 40e250e to 3474366 Compare August 8, 2023 20:02
@@ -37,6 +37,7 @@ The table below contains the Python type and the corresponding function argument
| `decimal.Decimal` | `rust_decimal::Decimal`[^5] | - |
| `ipaddress.IPv4Address` | `std::net::IpAddr`, `std::net::IpV4Addr` | - |
| `ipaddress.IPv6Address` | `std::net::IpAddr`, `std::net::IpV6Addr` | - |
| `os.PathLike ` | `PathBuf`, `Path` | `&PyString`, `&PyUnicode` |
| `pathlib.Path` | `PathBuf`, `Path` | `&PyString`, `&PyUnicode` |
Copy link
Member

Choose a reason for hiding this comment

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

Isn't pathlib.Path also an os.PathLike, i.e. doesn't the first line basically include the second one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I left the two lines for explictness because I assumed a lot of people are not used to os.PathLike. If you prefer I can remove the pathlib.Path line.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would merge them and maybe use "pathlib.Path, os.PathLike" for the first column to stay explicit. But let's see what the other maintainers think.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of upending this table anyway and describing it from the Rust types (what they accept and what they convert to), so I'm ok with leaving this as two rows for now.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks - please accept the new licensing change in #3108 before we can merge this.

@Tpt
Copy link
Contributor Author

Tpt commented Aug 9, 2023

@davidhewitt Thank you! Done.

@adamreichold adamreichold added this pull request to the merge queue Aug 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 9, 2023
@Tpt
Copy link
Contributor Author

Tpt commented Aug 9, 2023

Pypy 3.10 fails with "undefined reference to `PyOS_FSPath'". What is strange is that it's supposed to be supported by Pypy 3.7+: https://doc.pypy.org/en/latest/release-v7.3.0.html#python-3-6-c-api

@adamreichold
Copy link
Member

Is it possible that we need to link this under a different name using a PyPy prefix (we do this for many other symbols, c.f. e.g. #3040)? cc @mattip

@davidhewitt
Copy link
Member

Yes, on all supported PyPy versions it looks like it is exported under the PyPy prefix: https://github.com/PyO3/python3-dll-a/blob/main/src/libpypy3-c.def#L449

(I would love to catch cases where our definition is wrong in pyo3-ffi-check, but so far I didn't work out a way to do this, probably rustdoc metadata is insufficient to do so.)

@Tpt
Copy link
Contributor Author

Tpt commented Aug 9, 2023

Thank you! I have added the link_name attribute for Pypy.

(I would love to catch cases where our definition is wrong in pyo3-ffi-check, but so far I didn't work out a way to do this, probably rustdoc metadata is insufficient to do so.)

Dumb idea: if Pyo3 is supposed to cover all/most symbols expose al PyPy* symbol one can just grep each of them to check if there are present in PyO3 source code. This might allow to cover link names.

@adamreichold adamreichold added this pull request to the merge queue Aug 9, 2023
Merged via the queue into PyO3:main with commit 1df7270 Aug 9, 2023
30 checks passed
@Tpt Tpt deleted the pathlike branch August 17, 2023 07:48
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.

None yet

3 participants