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

fix(c-api) wasi_version_t wasn't bound correctly to C #2058

Merged
merged 11 commits into from
Jan 28, 2021

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Jan 26, 2021

Description

Because the enum variants weren't in uppercase and because one variant (InvalidVersion) has a non-literal value, cbindgen was ignoring the variants. This patch fixes that. It's not a breaking changes since the WASI version constants weren't present in the wasmer_wasm.h file before.

Note: u32::max_value() is soft-deprecated, we should use u32::MAX instead. Notice that none of them works with cbindgen, so I've hardcoded the value of u32::MAX -1 (now) here.

Review

  • Add a short description of the the change to the CHANGELOG.md file

@Hywan Hywan force-pushed the fix-c-api-wasi-version branch from 55ff142 to 434285c Compare January 26, 2021 12:11
@Hywan Hywan requested a review from MarkMcCaskey January 26, 2021 12:13
@Hywan Hywan marked this pull request as ready for review January 26, 2021 12:13
@Hywan Hywan requested a review from jubianchi as a code owner January 26, 2021 12:13
@Hywan Hywan self-assigned this Jan 26, 2021
@Hywan Hywan added bug Something isn't working 📦 lib-c-api About wasmer-c-api labels Jan 26, 2021
@Hywan Hywan changed the title fix(c-api) wasm_version_t wasn't bound correctly to C fix(c-api) wasi_version_t wasn't bound correctly to C Jan 26, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
@Hywan
Copy link
Contributor Author

Hywan commented Jan 27, 2021

bors r+

bors bot added a commit that referenced this pull request Jan 27, 2021
2058: fix(c-api) `wasi_version_t` wasn't bound correctly to C r=Hywan a=Hywan

# Description

Because the enum variants weren't in uppercase _and_ because one variant (`InvalidVersion`) has a non-literal value, `cbindgen` was ignoring the variants. This patch fixes that. It's not a breaking changes since the WASI version constants weren't present in the `wasmer_wasm.h` file before.

Note: `u32::max_value()` is soft-deprecated, we should use `u32::MAX` instead. Notice that none of them works with `cbindgen`, so I've hardcoded the value of `u32::MAX` here.

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Ivan Enderlin <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jan 27, 2021

Build failed:

@Hywan
Copy link
Contributor Author

Hywan commented Jan 28, 2021

Error on Windows (for the record):

D:\a\wasmer\wasmer\tests\integration\cli/../../../lib/c-api\wasmer_wasm.h:105:18: error: typedef redefinition with different types ('uint32_t' (aka 'unsigned int') vs 'wasi_version_t')
    typedef uint32_t wasi_version_t;
                     ^
    D:\a\wasmer\wasmer\tests\integration\cli/../../../lib/c-api\wasmer_wasm.h:70:6: note: previous definition is here
    enum wasi_version_t {

Hywan added 2 commits January 28, 2021 09:11
On Windows, using a `u32` representation for `wasi_version_t` fails if
a C++ compiler is used to treat a C program. So we change our strategy
here. We use a C representation to be FFI-safe, and the
`INVALID_VERSION` variant is now set to -1 instead of `u32::MAX`.

This patch also adds unit tests for `wasi_get_wasi_version` so that we
are sure of the behavior of this `type` on all platforms.
@Hywan
Copy link
Contributor Author

Hywan commented Jan 28, 2021

7a435ef

On Windows, using a u32 representation for wasi_version_t fails if
a C++ compiler is used to treat a C program. So we change our strategy
here. We use a C representation to be FFI-safe, and the
INVALID_VERSION variant is now set to -1 instead of u32::MAX.

This patch also adds unit tests for wasi_get_wasi_version so that we
are sure of the behavior of this type on all platforms.

@Hywan
Copy link
Contributor Author

Hywan commented Jan 28, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 28, 2021

@bors bors bot merged commit 3d6e4ad into wasmerio:master Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-c-api About wasmer-c-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants