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

Explicit package declarations should require root package declaration #380

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

macovedj
Copy link
Contributor

@macovedj macovedj commented Jul 18, 2024

Recently, support for explicit package declarations was added to wit, enabling multiple package declarations in a single wit file. This PR proposes that using this new syntax should still require a "root" package declaration.

Today, mixing declarations below isn't yet supported. This PR proposes that it not only be supported, but required, though just as today, the top level declaration could appear in another file in a wit project directory.

package root:package;

package first:explicit {
  ...
}

package second:explicit {
  ...
}

With this requirement, we can retain the concept of the package that is defined by a wit project, while also being able to define multiple packages in a single project.

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.

Personally I'm in favor of this change in talking with @macovedj. It's been difficult to rationalize how tooling should be updated when the "thing you just parsed" returns a list of packages when there should probably just be a "main package" somewhere in there. I think that this change returns us back to that original state of having a "main package" at all times but we still have all the benefits of inline curly-braced package blocks for ease of use, reading, single-file storage, etc.

Rolling this change out will be a little weird on the wasm-tools side of things but I'm hoping that the usage of multi-package forms hasn't spread too too much.

Also cc @azaslavsky since you worked on the package .. { ... } forms as well

Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

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

This makes sense to me too, thanks for working through this.

design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
@azaslavsky
Copy link
Contributor

I'm generally on board with this change. Is there a companion PR with the implementation in the main wasm-tools repository? The reason I ask is that some tooling for producing WIT, like the pretty printer, might struggle to decide what the "main" package is when printing some set of packages for view. So we'd want to either provide a means to specify that, or else loosen the "must have a root" rule in such cases. Also, today we have users specify their "main" package on the command line when using the all-explicit mode, which should probably be deprecated, lest it collide with this new canonical method of "main" specification.

I suspect there are other similar tricky bits, so it would be good to have a working implementation to work out these corner cases alongside this spec change.

@macovedj
Copy link
Contributor Author

I'm generally on board with this change. Is there a companion PR with the implementation in the main wasm-tools repository?

Been noodling on it. Hoping I can have something up for you to follow this coming week.

@alexcrichton
Copy link
Collaborator

My hope is to definitely deprecate/remove the current "set of packages" input where it's just a bunch of package ... { ... } blocks in favor of this feature of "must have a top-level package ...; to explicitly indicate what the main package is. That'll hopefully simplify the various tooling pieces where everything is still working with a single world/package at a time rather than a set

@macovedj
Copy link
Contributor Author

Is there a companion PR with the implementation in the main wasm-tools repository?

Still working through some of the tests and tidying up a bit, but figured I'd put it up here so you could get a feel for the changes

@alexcrichton
Copy link
Collaborator

Ok I believe that the implementation at bytecodealliance/wasm-tools#1700 is good to go now. It's somewhat urgent to get that out because the C# bindings generator was about to start relying on multi-package output in a way that is incompatible with this proposal. I'd like to head that off and avoid any workaround/compat bits by only having in-the-wild C# things use this new format of package-at-the-top and optionally-nested-packages-afterwards

macovedj and others added 3 commits August 12, 2024 09:54
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
@lukewagner lukewagner merged commit 4eb255f into WebAssembly:main Aug 13, 2024
1 check passed
ydnar added a commit to bytecodealliance/go-modules that referenced this pull request Aug 23, 2024
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.

4 participants