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

Update wasm-tools so that we can use latest wit-parser #13

Open
wants to merge 5 commits into
base: nomic-enhancements-release
Choose a base branch
from

Conversation

mjoerussell
Copy link

The fixes for whitespace in doc comments got merged into the head of wasm-tools, as you'd expect, but jco is a few versions behind. This PR updates the dependencies for wasm-tools and a few other necessary ones so that we can utilize the wit-parser fix.

In newer versions of wit-bindgen they've added new instructions for handling lowering async and futures. These changes haven't been implemented in bytecodealliance/jco yet, so where required those instructions have been stubbed out here. The only exception is Instruction::Flush in function_bindgen.rs. That instruction is being emitted when generating code for slang, so some kind of "real" implementation needed to be added. I went with the most minimal implementation I could come up with. I have no guarantee that this is how they intend this instruction to be handled, but I've tested it with infra ci and everything works (with one small exception that will be handled in slang itself).

…ush` so that slang's `infra ci` command succeeds.

There's no guarantee that this is the intended implementation of this instruction. However, I've check the generated js output for slang and didn't
notice any weird output.
@@ -2,18 +2,6 @@
# It is not intended for manual editing.
version = 3
Copy link

@OmarTawfik OmarTawfik Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with one small exception that will be handled in slang itself

was this the ComponentError commit we added here?
is there anything else missing?

Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Copy link

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of questions. Otherwise, LGTM!

Copy link

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one question. LGTM otherwise. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants