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

cleanup: simplify main module resolution #612

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Conversation

zshipko
Copy link
Contributor

@zshipko zshipko commented Nov 28, 2023

This PR simplifies the resolution of the main module when multiple modules are provided. Before we would try to look at the path/URL when the wasm was coming from disk or via HTTP, now any module missing a name will be used as main. This is much nicer and more consistent since this is what was being done when no filename was available (i.e. raw data modules). It also make sense because non-main modules will need to be named for the functions to be linked correctly.

@nilslice
Copy link
Member

nilslice commented Nov 28, 2023

does this impact anything in the go or js SDKs? cc/ @mhmd-azeez

(I mean in the sense that we need to make a similar change in those implementations)

@chrisdickinson
Copy link
Contributor

chrisdickinson commented Nov 28, 2023

@nilslice While the JS SDK supports loading multiple modules, it doesn't currently implement linking (extism/js-sdk#29). (I'd like to get that done as part of the post-beta/pre-release polish, though.) (Edit: In addition, we'll want to implement proper name derivation that matches libextism. Right now it guesses absent names based on hash or index & doesn't have a concept of a main.)

@nilslice
Copy link
Member

@chrisdickinson ok great, thanks! still probably worth a check on the Go SDK, but if I recall there is some existing special checking done to handle this.. just want to be sure we are consistent

@zshipko
Copy link
Contributor Author

zshipko commented Nov 28, 2023

Good point, I can take a look at the go-sdk too and if I can't figure it out will just open an issue.

chrisdickinson
chrisdickinson previously approved these changes Nov 28, 2023
Copy link
Contributor

@chrisdickinson chrisdickinson left a comment

Choose a reason for hiding this comment

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

LGTM! This is a nice refinement. The only open question I had was whether the kernel counted towards our minimum module count check. (Edit: I was misreading the module count check, apologies! This looks great.)

@zshipko
Copy link
Contributor Author

zshipko commented Nov 29, 2023

Merging this now and opened a PR to update the go-sdk: extism/go-sdk#42

@zshipko zshipko merged commit af4fd18 into main Nov 29, 2023
5 checks passed
@zshipko zshipko deleted the cleanup-main-resolution branch November 29, 2023 00:15
zshipko added a commit to extism/go-sdk that referenced this pull request Nov 29, 2023
- Update main module resolution to match
extism/extism#612
- Fix JSON de-serialization of `extism.Manifest` using the
`concreteManifest` type (et me know if there is a nicer way to do this
in Go)

---------

Co-authored-by: Muhammad Azeez <muhammad@dylibso.com>
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.

None yet

3 participants