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

High level Capsule support #169

Merged
merged 11 commits into from May 30, 2019

Conversation

Projects
None yet
2 participants
@gracinet
Copy link
Contributor

commented Jan 10, 2019

Python Capsules are the preferred way to export / import C-level APIs between extension modules (see https://docs.python.org/3/c-api/capsule.html). I believe them to be key for incremental switch to Rust of existing C extensions.

The first commit introduces the PyCapsule struct (based on symbols already provided by python-sys), the two subsequent ones add macros that take care of the direct function pointer case and caching using sync::Once on a static mut

gracinet added some commits Dec 28, 2018

PyCapsule struct and implementation
This introduces a higher level (cpython) wrapper for Python capsules.
The rustdoc example demonstrate how to use them for static structs of
data and functions as exposed within the standard library.
py_capsule_fn! macro for direct function capsules
The `py_capsule_fn!` macro will make it easier (and safe) to
retrieve directly a function from a capsule.

Indeed the function type has to be specified on the fly, and
it wouldn't be safe to `transmute` without being sure we are
working on a function type. The retrieved function itself is
unsafe, like any function obtained through FFI should be.

The macro also adds a caching layer, using a static mut at the
top of the module it defines
py_capsule! macro for the "data" case
We believe it's slightly simpler to use, and also provides the
same kind of caching as `py_capsule_fn!` does.
@markbt
Copy link
Collaborator

left a comment

This looks reasonable to me. I've put a few suggestions for improvement, but I don't have much to add.

Because of #167, you'll need to update this to support Rust 2018 macros (see the PR for the link to the method for doing that).

Show resolved Hide resolved src/objects/capsule.rs Outdated
/// The arguments of this macro are
/// - the segments of the full dotted name of the targetted capsule,
/// - `$rustmod`, a suitable module name,
/// - `$ruststruct`, the name of the target `struct`.

This comment has been minimized.

Copy link
@markbt

markbt Jan 10, 2019

Collaborator

It's not clear to me from this description how to actually call this - there are 4 arguments to the macro, as far as I can tell. I think a better version of this would be:

/// This macro takes:
/// - The fully-dotted name of the module that contains the target capsule,
/// - The name of the target capsule,
/// - A rust module name, which will contain the retrieval function, and
/// - The name of the `repr(C)` struct contained within the capsule.

See also my comments below about perhaps a better way of writing the macro syntax.

This comment has been minimized.

Copy link
@gracinet

gracinet May 16, 2019

Author Contributor

Reformulated for the new macro syntax

Show resolved Hide resolved src/objects/capsule.rs Outdated
Show resolved Hide resolved src/objects/capsule.rs Outdated
/// This macro takes the following arguments:
/// - segments of the full Python dotted name of the capsule
/// - `$rustmod`, a suitable name to define a module
/// - signature of the wished function (the macro will insert the `extern unsafe fn`)

This comment has been minimized.

Copy link
@markbt

markbt Jan 10, 2019

Collaborator

As above, maybe:

/// This macro takes:
/// - The fully-dotted name of the module that contains the target capsule,
/// - The name of the target capsule,
/// - A rust module name, which will contain the retrieval function, and
/// - The signature of the function contained in the capsule.

This comment has been minimized.

Copy link
@gracinet

gracinet May 16, 2019

Author Contributor

Reformulated for the new macro syntax.

Show resolved Hide resolved src/objects/capsule.rs Outdated
Show resolved Hide resolved src/objects/capsule.rs Outdated

gracinet added some commits Jan 11, 2019

Rustdoc improvements for the function pointer case
In particular, since `py_capsule_fn!` is meant to be the main
entry point for this case, I felt compelled to restate the assumption
about pointer sizes.
@gracinet

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

@markbt thanks for the review ! For now, I'm making a new commit for each of the changes you requested, but I think, I'll rewrite the whole branch at the end in three commits (struct, data macro, fn macro) at the end of the process.

I'm getting rid of the trivial changes right now, but the macro changes will take me a longer while.

gracinet added some commits May 11, 2019

More self-explanatory syntaxes for the capsule macros
This follows suggestion by @markbt, with the slight difference
of `py_capsule` using `for` instead of the semicolon.

Perhaps, since the capsule doesn't become exactly the Rust module
the `as` should also be rephrased as `into`:

py_capsule!(from dotted.name import caps into rustmod for RustStruct)
Avoid 'impl Trait' arguments
Although there are signs that rust-cpython is preparing for
2018 edition, Travis tests are still running with 1.25, which
doesn't support 'impl Trait'
More Rust 1.25 fixes
- `c_void` must be imported from libc only (don't know how it worked
   in my current 1.31 anyway)
- unless there's an explicit main(), the doctest engine wouldn't
  put the `extern crate` declarations at toplevel.
@gracinet

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

@markbt what do you think of the present state (especially the macro syntax) ?
Still need to rebase and squash in but 3 commits, also maybe add that the defaulting of the Rust struct to the capsule name.
I don't think there are many changes to be made for the macros declarations, because none of these is internal but I may be wrong on that part (hard to tell, with the current master not compiling yet with edition="2018")

Insisting on `static` for capsule creation from Rust
Creating a capsule pointing to some `const` definitely does
not work: that's what `static` is meant for.

Most of the creation methods had been written with mutable raw
pointers, leading to cast incompatibilities for actual `static`
data. They could have been worked around, but it's better to
insist on immutable pointers and refs from the ground up.

Chased all unnecessary occurrences of `*mut` and `&mut` away
in the process.

@gracinet gracinet force-pushed the gracinet:capsule branch from 7498a9a to a9b6965 May 16, 2019

@gracinet

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@markbt

I've been using this in both directions in the concrete case of Mercurial. This a for now in an unsubmitted topic yet but it works for me, and we'd need to rely on it quite soon now.

The present macro syntaxes are:

py_capsule!(from some.py.mod import something as rustmod for RustStruct)
py_capsule_fn!(from some.py.mod import fun as rustmod signature (a: c_int) -> c_int)

I've made another branch for 2018 edition that shows that the macros don't need to be adapted, if you want to check it: https://github.com/gracinet/rust-cpython/tree/2018

@gracinet

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

A next step (probably a subsequent PR) would be to have default values for RustStruct (in the data case) and rustmod in the function case:

py_capsule!(from some.py.mod import something as rustmod)
py_capsule!(from some.py.mod import something as rustmod for RustStruct)
py_capsule!(from some.py.mod import something as rustmod fn(a: c_int) -> c_int)

with a 3-form py_capsule! macro deferring to two inner macros: py_capsule_data! and py_capsule_fn!

This would probably need some adaptation for the 2018 edition, although I'd plan to make the two helpers public, too (at least for backwards compatibility).

My take on this is that it can probably wait until rust-cpython switches to the 2018 edition. Perhaps my above mentionned branch can serve as a baseline work for this, too.

In py_capsule_fn!, using 'signature' token instead of colon
I feel it's even more explicit and probably more reliable to
parse correctly.

@gracinet gracinet force-pushed the gracinet:capsule branch from a9b6965 to 4f67c18 May 16, 2019

@markbt

markbt approved these changes May 30, 2019

Copy link
Collaborator

left a comment

Yep, this looks like a good first version for capsule support.

@markbt markbt merged commit 156c8a4 into dgrunwald:master May 30, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.