Updates to better support ormsgpack on GraalPy. #5121
Conversation
timfel
force-pushed
the
tim/ormsgpack-graalpy-updates
branch
3 times, most recently
from
160af8f to
5f8690c
Compare
|
I'm not sure what to do about the total code coverage. With the added test, the patch coverage reports 100% of the diff is hit, but the overall coverage went down? How is that possible? |
There was a problem hiding this comment.
Thanks for this. The coverage has a known bug in #5080, which I'm working to debug.
I have added CI-build-full label which you'll need for the GraalPy test run anyway, the coverage should match main with that label applied.
Main comment which I have is that I'd prefer we did not expose _PyLong_Sign.
timfel
force-pushed
the
tim/ormsgpack-graalpy-updates
branch
from
5f8690c to
2c9ff86
Compare
pyo3-ffi/src/cpython/bytesobject.rs
Outdated
| #[cfg(not(Py_LIMITED_API))] | ||
| #[inline] | ||
| pub unsafe fn PyBytes_AS_STRING(op: *mut PyObject) -> *const c_char { | ||
| #[cfg(not(any(PyPy, GraalPy)))] | ||
| return &(*op.cast::<PyBytesObject>()).ob_sval as *const c_char; | ||
| #[cfg(any(PyPy, GraalPy, Py_LIMITED_API))] | ||
| return crate::PyBytes_AsString(op); | ||
| } |
There was a problem hiding this comment.
Please do not put the shim here, put it in https://docs.rs/pyo3-ffi/latest/pyo3_ffi/compat/index.html
There was a problem hiding this comment.
Also, this most likely does not do what you intend, since cfg(Py_LIMITED_API) is never true inside this function.
There was a problem hiding this comment.
Also, this most likely does not do what you intend, since cfg(Py_LIMITED_API) is never true inside this function.
True, the entire function shouldn't be there on the limited API, I'll just drop this inside the function.
Please do not put the shim here, put it in https://docs.rs/pyo3-ffi/latest/pyo3_ffi/compat/index.html
I'm not sure I follow. The docs for the compat module say "Some CPython C API functions added in recent versions of Python are inherently safer to use than older C API constructs. This module exposes functions available on all Python versions that wrap the old C API on old Python versions and wrap the function directly on newer Python versions.". PyBytes_AS_STRING is neither new nor safe. Should it really be in the compat module if the docs then give the impression that it's a "newer" and "safer" API than alternative?
In my mind PyBytes_AS_STRING is really analogous to things like PyTuple_GET_ITEM etc, which is in cpython/tupleobject.rs, so cpython/bytesobject.rs seems to make sense here imo
There was a problem hiding this comment.
All that being said - I don't mind dropping it entirely if you think it shouldn't be there at all. I've just seen it used in both ormsgpack and orjson so I thought there's demand for this (and I wanted to try and minimize the diff I'll propose to those projects to support GraalPy)
There was a problem hiding this comment.
What you've written here is a shim, and we want this part of pyo3-ffi to correspond to the cpython headers as much as possible. So either the conditonal compilation inside this function shouldn't exist or it should be in the compat module.
timfel
force-pushed
the
tim/ormsgpack-graalpy-updates
branch
from
2c9ff86 to
75198eb
Compare
timfel
force-pushed
the
tim/ormsgpack-graalpy-updates
branch
from
75198eb to
e12a0b8
Compare
pyo3-ffi/src/object.rs
Outdated
| @@ -147,13 +147,13 @@ pub struct PyVarObject { | |||
| // skipped private _PyVarObject_CAST | |||
|
|
|||
| #[inline] | |||
| #[cfg(not(all(PyPy, Py_3_10)))] | |||
| #[cfg(not(all(GraalPy, PyPy, Py_3_10)))] | |||
There was a problem hiding this comment.
This looks wrong, it's impossible to have graalpy && pypy.
Probably meant to be
| #[cfg(not(all(GraalPy, PyPy, Py_3_10)))] | |
| #[cfg(not(any(GraalPy, all(PyPy, Py_3_10))))] |
pyo3-ffi/src/object.rs
Outdated
| #[cfg_attr(docsrs, doc(cfg(all())))] | ||
| pub unsafe fn Py_Is(x: *mut PyObject, y: *mut PyObject) -> c_int { | ||
| (x == y).into() | ||
| } | ||
|
|
||
| #[cfg(all(PyPy, Py_3_10))] | ||
| #[cfg(all(GraalPy, PyPy, Py_3_10))] |
There was a problem hiding this comment.
Similar I guess
| #[cfg(all(GraalPy, PyPy, Py_3_10))] | |
| #[cfg(any(GraalPy, all(PyPy, Py_3_10)))] |
timfel
force-pushed
the
tim/ormsgpack-graalpy-updates
branch
from
e12a0b8 to
57349c3
Compare
* Expose the vectorcall functions also on GraalPy * Add PyBytes_AS_STRING, seen in ormsgpack * Call Py_Is function on GraalPy
* Expose the vectorcall functions also on GraalPy * Add PyBytes_AS_STRING, seen in ormsgpack * Call Py_Is function on GraalPy
I noticed these things mostly when trying to run ormsgpack on GraalPy, but I think they are useful in general, so putting it out here to get some feedback.