wasmtime / PR #9485 Wasmtime toplevel module: re-export w... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9485 Wasmtime toplevel module: re-export w...

Wasmtime GitHub notifications bot (Oct 18 2024 at 18:34):

cfallin opened PR #9485 from cfallin:reexport-wasmparser to bytecodealliance:main:

In some use-cases, an embedder might also use wasmparser directly. It might be useful in these cases to keep the version the embedder uses in sync with the version that Wasmtime uses, both for exact feature-support compatibility reasons and to remove the redundancy in the dependency tree.

wasmparser is a required dependency of the wasmtime-environ crate, even in builds without compiler backends, so this re-exports the crate at the wasmtime crate level through that path.

Wasmtime GitHub notifications bot (Oct 18 2024 at 18:34):

cfallin requested fitzgen for a review on PR #9485.

Wasmtime GitHub notifications bot (Oct 18 2024 at 18:34):

cfallin requested wasmtime-core-reviewers for a review on PR #9485.

Wasmtime GitHub notifications bot (Oct 18 2024 at 18:39):

cfallin updated PR #9485.

Wasmtime GitHub notifications bot (Oct 18 2024 at 18:39):

cfallin edited PR #9485:

In some use-cases, an embedder might also use wasmparser directly. It might be useful in these cases to keep the version the embedder uses in sync with the version that Wasmtime uses, both for exact feature-support compatibility reasons and to remove the redundancy in the dependency tree.

Wasmtime GitHub notifications bot (Oct 18 2024 at 18:46):

fitzgen submitted PR review:

I have some concerns around semver here.

This public dependency means that, in practice, we can't change our wasmparser dependency as part of any bug fixes that we want to backport, say for a security bug, because wasmparser doesn't really do patch releases. We don't usually do patch releases for wasm-tools because there are too many tools and crates and types and APIs to keep track of whether we had breaking changes or not, which are exactly the same difficulties we would run into here when backporting patches. And if we get this wrong, then we break embedders' compilations, which is a terrible downstream experience.

I also recognize that staying in sync with wasmtime's parsing is a valid use case, so I'm not sure what to do.

Am I overly worried about the semver issues? Making this a bigger deal than it should be?

Does it make sense to put this behind a cargo feature and document that we don't necessarily follow semver for this feature and its re-export?

Is there an alternative solution for this use case that I'm not aware of?

Wasmtime GitHub notifications bot (Oct 18 2024 at 18:55):

cfallin commented on PR #9485:

Excellent points. In a vacuum I agree it'd make sense to minimize what we promise -- the wasmtime API should be about Wasm execution only -- but as you note, this is motivated by a real issue on the other side, too.

I think it might be worth thinking about the bugfix case further. If the embedder has the requirement that its wasmparser dependency matches its parsing behavior with Wasmtime's, and if we need to support this requirement, then already wasmparser's behavior is relevant when we think about a wasmtime patch release. So I think this already means that we either need to have the property that backports work with the version of wasmparser matched with the old version of wasmtime, or if not, we do a patch release of wasm-tools. In practice I can't think of any backported bugfixes we've had recently at least that were tied closely to a version of wasmparser; are you aware of any?

I'm also totally fine with putting this re-export behind a feature. The question still remains whether we follow semver or whether it's a "semver-exempt" corner of the API when the feature is on; obviously the former is a nicer experience for any embedder with this requirement.

Anyway, my personal opinion is that as long as this use-case exists out there, we don't gain much in terms of global simplicity or ease of maintenance by pushing the requirement outside of our own local semver bounds, but I'd be curious what others (@alexcrichton ?) think too!

Wasmtime GitHub notifications bot (Oct 18 2024 at 19:06):

fitzgen commented on PR #9485:

I'm not aware of any security backports that required wasmparser bumps. Again, maybe I'm being paranoid.

But for the use case of "I need to use the exact same parser as wasmtime" then I think it really does make sense to say "you really get the exact same parser that wasmtime uses, regardless of the versioning that wasmtime is otherwise using" and that semver opt-out motivates putting this behind a cargo feature, IMO. Taken altogether, that feels like a consistent approach.

Wasmtime GitHub notifications bot (Oct 18 2024 at 19:31):

cfallin updated PR #9485.

Wasmtime GitHub notifications bot (Oct 18 2024 at 19:32):

cfallin commented on PR #9485:

OK, cool, I've added an off-by-default feature that gates this re-export -- thanks!

Wasmtime GitHub notifications bot (Oct 18 2024 at 20:01):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 18 2024 at 20:01):

fitzgen created PR review comment:

Thanks for writing this up!

Wasmtime GitHub notifications bot (Oct 18 2024 at 20:17):

fitzgen merged PR #9485.

Wasmtime GitHub notifications bot (Oct 21 2024 at 09:15):

alexcrichton commented on PR #9485:

Am I overly worried about the semver issues? Making this a bigger deal than it should be?

Personally I think this may be the case, I'm not too worried about this. I think this is something we'd have to be cognizant of when responding to a security issue but it wouldn't be the end of the world to issue backports for wasmparser too.

That being said I think the docs added here are fine. We can just see how it goes over time!

Last updated: Dec 23 2024 at 12:05 UTC