-
Notifications
You must be signed in to change notification settings - Fork 402
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 example of c calling rust #74
Add example of c calling rust #74
Conversation
Is the buildkite presubmit intended to not be publicly visible (with an account)? |
I think that's a design shortcoming. The detail page there lands on an administrative page. FWICT, you can see the build results without logging in. |
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.
Perhaps we should have a more principled way of organizing these examples? For example, I'd expect to see the "call C from rust" example in here as well. What do you think about moving the matrix example into this directory (as "rust_calling_c") in this PR?
examples/ffi/c_calling_rust/BUILD
Outdated
srcs = ["main.c"], | ||
deps = [":wrapper"] | ||
) | ||
|
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: Repo style is no extra newline (this file and rest)
@@ -0,0 +1,20 @@ | |||
load("@//rust:rust.bzl", "rust_library", "rust_test", "rust_binary") |
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: Other examples have a visibility defined:
package(default_visibility = ["//visibility:public"])
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.
Should we weaken the rest of the examples' visibilities?
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'm not excited about anyone depending on this or other examples
I didn't move the matrix example because I had already made changes to it in #61, I can follow suit in that pr |
fd61799
to
8cbf0c5
Compare
Updated #61 to include "rust_calling_c" rename as well. |
@acmcarther Anything else for this one? |
LGTM, merging! |
Addresses #55.