-
Notifications
You must be signed in to change notification settings - Fork 824
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
feat(c-api) Add the WASMER_VERSION*
constants, and the wasmer_version*
functions
#1916
Conversation
bors try |
lib/c-api/src/lib.rs
Outdated
/// # } | ||
/// ``` | ||
#[no_mangle] | ||
pub extern "C" fn wasmer_version() -> *mut c_char { |
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 it be *const c_char
?
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.
same as @syrusakbary, why a *mut
?
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.
So that the user doesn't need to cast it when freeing it. That's stupid, but I think it's a better ergonomic.
tl;dr: It avoids writing this:
const char* version = wasmer_version();
free((char*) version); // this cast
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.
We'll need to fix the free
bug before we can ship this but otherwise looks good
lib/c-api/src/lib.rs
Outdated
/// printf("%s", version); | ||
/// | ||
/// // Free it, as we own it. | ||
/// free(version); |
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.
From the docs:
The pointer which this function returns must be returned to Rust and reconstituted using CString::from_raw to be properly deallocated. Specifically, one should not use the standard C free() function to deallocate this string.
I don't think we can ever mix C and Rust ownership: CString
is an owned type on the Rust side, we can't pass it back to C without providing our own destructor. Probably simpler to have the user pass us a buffer or to return a pointer to a static string which does not need to be freed
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.
Note: I did the comments as an approval by mistake.
Actually, I think there might be a better solution than a function that returns a string.
Since we are generating the headers at build time, we can check what the version is for the crate, and add that as a constant string variable in the generated header file.
Thoughts @Hywan @MarkMcCaskey @jubianchi ?
Let's use: static const char * WASMER_VERSION = "1.0.0"; |
Yeah, that makes sense, though we should probably use a C macro: #define WASMER_VERSION "version-number" because |
Actually, this has one major draw back... perhaps we want to do both, but it's nice to have a standard API for querying which version of Wasmer you're running against. This will be important with 1.0.0 when we have more claims to stability and users may ship code that dynamically links against the Wasmer C API and may want to query what version it's running against |
Yup, that's the same I was thinking. Let's do it via macro and static string then? |
Yeah, a static string works but the static has to be defined on the Rust side, not the C side, or it has the same problem as the macro |
Of course there is! This PR was a kickstart for the discussion :-). |
I wonder whether it's required to add a C function for that. I've added the following C constants, and I believe the user can query everything that is common with those: #define WASMER_VERSION "1.0.0-beta1"
#define WASMER_VERSION_MAJOR 1
#define WASMER_VERSION_MINOR 0
#define WASMER_VERSION_PATCH 0
#define WASMER_VERSION_PRE "beta1" Note that Thoughts? |
13d38f3
to
2ce2c9c
Compare
wasmer_version
functionWASMER_VERSION*
C constants
bors try |
This was not addressed @Hywan |
When the `.h` files aren't accessible, it is useful to get a function to retrieve the version of the Wasmer C API.
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.
Seems good but:
- What about having
wasmer_version_major()
,wasmer_version_minor()
,wasmer_version_patch()
? - We also have to expose the pre-release part if we want a "full" API;
WASMER_VERSION*
C constantsWASMER_VERSION*
constants, and the wasmer_version*
functions
bors r+ |
Fix #1534.
Description
This PR adds thewasmer_version
. It compute a string based on theCARGO_PKG_VERSION
environment variable value at compile-time. I know an enum could be better, but it's a start.This PR adds 4 C constants:
Thay way, the user can query the version of Wasmer more easily.
In addition, it adds 4 functions (when the header files aren't accessible):
Review