-
Notifications
You must be signed in to change notification settings - Fork 10
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 ffi creation #59
Conversation
Codecov Report
@@ Coverage Diff @@
## main #59 +/- ##
==========================================
+ Coverage 97.72% 97.83% +0.10%
==========================================
Files 9 9
Lines 969 968 -1
==========================================
Hits 947 947
+ Misses 22 21 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
e967e1c
to
a90002a
Compare
|
||
python: | ||
@head -n $(LINE) Cargo.toml > cargo/$@/Cargo.toml | ||
@echo "pyo3 = { version = \"0.18.0\", features = [\"abi3\", \"extension-module\"] }" >> cargo/$@/Cargo.toml |
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 was pretty gross but I was in a rush.
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.
Would be nice to use a toml parser or something to edit the section and re-export.
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.
Or, template everything.
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.
yeah add an issue so we track it I think it's fine for now
src/core/matter/mod.rs
Outdated
let result = if let Some(code) = code { | ||
let (raw, raw_size) = if raw.is_none() || raw_size.is_none() { | ||
return Err(PyValueError::new_err("code present, raw and raw_size missing")); | ||
} else { | ||
(raw.unwrap(), raw_size.unwrap()) | ||
}; | ||
|
||
Self::new_with_code_and_raw(code, raw, raw_size) | ||
} else if let Some(qb64) = qb64 { | ||
Self::new_with_qb64(qb64) | ||
} else if let Some(qb64b) = qb64b { | ||
Self::new_with_qb64b(qb64b) | ||
} else if let Some(qb2) = qb2 { | ||
Self::new_with_qb2(qb2) | ||
} else { |
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.
Someone should add validation if this gets merged, that multiple things aren't set. Example: qb2 and qb64 both specified = bad.
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 comment is only outdated because code moved, we should still do this.
a90002a
to
fe530c2
Compare
578062b
to
eb6efd2
Compare
|
||
use crate::core::util; | ||
use crate::error::{err, Error, Result}; | ||
|
||
pub mod tables; | ||
|
||
#[cfg_attr(feature = "python", pyclass)] |
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.
Just this line in the core code, and the required gated-use
.
132f2b4
to
c8496d6
Compare
221c1f7
to
321caad
Compare
321caad
to
7b93440
Compare
Side note: the tarpaulin action occasionally fails, we should look at using a make or shell command rather than the action. |
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.
🚀
cargo fix | ||
cargo fmt | ||
|
||
preflight: |
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 been wanting to do this !!
pub mod cigar; | ||
pub mod counter; | ||
pub mod diger; | ||
pub mod matter; |
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.
Does Matter want to be public? I wouldn't expect people to be initializing it as it's a base for the others?
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.
Possibly not, but in the Rust syntax we'd probably need a type because using a trait you initialize like this:
let verfer = <Matter as Verfer>::new_with_code_and_raw(/* ... */);
.. we may be able to alias all these? I can look into this in another issue.
|
||
python: | ||
@head -n $(LINE) Cargo.toml > cargo/$@/Cargo.toml | ||
@echo "pyo3 = { version = \"0.18.0\", features = [\"abi3\", \"extension-module\"] }" >> cargo/$@/Cargo.toml |
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.
yeah add an issue so we track it I think it's fine for now
@jasoncolburne I would consider usage of uniffi Rust crate for FFI bindings.
|
@Artemkaaas Thanks for this, it sounds less hacky than what I threw together! Someone mentioned it recently but it went past me and I didn't really know what it offered. I created a ticket for investigation: #65 |
Rationale
Wanted to set us up for success. I created the beginnings of the Rust API layer and the Python FFI. Comments welcome.
Also, this unlocks parallel streams of work and tasks with a lower bar to entry for the project.
Changes
Matter
class exposed.README.md
with instructions for new developers.I am pretty happy with how I cleaned up the directory structure after a few iterations and separated the python code from the main library. The only changes to the core library are to support a simple
#[pyclass]
above the definition of the struct.Testing
You are encouraged to check out the branch and try these commands.
Screenshots (updated README.md)
Here is a live link to a preview of this file.