wasmtime / PR #7385 Add compatibility shims for Wasmtime ... · git-wasmtime

Stream: git-wasmtime

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

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:

A complete copy of the old CLI parser is now included in wasmtime by default.

The WASMTIME_NEW_CLI=0 environment variable can force usage of the old CLI parser for the run and compile commands.

The WASMTIME_NEW_CLI=1 environment variable can force usage of the new CLI parser.

Otherwise both the old and the new CLI parser are executed. Depending on the result one is selected to be executed, possibly with a warning printed.

If both CLI parsers succeed but produce the same result, then no warning is emitted and execution continues as usual.

If both CLI parsers succeed but produce different results then a warning is emitted indicating this. The warning points to #7384 which has further examples of how to squash the warning. The warning also mentions the above env var to turn the warning off. In this situation the old semantics are used at this time instead of the new semantics. It's intended that eventually this will change in the future.

If the new CLI succeeds and the old CLI fails then the new semantics are executed without warning.

If the old CLI succeeds and the new CLI fails then a warning is issued and the old semantics are used.

If both the old and the new CLI fail to parse then the error message for the new CLI is emitted.

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.

Wasmtime GitHub notifications bot (Oct 26 2023 at 21:01):

alexcrichton requested pchickey for a review on PR #7385.

Wasmtime GitHub notifications bot (Oct 26 2023 at 21:01):

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

Wasmtime GitHub notifications bot (Oct 26 2023 at 21:01):

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

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!

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!

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?

Wasmtime GitHub notifications bot (Oct 26 2023 at 21:42):

cfallin created PR review comment:

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

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.

Wasmtime GitHub notifications bot (Oct 26 2023 at 21:45):

cfallin submitted PR review.

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."?

Wasmtime GitHub notifications bot (Oct 26 2023 at 22:08):

alexcrichton updated PR #7385.

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!

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!

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)?

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.

Wasmtime GitHub notifications bot (Oct 27 2023 at 14:36):

alexcrichton submitted PR review.

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"

Wasmtime GitHub notifications bot (Oct 27 2023 at 14:39):

alexcrichton submitted PR review.

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.

Wasmtime GitHub notifications bot (Oct 27 2023 at 16:28):

cfallin submitted PR review.

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.

Wasmtime GitHub notifications bot (Oct 27 2023 at 16:30):

cfallin submitted PR review.

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 :-)

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.

Wasmtime GitHub notifications bot (Oct 27 2023 at 16:41):

alexcrichton submitted PR review.

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:

How do I resolve this warning now? For this the warning explains WASMTIME_NEW_CLI=0 is the way to go. Most users might just stop here.

How do I resolve the warning for the future? The warning for this explains that this will break, links to an issue with more context, and provides the env var to see the breakage. Further action though in fixing this would require them knowing how Wasmtime was invoked/installed and how to update, which I don't think the warning can help with.

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.

Wasmtime GitHub notifications bot (Oct 27 2023 at 16:52):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 27 2023 at 16:52):

cfallin created PR review comment:

I guess there are two "user stories" here:

the person who is unknowingly invoking Wasmtime indirectly, e.g. as part of go run or a CI shell script or ... -- and wants to fix the warning (short-term and long-term);

the person who is actually developing scripts or tools that invoke Wasmtime, understands that there are different versions, and is trying to parse this error message closely to figure out what's going on.

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 :-)

Wasmtime GitHub notifications bot (Oct 27 2023 at 16:56):

alexcrichton submitted PR review.

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.

Wasmtime GitHub notifications bot (Oct 27 2023 at 17:52):

alexcrichton updated PR #7385.

Wasmtime GitHub notifications bot (Oct 27 2023 at 17:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 27 2023 at 17:52):

alexcrichton created PR review comment:

Ok, how about the current wording?

Wasmtime GitHub notifications bot (Oct 27 2023 at 18:15):

cfallin submitted PR review.

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)

Wasmtime GitHub notifications bot (Oct 27 2023 at 19:21):

alexcrichton updated PR #7385.

Wasmtime GitHub notifications bot (Oct 27 2023 at 19:22):

alexcrichton submitted PR review.

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!

Wasmtime GitHub notifications bot (Oct 27 2023 at 19:22):

alexcrichton has enabled auto merge for PR #7385.

Wasmtime GitHub notifications bot (Oct 27 2023 at 23:43):

alexcrichton updated PR #7385.

Wasmtime GitHub notifications bot (Oct 28 2023 at 01:01):

alexcrichton merged PR #7385.

Last updated: Nov 22 2024 at 16:03 UTC