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

Implement merging semver-compatible interfaces in imports #1815

Merged

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Sep 23, 2024

This commit is the first half and the meat of the implementation of #1774 where, by default, WIT processing tools will now merge world imports when possible based on semver versions. The precise shape of how this is done has gone through a few iterations and this is the end result I've ended up settling on. This should handle all the various cases we're interested in and continue to produce valid worlds within a Resolve (with the help of elaborate_world added in #1800). CLI tooling has been updated with flags to configure this behavior but the behavior is now enabled by default.

Closes #1774

@alexcrichton
Copy link
Member Author

This is a draft PR at this time because changes are needed to wit-component to enable this to actually work. While interfaces are correctly merged at the WIT level they're not yet hooked up to core wasm that imports the "old" version. Once #1810 lands that connection will be much easier to implement and I plan on adding a follow-up with test cases here as well.

This commit is the first half and the meat of the implementation
of bytecodealliance#1774 where, by default, WIT processing tools will now merge world
imports when possible based on semver versions. The precise shape of how
this is done has gone through a few iterations and this is the end
result I've ended up settling on. This should handle all the various
cases we're interested in and continue to produce valid worlds within a
`Resolve` (with the help of `elaborate_world` added in bytecodealliance#1800). CLI
tooling has been updated with flags to configure this behavior but the
behavior is now enabled by default.
This commit implements the final bit of bytecodealliance#1774 where the `wit-component`
crate will now match imports based on semver version in addition to the
previous exact-name matching that was performed previously.
@alexcrichton alexcrichton marked this pull request as ready for review September 23, 2024 18:04
@alexcrichton
Copy link
Member Author

Ok and now with #1810 landed I was hoping this was ready to go. I realized though I should also test with the original .NET component that @dicej had to ensure this works. That's uncovering some more issues so this isn't actually quite ready yet.

This is now possible with import version matching so refactor the
internal data structures to better support insertion of possibly
duplicate shims. Most of wit-component was already ready for this
refactoring, it was just the initial generation of shims that needed to
be reorganized slightly.
Any interface could be modified, so elaborate all of them to fixup anything
that needs new imports.
This commit updates how the semver-merging bits of `Resolve` work by
moving it towards the end of the encoding process rather than
once-per-world-merged. That helps both deduplicate work and fix some
asserts I'm seeing that are tripping if a `Resolve` is import-merged and
then merged again elsewhere.

This additionally simplifies some APIs because the boolean of what to do
isn't threaded to quite so many locations.
Dependencies might depend on replaced interfaces so the dependency edges
there need to be updated in the same manner as imports.
@alexcrichton
Copy link
Member Author

Ok Joel you gave me a module.wasm awhile ago which raised this issue initially and I now get:

$ cargo run component embed --world proxy ../wasi-http-0.2.0/wit ./module.wasm | \
    cargo run component embed --world imports ../wasi-http-0.2.1/wit | \
    cargo run component new --adapt ./wasi_snapshot_preview1.reactor.wasm | \
    tee composed.wasm | \
    cargo run validate

all passes. A simple wasmtime serve of the component works as well and I also see:

$ wasm-tools component wit ./composed.wasm
package root:component;

world root {
  import wasi:cli/[email protected];
  import wasi:cli/[email protected];
  import wasi:cli/[email protected];
  import wasi:cli/[email protected];
  import wasi:cli/[email protected];
  import wasi:cli/[email protected];
  import wasi:cli/[email protected];
  import wasi:io/[email protected];
  import wasi:io/[email protected];
  import wasi:io/[email protected];
  import wasi:clocks/[email protected];
  import wasi:filesystem/[email protected];
  import wasi:filesystem/[email protected];
  import wasi:sockets/[email protected];
  import wasi:clocks/[email protected];
  import wasi:sockets/[email protected];
  import wasi:random/[email protected];
  import wasi:cli/[email protected];
  import wasi:cli/[email protected];
  import wasi:cli/[email protected];
  import wasi:http/[email protected];

  export wasi:http/[email protected];

}
...

where notably was:io/poll is only present at 0.2.1 and other interfaces are deduplicated.

Also ensure to at least try to test the new merging function.
@alexcrichton alexcrichton added this pull request to the merge queue Sep 24, 2024
Merged via the queue into bytecodealliance:main with commit 51ade65 Sep 24, 2024
30 checks passed
@alexcrichton alexcrichton deleted the merge-imports-with-versions branch September 24, 2024 18:01
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.

Deduplicate interfaces by version in wit-component instead of importing multiple versions
2 participants