-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Wasmtime toplevel module: re-export wasmparser. #9485
Conversation
1199f80
to
957843c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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! |
I'm not aware of any security backports that required 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. |
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.
957843c
to
4950e12
Compare
OK, cool, I've added an off-by-default feature that gates this re-export -- thanks! |
# Enables a re-export of wasmparser, so that an embedder of Wasmtime can use | ||
# exactly the version of the parser library that Wasmtime does. Sometimes this | ||
# is necessary, e.g. to guarantee that there will not be any mismatches in | ||
# which modules are accepted due to Wasm feature configuration or support | ||
# levels. | ||
# | ||
# Note that when this feature is enabled, the version of wasmparser that is | ||
# re-exported is *not subject to semver*: we reserve the right to make patch | ||
# releases of Wasmtime that bump the version of wasmparser used, and hence the | ||
# version re-exported, in semver-incompatible ways. This is the tradeoff that | ||
# the embedder needs to opt into: in order to stay exactly in sync with an | ||
# internal detail of Wasmtime, the cost is visibility into potential internal | ||
# version changes. This is why the re-export is guarded by a feature flag which | ||
# is off by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this up!
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! |
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.