Support multiple package in a single WIT source #1577

azaslavsky · 2024-05-26T00:24:29Z

This change implements the narrow version of the work proposed at WebAssembly/component-model#313, and added to the language specification in WebAssembly/component-model#340. Multiple packages may now be described in a single WIT file, also known as "explicit" package declaration (ie, a block scope for every package), in addition to the already support "implicit" mode (a single package, spread over one or more files, with no block scope at the top level).

Relevant tools, like component embed and component semver-check, have been updated and given tests where required.

A great number of APIs that previously assumed that they would receive exactly one Package/PackageId/other package-describing object, or a Result thereof, now receive vectors of these objects. This spidered out to various call-sites and tools in a fairly broad manner. I've made an effort to have these tools gracefully handle multiple packages where necessary, or, in cases where the tool cannot accept multiple packages, gracefully fail on these sorts of inputs.

The AST has been modified a good deal to accommodate these new changes. Specifically, the concept of a DeclList has been introduced, to contain all of the non-package declarations in a package scope. At the top-level, the AST is now a union of either a list of ExplicitPackages or a single PartialImplicitPackage; we try to deduce which of these two modes the given WIT file is in as early as possible, so that we can properly establish scope and start building the relevant package-level/file-level DeclList, respectively. Care is taken throughout the parser to catch scenarios where the explicit and implicit styles have been mixed and return a descriptive error to the user explaining that this is not allowed.

As of this PR, single packages written in the explicit style (package my-only-package:-in-this-file { ... }) are allowed. My feeling is that the parser should accept this, but a future linter should discourage it and "auto-unwrap" such spellings into the implicit style.

azaslavsky · 2024-05-27T16:55:26Z

I believe this is ready for review.

Afaict, the failing tests are all failing on head, with the exception of Rustfmt. However, when I run cargo fmt locally it produces no changes, so I'm not sure where the difference in formatting settings may be occurring.

azaslavsky · 2024-05-27T16:59:01Z

Also worth noting for future reviewers: I realize this is a rather large PR, which I think is due to this change modifying a lot of internal APIs to be be vectors of Package, rather than singletons. I've tried to make the individual commits meaningful in their own right, so they each have their own commit messages and so on. The first 11 commits in this PR can be reviewed in order if more bite-sized review chunks are desired.

alexcrichton · 2024-05-28T17:27:34Z

Afaict, the failing tests are all failing on head

Ah looks like we're getting bitten by a warning in a new Rust nightly, and I think this should be fixed by #1581

Also worth noting for future reviewers: I realize this is a rather large PR, which I think is due to this change modifying a lot of internal APIs to be be vectors of Package, rather than singletons

No worries! That comes with the territory of changes like this. I'll dig in today for review.

alexcrichton

This looks fantastic to me, thank you again so much for working on this! I'm definitely looking forward to being able to refactor tools to use this and take advantage of it. I think this is going to unlock some quite nice refactorings and downstream abilities of other tooling.

alexcrichton · 2024-05-28T17:35:23Z

src/bin/wasm-tools/component.rs

+    /// This is the path, within the `WIT` source provided as a positional argument, to the `world`
+    /// that the core wasm module works with. This can either be a bare string which is a document
+    /// name that has a `default world`, or it can be a `foo/bar` name where `foo` names a document
+    /// and `bar` names a world within that document. If the `WIT` source provided contains multiple
+    /// packages, this option must be set, and must be of the fully-qualified form (ex:
+    /// "wasi:http/proxy")


If you're up for some extra edits much of this preexisting documentation was already wrong/outdated. For example default worlds were removed awhile ago. If you'd like I think it'd be reasonable to update this entirely in addition to the qualification you've added.

Tbh, I'm not entirely clear on which parts of this comment are no longer relevant, so I'm not totally comfortable making deletions :/ My knowledge of the existing state of the tooling still leaves a lot to be desired!

No worries! I'm happy to take a pass after this PR as well.

crates/wit-smith/src/lib.rs

crates/wit-parser/src/lib.rs

crates/wit-parser/src/resolve.rs

alexcrichton · 2024-05-28T19:40:03Z

crates/wit-parser/src/resolve.rs

+                last = Some(pkgid);
+            }
+
+            ret.push((last.unwrap(), files));


This is me musing a bit here, but I wanted to highlight this a bit. No action needed in this PR, but figured I'd jot down my ideas.

Here the previous API returned the PackageId of the "top" package. That's being much more ambiguous after this PR because the concept of a "top" package is sort of gone where the top thing might have a whole bunch of packages. Once we buy into the concept of multiple "top" packages then some other related issues like #1461 and #1462 become more digestable (IMO at least).

AFAIK the only downside to having multiple "top" packages is the "world selection" process. Currently that allows the shorthand of specifying a single foo world name (or no name at all) and an appropriate world is selected. If we can reframe that functionality somehow to work with a set of "top" packages then it'd be pretty easy to implement those issues. That being said I don't know how best to rationalize how to select a world from a set of "top" packages.

I also bring this up because I can see here that maintaining a return value of the "top" packages is kinda awkward here and might be more awkward with one of my suggestions below of using append here. Anyway just wanted to muse a bit here in case you were inspired by something and had an idea.

crates/wit-parser/src/decoding.rs

crates/wit-parser/src/ast.rs

azaslavsky · 2024-06-01T01:05:39Z

Thanks for the thorough feedback @alexcrichton!

I've squashed existing commits, merged onto HEAD, and fixed some of the more striaghtforward comments. I'll try to address the remainder over the weekend!

alexcrichton

Thanks again for this! Just a few more minor things but other than that happy to land 👍

alexcrichton · 2024-06-05T21:03:29Z

crates/wit-parser/src/resolve.rs

-            let id = self.push_file(path)?;
-            Ok((id, vec![path.to_path_buf()]))
+            let ids = self.push_file(path)?;
+            let mut pkg_ids = Vec::new();
+            let mut path_bufs = Vec::new();
+            for id in ids {
+                pkg_ids.push(id);
+                path_bufs.push(path.to_path_buf());
+            }
+            Ok((pkg_ids, path_bufs))


I think this might be able to stay roughly as it was before, except renaming id to ids? There's no need to ensure that the two vectors have the same length, it's just that the second vector is the set of files to produce the first.

alexcrichton · 2024-06-05T21:06:21Z

crates/wit-parser/src/resolve.rs

        }
    }

+    fn _sort_unresolved_packages(


It's ok in general to skip the leading underscore for private methods in Rust since the visibility (or lack thereof here) covers that. Some of the other methods have this underscore prefix but that's just because the public and private versions both wanted to be called the same thing so I gave up on naming an gave the private one a leading _. For this case specifically though it's ok to just call this sort_unresolved_packages

alexcrichton · 2024-06-05T21:08:35Z

crates/wit-component/src/printing.rs

-            writeln!(&mut self.output, "}}\n")?;
-        }
+    /// Print a set of one or more WIT packages into a string.
+    pub fn print(&mut self, resolve: &Resolve, pkg_ids: Vec<PackageId>) -> Result<String> {


Sounds good to me 👍

alexcrichton · 2024-06-05T21:08:43Z

crates/wit-component/src/lib.rs

+///   * If there is a single resolved package with multiple worlds, the world name MUST be supplied,
+///     but MAY or MAY NOT be fully-qualified.
+///   * If there are multiple resolved packages, the world name MUST be fully-qualified.
+pub fn resolve_world_from_name(


Sounds good to me 👍

bytecodealliance/wasm-tools#1577

Do not write with additional braces for single-file, multi-package format. WebAssembly/component-model#340 bytecodealliance/wasm-tools#1577

bytecodealliance/wasm-tools#1577

Do not write with additional braces for single-file, multi-package format. WebAssembly/component-model#340 bytecodealliance/wasm-tools#1577

This is a follow-up to bytecodealliance#1577 to refactor a bit to have bindings generators continue to use `Resolve::select_world` for figuring out what to generate bindings for.

* Fold `resolve_world_from_name` helper into `select_world` This is a follow-up to #1577 to refactor a bit to have bindings generators continue to use `Resolve::select_world` for figuring out what to generate bindings for. * Review comments and API changes * Rename `Resolve::append` to `Resolve::push_group` * Add `Resolve::push_str` * Change `&Path` arguments to `impl AsRef<Path>` * Fix fuzzer build

This commit changes the output of `wasm-tools component wit` to by default output all WIT packages using the multi-package syntax implemented in bytecodealliance#1577. This means that a complete view of the WIT will be seen when running this command instead of just the top-level world.

* Improve the `--out-dir` flag of `component wit` * Lift files in `deps` outside of their folders to be `deps/*.wit` instead of `deps/*/main.wit` since this structure is now supported. * Handle multi-package input documents by placing each package in the main folder. * Name the main wit file after the package name. * Add some tests for these cases. * Print all WIT packages by default This commit changes the output of `wasm-tools component wit` to by default output all WIT packages using the multi-package syntax implemented in #1577. This means that a complete view of the WIT will be seen when running this command instead of just the top-level world. * Fix build of fuzzer * Sanitize output to be more platform agnostic

azaslavsky force-pushed the component-model-313 branch from 336123a to b4e83ed Compare May 27, 2024 06:02

azaslavsky marked this pull request as ready for review May 27, 2024 16:54

alexcrichton reviewed May 28, 2024

View reviewed changes

azaslavsky added 3 commits May 31, 2024 16:52

Squashed and rebased previous work onto HEAD

9fdcd8c

Updated push_path and push_dir return signature

a837ff2

Fix pretty printing signature and output

7b36db9

azaslavsky force-pushed the component-model-313 branch from b73dbb8 to 7b36db9 Compare June 1, 2024 01:04

azaslavsky added 6 commits June 3, 2024 18:13

Fix some more comments

74913a5

Added ResloverKind and UnresolvedPackageGroup

0ae683b

SourceMap is actually per UnresolvedPackageGroup now

81a7234

New append method is now topologically sorted

350e39a

Add some tests

08eec14

Cargo fmt

a87f944

azaslavsky requested a review from alexcrichton June 4, 2024 22:56

Fix cargo fmt crankiness

581c2b2

alexcrichton approved these changes Jun 5, 2024

View reviewed changes

Resolve final comments

b1f4914

alexcrichton enabled auto-merge June 6, 2024 00:21

alexcrichton added this pull request to the merge queue Jun 6, 2024

Merged via the queue into bytecodealliance:main with commit 2728f93 Jun 6, 2024
25 checks passed

azaslavsky mentioned this pull request Jun 6, 2024

Describe "explicit"-style WIT package grammar WebAssembly/component-model#340

Merged

ydnar added a commit to bytecodealliance/go-modules that referenced this pull request Jun 8, 2024

wit: add support for WIT files with multiple packages

607b7c1

bytecodealliance/wasm-tools#1577

ydnar mentioned this pull request Jun 8, 2024

wit: add support for WIT files with multiple packages bytecodealliance/go-modules#103

Merged

ydnar added a commit to bytecodealliance/go-modules that referenced this pull request Jun 8, 2024

wit: add support for WIT files with multiple packages

8626c96

bytecodealliance/wasm-tools#1577

ydnar added a commit to bytecodealliance/go-modules that referenced this pull request Jun 9, 2024

wit: special-case Resolve with single package

85452a6

Do not write with additional braces for single-file, multi-package format. WebAssembly/component-model#340 bytecodealliance/wasm-tools#1577

ydnar added a commit to bytecodealliance/go-modules that referenced this pull request Jun 11, 2024

wit: add support for WIT files with multiple packages

7f84a85

bytecodealliance/wasm-tools#1577

ydnar added a commit to bytecodealliance/go-modules that referenced this pull request Jun 11, 2024

wit: special-case Resolve with single package

e4aea1e

Do not write with additional braces for single-file, multi-package format. WebAssembly/component-model#340 bytecodealliance/wasm-tools#1577

macovedj mentioned this pull request Jun 11, 2024

Enable binary encoding and printing of multi-package wit files #1609

Closed

alexcrichton mentioned this pull request Jun 11, 2024

upgrade to wasm-tools 210 bytecodealliance/wasmtime#8772

Closed

alexcrichton mentioned this pull request Jun 11, 2024

Fold resolve_world_from_name helper into select_world #1611

Merged

Mossaka mentioned this pull request Jul 28, 2024

request: Support multiple package in a single WIT bytecodealliance/cargo-component#325

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple package in a single WIT source #1577

Support multiple package in a single WIT source #1577

azaslavsky commented May 26, 2024 •

edited

Loading

azaslavsky commented May 27, 2024

azaslavsky commented May 27, 2024

alexcrichton commented May 28, 2024

alexcrichton left a comment

alexcrichton May 28, 2024

azaslavsky Jun 1, 2024

alexcrichton Jun 3, 2024

alexcrichton May 28, 2024

azaslavsky commented Jun 1, 2024

alexcrichton left a comment

alexcrichton Jun 5, 2024

azaslavsky Jun 6, 2024

alexcrichton Jun 5, 2024

azaslavsky Jun 6, 2024

alexcrichton Jun 5, 2024

alexcrichton Jun 5, 2024

Support multiple package in a single WIT source #1577

Support multiple package in a single WIT source #1577

Conversation

azaslavsky commented May 26, 2024 • edited Loading

azaslavsky commented May 27, 2024

azaslavsky commented May 27, 2024

alexcrichton commented May 28, 2024

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaslavsky commented Jun 1, 2024

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaslavsky commented May 26, 2024 •

edited

Loading