-
Notifications
You must be signed in to change notification settings - Fork 127
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
Make Rust bindgen
build dependency optional
#382
Conversation
Now that header definitions are stable across builds (ethereum#377), we can remove the Rust `bindgen` build dependency by gating it to an optional feature. This also transitively removes the dependency on `libclang` for Rust bindings.
d2b989e
to
4cc13b5
Compare
4cc13b5
to
f158e0a
Compare
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.
Hey @DaniPopes, I like this change, thanks!
I was unfamiliar with linguist-generated
before. Looks like it will suppress diffs for GitHub PRs, which I think is a good idea. Would appreciate @asn-d6's opinion before merging though.
Yep, and excludes from statistics like the language percentages Just realized that this fails on Windows because the I'd disable this for Windows because it's pretty minor, but let me know what I should do |
Ah Windows... It would be nice if this worked for all platforms. My first thought is to force the enum to be unsigned in C. It's not ideal, but this should work: --- a/src/c_kzg_4844.h
+++ b/src/c_kzg_4844.h
@@ -94,9 +94,10 @@ typedef Bytes48 KZGProof;
/**
* The common return type for all routines in which something can go wrong.
+ * Note, values are large to force enum type to be an unsigned integer.
*/
typedef enum {
- C_KZG_OK = 0, /**< Success! */
+ C_KZG_OK = 0x80000000, /**< Success! */
C_KZG_BADARGS, /**< The supplied data is invalid in some way. */
C_KZG_ERROR, /**< Internal error - this should never occur. */
C_KZG_MALLOC, /**< Could not allocate memory. */ Or this, which I think it pretty ugly but it's more clear what's going on: /**
* The common return type for all routines in which something can go wrong.
*/
typedef uint32_t C_KZG_RET;
/**
* The possible return values for C_KZG_RET.
*/
enum {
C_KZG_OK = (C_KZG_RET)0,
C_KZG_BADARGS = (C_KZG_RET)1,
C_KZG_ERROR = (C_KZG_RET)2,
C_KZG_MALLOC = (C_KZG_RET)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.
LGTM! Thanks!
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.
LGTM 🥇 thanks again!
Now that header definitions are stable across builds (#377), we can remove the
Rust
bindgen
build dependency by gating it to an optional feature. This alsotransitively removes the dependency on
libclang
for Rust bindings.