-
Notifications
You must be signed in to change notification settings - Fork 426
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
python calling rust example #75
Conversation
I can't unblock this for CI. Could I bother you to push a commit (perhaps removing "debug stuff", unless you think you'll need it to debug OSX)? I think the ci pipeline forgot about this verification request and the link is now 404ing. |
I don't think it's right referring to the mangled solib path, but don't have a good idea of what The Right Way is yet. You just need any commit to re-trigger CI? |
Oops, didn't submit my comment! Yeah I just need a commit so that the CI sees it. |
c673849
to
a4f944a
Compare
import os | ||
from ctypes import cdll | ||
|
||
# TODO: This is a hack to compensate for rootpath not giving us the _solib_* path... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot quite put my finger on how hacky this is.. let me know what ya'll think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow yeah this is pretty scary.
I can't tell at a glance -- is this brittle w.r.t. where generated artifacts land? I don't much care either way since this is an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ultimately searches all of runfiles, so unless the design of collecting shared libraries in one location changes dramatically, it should be safe. That said, searching should really not be necessary and I'm not sure if it's a deficiency in python/c++ rules or my lack of knowledge here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've totally context switched from this, so I'm going to approach this in a really sterile way:
Including an example of something like this seems to me to be equivalent to suggesting that this is a good approach to do Py -> Rust FFI, and that we'll support such a thing into the future. Now, "support" may not be a real obligation on our part at all -- is it?
Also, if this is something you're interested in (personally or as an organization) can you informally commit to updating it if you discover a better/more "production-y" way to do it? I have vague nebulous concerns about stale examples.
#[no_mangle] | ||
pub extern fn triple_it(x: i32) -> i32 { | ||
x * 3 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit newline
@@ -0,0 +1,61 @@ | |||
# TODO: Native extensions are what we really want, cdll.LoadLibrary is not particularly great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe amend this file name with "example" so that it isn't viewed as some hidden functionality? Unsure what users or readers would think.
I suspect that anyone interested in this functionality would copy+paste this code into their workspace -- ideally the code in condition enough for users who would do that. Judgement call for you.
crate_name = rust_lib.split(":")[-1] | ||
out = name + ".py" | ||
native.genrule( | ||
name = "__generate" + out, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit of nits: "generated"
import os | ||
from ctypes import cdll | ||
|
||
# TODO: This is a hack to compensate for rootpath not giving us the _solib_* path... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow yeah this is pretty scary.
I can't tell at a glance -- is this brittle w.r.t. where generated artifacts land? I don't much care either way since this is an example.
I did this mostly to see how difficult / complicated setting up the integration was, I think it's reasonably straightforward (and would be dead simple if the $(rootpath) gave the path we wanted). I think the best case scenario for this sort of integration is for python to have the capacity to take a header and a shared library and generate all of the boilerplate, and for rust to be able to produce the shared library and header (with cbindgen). IIUC this is roughly how milksnake works, as opposed to directly creating a python native extension. The API would look roughly the same as the current example, however would work for functions that aren't In general I don't have an immediate use for this, but I think it's nice to say "look, it's easy to use rust with xxx". It is okay with me for this to continue to sit in a pull request (it'll be a year old soon!), but I don't mind providing a similar level of guidance / support as with the rest of the rules. |
I would rather not have it sit in a PR forever. if it works and we can put it in CI, then you should dust it off to commit it. If it doesn't really work, how about turning it into examples/ffi/python_calling_rust/readme.md. There you can describe the approach and how a more disciplined solution might work. A PR is essentially undiscoverable. |
Will close this for now, should be just as easy to find. Would make sense to revisit when bazel-python has support for native extensions. |
This is not well supported by python rules, but sorta works. bazelbuild/bazel#1475 outlines the hacky setup.
Might also want to create a pyo3 or similar example outside of this repo.