-
Notifications
You must be signed in to change notification settings - Fork 172
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 cpp-to-wasm test #968
Add cpp-to-wasm test #968
Conversation
Update: I've fixed the allocation issue. The layout issue remains. original bug detailsThe runtime error (for both files) is:
which sounds like i need to set up memory infra somehow |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Codecov Report
@@ Coverage Diff @@
## main #968 +/- ##
==========================================
+ Coverage 74.89% 75.00% +0.11%
==========================================
Files 221 216 -5
Lines 12811 12737 -74
==========================================
- Hits 9595 9554 -41
+ Misses 3216 3183 -33 Continue to review full report at Codecov.
|
Pull Request Test Coverage Report for Build 88639083c982d3f0dc657a4830b1e8ac01848d05-PR-968
💛 - Coveralls |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
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 100% sure I see where this is going yet.
@@ -0,0 +1,1300 @@ | |||
<!doctype html> |
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: Where did this giant HTML and JS file come from? Please add a copyright header with more info, or replace it with a smaller file that you write yourself.
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 accidentally checked them in, they shouldn't be checked in. My bad.
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.
They all come from Emscripten, to be clear.
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.
Emscripten is kinda annoying to use, Rust's WASM story is very good (and Rust moves its boilerplate into separate crates), emcc on the other hand produces a lot of boilerplate at once because it's nowhere near as good.
ffi/capi/src/wasm_glue.rs
Outdated
fn calloc(num: usize, size: usize) -> *mut u8; | ||
fn realloc(ptr: *mut u8, size: usize) -> *mut u8; | ||
} | ||
unsafe impl GlobalAlloc for WasmMalloc { |
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.
Question: why not dlmalloc?
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.
It doesn't work! I didn't bother to debug it.
ffi/capi/src/wasm_glue.rs
Outdated
fn malloc(size: usize) -> *mut u8; | ||
fn free(ptr: *mut u8); | ||
fn calloc(num: usize, size: usize) -> *mut u8; | ||
fn realloc(ptr: *mut u8, size: usize) -> *mut u8; |
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.
Question: are these imports implemented anywhere? I don't see them in main.js
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.
Wasm implements them (or at least, emcc does). When you build C/++ code with emcc it's already hooked up to an allocator, exposed as the regular malloc/free stuff.
Filed rust-lang/rust#88152 upstream |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
I finally got it working. Aside from the diplomat update, it should be reviewed afresh, a lot has changed (hence the force-pushes, but I also wanted to get rid of files that got accidentally checked in) Marking as a draft since this won't work until serde-rs/serde#2076 lands anyway. Also I need to get the WASM CI working (and convert this into |
denylist = ["bench", "wearos", "freertos", "x86tiny", "smaller_static"] | ||
|
||
[lib] | ||
crate-type = ["staticlib", "rlib", "cdylib"] | ||
crate-type = ["staticlib", "rlib"] |
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.
dylib had to be removed because it doesn't work for emscripten (without what seems to be a large amount of extra effort),
we're not using cdylibs anyway, and it's still possible to compile icu_capi to a cdylib, just not in the emscripten context, and unfortunately rust-lang/cargo#4881 doesn't exist.
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
Finally works! It's still not mergeable because of the serde patch, but it's ready for review! cc @sffc (I squashed up most of the CI commits because I was trying a lot of things that didn't work) |
And we have a new serde version! Usually when you are relying on a newer version of a crate you should specify that version in Cargo.toml (not just Cargo.lock) so that users will also pull in at least that version. However in this case we need the dependency bump only for a single CI test for a strange platform, and the updated dependency is something users of that platform will need anyway, so I only changed Cargo.lock. |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Rebased over #974 |
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
.github/workflows/build-test.yml
Outdated
@@ -229,7 +229,11 @@ jobs: | |||
- name: Load nightly Rust toolchain for WASM. | |||
run: | | |||
rustup install nightly-2021-02-28 | |||
rustup install nightly |
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: Pin a version of nightly because nightly versions are frequently flaky.
While you're at it, try bumping the 2021-02-28 version to see if that test starts running again in a newer nightly. Then maybe you can use the same, newer nightly for both wasm32-unknown-unknown and wasm32-unknown-emscripten.
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.
That would be nice! I'll try
|
|
Okay, so updating the nightly doesn't work, pinning everything to nightly-2021-02-28. We should try to do a followup to fix things, because once rust-lang/rust#85499 lands we'll want to update to a rust including that so that some of the yoke bugs are fixed. |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
This is progress towards #967 , allowing others to work on debugging the issues here.
This compiles, but I have not yet added it to CI since it's not working correctly.
The current state shows the following warning:
This seems to be a return type thing (cc @shadaj, if you're interested / have time). Might be worth trying to switch to DiplomatResult, but either way the struct should work.
It's possible that my way of linking things is not ideal either.