Add compatibility shims for Wasmtime 13 CLI

[alexcrichton]

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 Tracking issue for migrating to the new CLI #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.

[cfallin]

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!

[cfallin]

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

[cfallin]

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.

[cfallin]

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

[cfallin]

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?

[alexcrichton]

Mind re-reviewing this? I'm trying to be really careful about the wording here since this message is likely all many folks will see and/or read so I'm trying to make it not too long, not too short, actionable, and accurate.

[cfallin]

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

[cfallin]

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

[alexcrichton]

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"

[cfallin]

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.

[cfallin]

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.

[alexcrichton]

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.

[cfallin]

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

[alexcrichton]

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.

[cfallin]

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

[alexcrichton]

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.

[alexcrichton]

Ok, how about the current wording?

[cfallin]

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)

[alexcrichton]

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

[github-actions]

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[github-actions]

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

• If you added a new Config method, you wrote extensive documentation for
  it.

  Our documentation should be of the following form:

  Short, simple summary sentence.
  
  More details. These details can be multiple paragraphs. There should be
  information about not just the method, but its parameters and results as
  well.
  
  Is this method fallible? If so, when can it return an error?
  
  Can this method panic? If so, when does it panic?
  
  # Example
  
  Optional example here.
  

• If you added a new Config method, or modified an existing one, you
  ensured that this configuration is exercised by the fuzz targets.

  For example, if you expose a new strategy for allocating the next instance
  slot inside the pooling allocator, you should ensure that at least one of our
  fuzz targets exercises that new strategy.

  Often, all that is required of you is to ensure that there is a knob for this
  configuration option in wasmtime_fuzzing::Config (or one
  of its nested structs).

  Rarely, this may require authoring a new fuzz target to specifically test this
  configuration. See our docs on fuzzing for more details.

• If you are enabling a configuration option by default, make sure that it
  has been fuzzed for at least two weeks before turning it on by default.

---

To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

[cfallin]

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.