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

Add machine-emulator native bindings for Rust #25

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

algebraic-dev
Copy link
Contributor

No description provided.

Copy link
Collaborator

@GCdePaula GCdePaula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

What are the best practices with FFIs in Rust? Should we commit the bindings.rs, or generate them in a build script (build.rs)? If it's the build script route, I guess we'd have to include the header files somewhere. I don't mind committing it for now, and later pulling it from a submodule.

I can never remember, what is the recommendation for committing Cargo.lock in Rust workspaces? Considering whether there are executables or just libs?

machine-bindings/cartesi-machine/src/errors.rs Outdated Show resolved Hide resolved
machine-bindings/cartesi-machine/src/errors.rs Outdated Show resolved Hide resolved
machine-bindings/cartesi-machine/src/errors.rs Outdated Show resolved Hide resolved
machine-bindings/cartesi-machine/src/ffi.rs Show resolved Hide resolved
@@ -0,0 +1,31 @@
//! Defines the [struct@Hash] type.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to use the same definition of Hash as Dave is using. Later, I think we could even bring your merkle data structure to this workspace.

@algebraic-dev
Copy link
Contributor Author

algebraic-dev commented Jan 24, 2024

Nice work!

What are the best practices with FFIs in Rust? Should we commit the bindings.rs, or generate them in a build script (build.rs)? If it's the build script route, I guess we'd have to include the header files somewhere. I don't mind committing it for now, and later pulling it from a submodule.

I can never remember, what is the recommendation for committing Cargo.lock in Rust workspaces? Considering whether there are executables or just libs?

Executables should commit the Cargo.toml, libraries shouldn't so I removed them.

I think that we can only generate the bindings using build.rs if we have the include files of libcartesi somewhere, I generated them from inside of the image so I think that's bad if we are developing outside of the image to generate it. I'm not sure of the conventions but I think that if can generate it without bloating the crate with a lot of header files it's better than keeping the generated file like that.

…orcodes and free error message immediately after copying it
Copy link
Collaborator

@GCdePaula GCdePaula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Can you give me some pointers on how to use this crate? I want to write a program in Rust that runs the machine.

machine-bindings/Cargo.lock Outdated Show resolved Hide resolved
machine-bindings/cartesi-machine-sys/build.rs Outdated Show resolved Hide resolved
machine-bindings/cartesi-machine/main.rs Outdated Show resolved Hide resolved
machine-bindings/cartesi-machine/src/errors.rs Outdated Show resolved Hide resolved
machine-bindings/cartesi-machine/src/errors.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@GCdePaula GCdePaula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✨✨

@algebraic-dev algebraic-dev merged commit 4029d88 into main Feb 14, 2024
1 check passed
@GCdePaula GCdePaula deleted the feature/rust-bindings branch March 23, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants