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 WIT.md package ids to match #222 #246

Merged
merged 1 commit into from
Sep 15, 2023
Merged

Update WIT.md package ids to match #222 #246

merged 1 commit into from
Sep 15, 2023

Conversation

lukewagner
Copy link
Member

#222 generalized registry identifier syntax to allow both nested namespaces (a:b:c) and nested packages (a:b/c/d), but didn't update WIT.md to match. This PR generalizes WIT interface names accordingly. It is still a TODO to expand WIT to allow expressing the new cases of implementation imports.

@alexcrichton
Copy link
Collaborator

Syntactically this looks good to me, but I'm pausing considering the more semantic implications of a change like this. These aren't entirely opaque to WIT for example because you can have:

package foo:bar

interface baz {
    // ...
}

and somewhere else:

use foo:bar/baz.{t1, t2}

where the / syntax is an implicit projection from a package. With this, however, if the resolver is faced with a:b:c/d/e/f I'm not sure how that would end up being resolved? It seems that as-written that'd be a package a:b:c/d/e with an interface/world f within that, but I wanted to confirm that's intended.

Additionally is there an idea perhaps on how to restrict filesystem layout? For example should the above be required to be something like:

a:b:c/
  root.wit - `package a:b:c`
  d/
    root.wit - `package a:b:c/d`
    e/
      root.wit - `package a:b:c/d/e`

Or similarly you'd have wit/deps/a:b:c/d/e/root.wit using wit-parser's current folder-based structure.

All-in-all I think I'd personally prefer to see an implementation of all of this end-to-end to suss out all the various points this needs to integrate first. That being said I do realize that this is "just" a synchronization of the binary format and WIT so it's probably not the right place to block on too much here. Basically I am not certain about this and I'd prefer to see something more end-to-end to get a better feeling for how easy it would be to generalize this syntax. Personally I prefer to do that before updating the repo but I also think it's totally ok to land this here as it hypothetically would have the 🦄 marker indicating that it's not actually implemented yet.

So overall this seems ok to land, but I'd at least personally consider it in the 🦄 category until it's implemented fully in wasm-tools and/or wasmtime

@lukewagner
Copy link
Member Author

Yep, what you're suggesting is what I was imagining (except perhaps namespaces turn into directories too?). I don't think there's any urgency to have this implemented in a Preview 2 timeframe, just some modest value in illustrating future extension points to aid discussion in the registry setting, so an emoji prefix like we're discussing in #229 makes sense.

@lukewagner
Copy link
Member Author

Do you suppose it's fine to merge this now and add the emojis (and new section explaining The Plan and documenting each emoji) as part of the a future PR to address #229?

@alexcrichton
Copy link
Collaborator

Certainly!

Copy link
Collaborator

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

oh sorry forgot this

@lukewagner lukewagner merged commit 673d5c4 into main Sep 15, 2023
2 checks passed
@lukewagner lukewagner deleted the update-wit-ids branch September 15, 2023 14:54
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