Stream: git-wasmtime

Topic: wasmtime / PR #7385 Add compatibility shims for Wasmtime ...


view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:01):

alexcrichton opened PR #7385 from alexcrichton:restore-old-cli to bytecodealliance:main:

This commit introduces a compatibility shim for the Wasmtime 13 CLI and prior. The goal of this commit is to address concerns raised in #7336 and other locations as well. While the new CLI cannot be un-shipped at this point this PR attempts to ameliorate the situation somewhat through a few avenues:

Note that this doesn't add up to a perfect transition. The hope is that most of the major cases of change at the very least have something printed. My plan is to land this on main and then backport it to the 14.0.0 branch as a 14.0.3 release.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:01):

alexcrichton requested pchickey for a review on PR #7385.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:01):

alexcrichton requested wasmtime-default-reviewers for a review on PR #7385.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:01):

alexcrichton requested wasmtime-core-reviewers for a review on PR #7385.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:42):

cfallin submitted PR review:

A few thoughts on the messages but otherwise LGTM; I jumped in here as I had some thoughts but others may want to take a look too to make sure we're not missing anything.

Thanks for putting this together so quickly!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:42):

cfallin submitted PR review:

A few thoughts on the messages but otherwise LGTM; I jumped in here as I had some thoughts but others may want to take a look too to make sure we're not missing anything.

Thanks for putting this together so quickly!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:42):

cfallin created PR review comment:

Likewise here; perhaps we can have a specific message ("this invocation is going to break" vs. "this invocation is parsed differently by old and new") and then a general informational blob afterward?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:42):

cfallin created PR review comment:

s/,/--/ (comma splice; replace with hyphen or semicolon)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:42):

cfallin created PR review comment:

s/you/You/, period at end, and blank line to separate this note (and perhaps also a blank line before the URL)?

Also: we should note that both settings of the environment variable will work for now. I think a common use-case is that someone has usage of the old CLI, they upgrade, and they need to silence warnings without being forced to immediately do the conversion. We should note that they have that option too.

Perhaps something like:

You can set an environment variable to force either the old or new CLI option format
with no warnings emitted. Set


    - WASMTIME_NEW_CLI=0 to get behavior identical to Wasmtime 13 and earlier, or
    - WASMTIME_NEW_CLI=1 to opt into the new CLI syntax without warnings or fallback.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:45):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 21:45):

cfallin created PR review comment:

Ah, actually, I see the different NEW_CLI=0 vs NEW_CLI=1 in the two different messages now; I like the idea of giving exactly the setting that will give the same behavior but without the warning but still think we should give the full picture. Could we give the above info then append "Use WASMTIME_NEW_CLI=x to silence this warning while retaining the same behavior in this case."?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 22:08):

alexcrichton updated PR #7385.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 23:41):

cfallin submitted PR review:

Some more thoughts on wording here; others' eyeballs would also be useful on this phrasing!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 23:41):

cfallin submitted PR review:

Some more thoughts on wording here; others' eyeballs would also be useful on this phrasing!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 23:41):

cfallin created PR review comment:

"going to be used" reads somewhat ambiguously to me: that could mean "in a few months when we flip a switch", or it could mean "in a few nanoseconds when I go ahead and call this function". Perhaps "and the new semantics are now used instead of the old semantics" (i.e., present rather than future tense)?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 23:41):

cfallin created PR review comment:

Perhaps add a new paragraph here, a line on its own separated by blank lines above and below, that says "Executing according to the old (Wasmtime version <= 13) CLI semantics.", and above, analogously, "according to the nwe (Wasmtime version >= 14) CLI semantics."? That would make crystal-clear which behavior was chosen.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 14:36):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 14:36):

alexcrichton created PR review comment:

If you're ok I'd prefer to leave precise wordsmithing of this message to later. This won't ever actually get executed due to the value of DEFAULT_OLD_BEHAVIOR and the assert!(false) prior.

I don't know how to best word this message because its hedging what's going to happen in the future which I think will shape what this should precisely say.

FWIW though "going to be used" is attempting to model what's actually about to happen, e.g. new.execute() instead of old.execute() below. This is trying to be a warning of "if you intended for the old behavior then things are gonna be broken now"

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 14:39):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 14:39):

alexcrichton created PR review comment:

Is the version number necessary though? I would imagine for most users they're not really watching the version number that closely so to me it seems like it's adding noise to an already fairly wordy message. In theory the only way to hit this case is additionally previously-crafted command line via a script or similar.

For example this is the error message that will be printed out to Go and TinyGo users who probably aren't interacting with the script that's running Wasmtime at all and may not even be aware that Wasmtime is used under the hood. In such a situation all they'll want to do is (a) disable the warning and (b) open an issue upstream about fixing the warning.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:28):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:28):

cfallin created PR review comment:

Sure, we can come back to it later! My thoughts still apply; fwiw, data point of one person, "going to be used" confused me, so I think it should probably be reworded somehow by the time we use this error message if we're hoping to be accurate/unambiguous.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:30):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:30):

cfallin created PR review comment:

I guess the version number is still relevant to these folks because there will be guidance around "Go 1.21 is broken with Wasmtime 14 before patch release"; and my experience with error messages at least is that I'd rather have more information than less, so I don't have to infer what's going on. Again, data point of one person, but this was a pretty confusing message to read without that detail, and you asked for feedback re: ambiguity and comprehensibility; it's always possible that my brain is atypical though :-)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:33):

cfallin submitted PR review:

r+ with above comments; I don't want to hold up the patch release so I'll leave it to your best judgment what final adjustments to make.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:41):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:41):

alexcrichton created PR review comment:

Hm I'd still like to dig in to what you're thinking because I don't think I fully understand. One difference though is that I approach this sort of design assuming users are reactionary. For example I would predict that users won't a-priori take a look at their system and ensure that all the versions are aligned based on a blog post we publish. Instead they'll execute their normal workflow or some workflow documented elsewhere and then all of a sudden a warning pops up. In that sense they may not be aware that Wasmtime was even being used let alone perhaps what vector Wasmtime was installed through. In that sense I'd assume the immediate questions are:

In that sense I'm not sure I understand what you mean by this message requiring inference as to what's going on. My hope was to have full context behind an issue for interested folks, so not everything is explained here, but I'm not sure how adding versioning information would be helpful.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:52):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:52):

cfallin created PR review comment:

I guess there are two "user stories" here:

For the first case (your target), I think the error message is right on as it is. "Set this env var and move on" etc. For the second case (I guess I see myself in these shoes), it seems like it might be useful to have a mental model of what the parser actually did. I might see that it succeeded but there's still a warning and be a bit confused; the immediate suggestion is to set an environment variable; what I really want is direct feedback of "I interpreted this the old way" or "I interpreted this the new way". Without that, it's a little bit of an opaque magic box and I can see it being confusing. (Basically, my intuition/instinct here is to make the mechanism as explicit and visible as possible: there are two ways to parse, here's what I did.)

Anyway, please do feel free to go either way on this -- it's pretty subjective, I'm happy to expound on what intuition is telling me, but I am just one data point and it's possible (likely even?) that as long as we (i) do the right thing by default and (ii) give a GitHub issue link, that'll cover most cases :-)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:56):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 16:56):

alexcrichton created PR review comment:

No no thank you for digging in here! There's no huge urgency as I don't plan on doing a patch release until Monday (unless someone wants to do it today themselves), so I wanted to spend some time to dig in more. What you say makes sense and I think I understand better, lemme take another pass at this.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:52):

alexcrichton updated PR #7385.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:52):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:52):

alexcrichton created PR review comment:

Ok, how about the current wording?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 18:15):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 18:15):

cfallin created PR review comment:

Looks good to me at least! (one grammar nit, s/silences/silence/, "to ... silence this warning", a parallel construction thing; now you can throw me out the window because grammar nerds are annoying)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 19:21):

alexcrichton updated PR #7385.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 19:22):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 19:22):

alexcrichton created PR review comment:

It's ok I'm the son of two english majors so it's really shame on me!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 19:22):

alexcrichton has enabled auto merge for PR #7385.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 23:43):

alexcrichton updated PR #7385.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 28 2023 at 01:01):

alexcrichton merged PR #7385.


Last updated: Dec 23 2024 at 12:05 UTC