Add PyMarshal_* functions to ffi interface. #460
Conversation
There was a problem hiding this comment.
Python 2.7 is no longer supported, so I think the marshal.rs module can be added directly into the ffi/ folder, not the ffi3/ folder. The ffi3 folder exists for historical reasons, because originally Python 2 and 3 had their own completely separate implementations. The ffi folder was recently created for a few modules that just used conditional compile statements to support both, but I think new modules should go in ffi (and eventually the old modules should move there too).
|
@de-vri-es I think generally no tests are added because the ffi stuff is used by the safe rust layer, and that is tested. I'm not sure what the policy is on expanding the ffi without a corresponding change in the safe Rust layer. I think it would be better to have a safe rust layer, but if it doesn't make sense to add one, then maybe just test the ffi layer. |
|
Though I'm not against adding the marshal module, I have no idea what we can use it for 🤔 |
Codecov Report
@@ Coverage Diff @@
## master #460 +/- ##
==========================================
+ Coverage 87.83% 87.87% +0.03%
==========================================
Files 64 65 +1
Lines 3388 3398 +10
==========================================
+ Hits 2976 2986 +10
Misses 412 412
Continue to review full report at Codecov.
|
Moved :)
I added a safe wrapper for The rest of the ffi functions take a
@kngwyu: The use case I have in mind is a proc macro that compiles python code and serializes it. Then at runtime, the compiled code is deserialized and executed (currently in a PR at m-ou-se/inline-python#5). |
src/marshal.rs
Outdated
| /// | ||
| /// The built-in marshalling only supports a limited range of object. | ||
| /// See the [python documentation](https://docs.python.org/3/library/marshal.html) for more details. | ||
| pub fn dumps(py: Python, object: &PyObject, version: i32) -> PyResult<Vec<u8>> { |
There was a problem hiding this comment.
How about returning PyResult<&PyBytes>?
E.g.,
use crate::conversion::FromPyPointer;
...
FromPyPointer::from_owned_ptr_or_err(py, bytes)would work.
There was a problem hiding this comment.
Ah, that avoids the possibly pointless copy into a Vec. Nice :)
src/marshal.rs
Outdated
| } | ||
|
|
||
| /// Deserialize an object from bytes using the Python built-in marshal module. | ||
| pub fn loads(py: Python, data: &impl AsRef<[u8]>) -> PyResult<PyObject> { |
There was a problem hiding this comment.
It's better to return PyResult<&PyAny>, since it has downcast* methods.
FromPyPointer::from_owned_ptr_or_err would work here too.
|
Thanks, I left some comments but your implementation mostly looks good.
Currently we don't test all of FFI functions, so I don't think tests are absolutely necessary.
Wow it's exciting 😃 |
|
Thanks for the feedback! I believe I processed the requested changes.
As input the /edit: The macro is already working btw. Once this lands in a released version I can simplify the implementation a bit though. Currently I expose the ffi functions there locally. :) |
| /// | ||
| /// See the [python documentation](https://docs.python.org/3/library/marshal.html) for more details. | ||
| /// | ||
| /// # Example: |
|
Thanks for the change, I approved this PR. |
|
@de-vri-es |
|
Rebased on master. Though it first had a DNS failure in the appveyor environment, but a re-trigger got it past that :) |
This PR adds the
PyMarshal_*functions [1] to the ffi interface.This can be used to serialize/deserialize a limited set of python objects. Most interestingly for me, it can serialize code objects. That makes it possible to split compiling and running of python code between different processes, which can be very useful.
I haven't added any tests, mainly because it looks like there are no tests for the ffi modules.
[1] https://docs.python.org/3/c-api/marshal.html