Skip to content
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 14 argument parsing change broke TinyGo and Go wasip1 support #7336

Closed
dgryski opened this issue Oct 23, 2023 · 10 comments
Closed

wasmtime 14 argument parsing change broke TinyGo and Go wasip1 support #7336

dgryski opened this issue Oct 23, 2023 · 10 comments
Labels
bug Incorrect behavior in the current implementation that needs fixing

Comments

@dgryski
Copy link

dgryski commented Oct 23, 2023

Go uses wasmtime as the default when running wasip1 binaries: https://github.com/golang/go/blob/master/misc/wasm/go_wasip1_wasm_exec . This script will break the default runner for users once they upgrade wasmtime.

Similarly, TinyGo constructs command lines to run wasm binaries. The changes to the argument parsing breaks that:
tinygo-org/tinygo#3970 .

@dgryski dgryski added the bug Incorrect behavior in the current implementation that needs fixing label Oct 23, 2023
@alexcrichton
Copy link
Member

Thanks for the report! If you're curious more discussion about this can be found on #6925 and related links.

Is it possible to have these scripts to get updated to require Wasmtime 14? Or is it desired that any Wasmtime version in the environment suffices? In the case that any Wasmtime version would suffice I think that will unfortunately require version detection to select the right CLI.

@ydnar
Copy link

ydnar commented Oct 23, 2023

Regardless of the merits of the redesigned CLI, I think having a compatibility shim with deprecation warnings might have been warranted, for at least one major version.

--max-wasm-stack: deprecated in wasmtime v14. Did you mean -W max-wasm-stack=N?

@alexcrichton
Copy link
Member

I wanted to be sure to copy over some comments from here to this dedicated issue as well as I suspect others will land here in the future too.

First off, if this is catching folks by surprise, I'm sorry about that! The intent was to gradually roll this out the best we could to prevent catching anyone by surprise. The places this was previously discussed prior to the Wasmtime 14 release were:

That being said if you're surprised by this change, that definitely wasn't sufficient communication! I would at least personally be interested in learning how to better communicate possible breaking changes like this to downstream consumers. Feel free to reach out to me (or other project members) either here, on email, or on Zulip, and we can continue discussion.

Now with that being there's two points that were discussed historically in the above discussions which I think are also important to reiterate here as well:

  1. It was discussed and explicitly decided to not have a period of warnings or similar related to this change. This primarily arose from the difficulty of detecting when to warn due to the fact that CLI args not only changed syntax (e.g. --disable-cache to -Ccache=n) but additionally changed how they were parsed. For example previously wasmtime foo.wasm --disable-cache would be parsed as disabling the cache in Wasmtime, but after this change would be parsed as passing the --disable-cache flag to foo.wasm. This means that, for example, if foo.wasm actually took a --disable-cache it's difficult to determine whether the warning should be emitted or not. In the end it was decided that by combining both the syntax and parsing changes in the same release (as opposed to two different releases) that it would assist in alerting users to a change as opposed to silently breaking.

  2. This is intended to be The Last major breaking change to the CLI. Emphasis was placed in multiple locations when talking about the change that this was basically the last opportunity to have changes made on this scale. Such a breaking change will not be allowed in the future. Minor changes may still happen but if they do they'll be required to go through a warning/deprecation/etc process.

I bring up these two points less to justify what happened but moreso to help highlight that these were discussed and to additionally summarize (to the best I can) the results of the discussions at the time. If folks are landing here then I'll reiterate again that it primarily means that the communication here wasn't handled as best as it could have. I (and I'm sure others) am interested in improving communication going forward, so please reach out of you have concerns and we can figure out how best to take into account more feedback and communicate more effectively.

@ydnar
Copy link

ydnar commented Oct 25, 2023

Thanks for the additional context!

That being said if you're surprised by this change, that definitely wasn't sufficient communication! I would at least personally be interested in learning how to better communicate possible breaking changes like this to downstream consumers. Feel free to reach out to me (or other project members) either here, on email, or on Zulip, and we can continue discussion.

In the spirit of constructive feedback, I think a couple things could have been improved with the rollout. The first is comms, and the second is implementation.

Comms

I think it’s important to distinguish between internal project comms and external comms. Zulip, GH issues, PRs, and internal discussion are just that—internal. I don’t think it’s reasonable to expect your users (or their users, who might also be affected) to keep abreast of internal discussions.

  • For breaking changes, it’s probably worth a blog post, a Tweet, and/or reaching out proactively to significant users ahead of shipping the change.
  • Second, it might have been worth waiting until Monday to ship the change. Shipping a big release on a Friday with little to no public notice gave folks little time during the work week to adapt to the change.
    • Somewhat related: monthly semver-major releases do not signal stability. Recommendation: preserve monthly release cadence, but shift to semver-minor, with additional communication ahead of breaking semver-major releases on a lower frequency.
  • Last, improved release documentation:
    • Publish a changelog on the website.
    • Document all of the CLI arguments in the documentation. Currently the only way to get complete CLI documentation is to install wasmtime or examine the source code.
    • Publish a migration guide for every CLI argument.
    • Add a changelog to the Wasmtime CLI page on crates.io.

Implementation

Given this was a breaking change for all users of the Wasmtime CLI, I think having some period of backwards compatibility is warranted.

  • For N (<= 2?) major versions, support both the existing CLI arguments in a frozen v13 state, as well as the new v14+ argument structure.
  • For each legacy argument, emit a deprecation warning with a translation from old → new so callers can adapt their programs quickly and easily.
  • Consider whether a longer duration of backwards compatibility is warranted, e.g. for arguments whose names and function have changed entirely (--mapdir to --dir).

Hopefully some of this is useful, and happy to discuss here or live. Thanks for your consideration!

@ydnar
Copy link

ydnar commented Oct 25, 2023

A couple other notes:

  1. Shipping major changes like this behind a feature flag or as a pre-release version is another way to give notice users affected by the change.
  2. A pre-release period or a period of backwards compatibility gives you a chance to refine the new CLI arguments (with further breaking changes) while existing users depend on the legacy CLI.
  3. Changes like this inevitably create work for every stakeholder. I have no doubt the new CLI redesign was well-considered and probably the right plan over the long term. But I think the question worth asking is: what is the work we want our users to bear, versus what work can we take on as maintainers to promote usage of the product by making our users’ lives easier?

@tschneidereit
Copy link
Member

(Only quickly replying to two points before I sign off for the night)

Somewhat related: monthly semver-major releases do not signal stability. Recommendation: preserve monthly release cadence, but shift to semver-minor, with additional communication ahead of breaking semver-major releases on a lower frequency.

We have very thoroughly thought about this before landing on this approach, see the section on release cadence and the rationale in the RFC for the 1.0 release.

The underlying problem is that we're in a space that is rapidly evolving, and we have a number of different embeddings of wasmtime that we think should always be able to communicate the version number of wasmtime, not the embedding itself, as the most important aspect. This has fundamental tensions in it, and I still believe that quickly iterating major version numbers is the best of a range of not-great options.

I think the question worth asking is: what is the work we want our users to bear, versus what work can we take on as maintainers to promote usage of the product by making our users’ lives easier?

As I mentioned over in golang/go#63718, and as Alex said above, we got this one wrong, and should've done a better job of communicating and rolling out this change. Nevertheless, please rest assured that we're very very aware of the fact that building a platform like wasmtime comes with great responsibility. And I think we're getting this right almost all of the time, in part because we put a lot of thought into how we go about both building and releasing wasmtime.

@tschneidereit
Copy link
Member

We just discussed this in the Wasmtime project call, and decided on a remediation plan.

Most importantly, work is underway right now to put out a patch release that restores support for the current CLI (while retaining support for the new one as well).

We also agreed on handling the CLI as a stable interface with strong compatibility guarantees, as well as the shape of a process for ensuring that we can keep to these guarantees, as well as how we'll communicate changes to the CLI. We'll publish an RFC detailing this, with ample opportunity to weigh in.

Note that we deem the changes to the new interface foundationally important for these stability guarantees, so the RFC and an accompanying blog post will also include details on how we'll fully transition, and eventually sunset support for the old interface.

alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Oct 26, 2023
This commit introduces a compatibility shim for the Wasmtime 13 CLI and
prior. The goal of this commit is to address concerns raised in bytecodealliance#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 bytecodealliance#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.
@alexcrichton
Copy link
Member

I've opened #7385 which implements compatibility shims and warnings. Once that lands on the main branch I will backport it to the 14.0.x release, likely as 14.0.3. My guess as to the ETA on that is probably early next week. I've additionally opened a tracking issue which will be pointed to by warnings the CLI will now print. That tracking issue contains further information about translating CLI invocations between the old and the new.

github-merge-queue bot pushed a commit that referenced this issue Oct 28, 2023
* Add compatibility shims for Wasmtime 13 CLI

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.

* Wordsmith messages

* Update messages

* More wording updates

* Fix grammar

* More updates
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Oct 28, 2023
* Add compatibility shims for Wasmtime 13 CLI

This commit introduces a compatibility shim for the Wasmtime 13 CLI and
prior. The goal of this commit is to address concerns raised in bytecodealliance#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 bytecodealliance#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.

* Wordsmith messages

* Update messages

* More wording updates

* Fix grammar

* More updates
alexcrichton added a commit that referenced this issue Oct 28, 2023
* Add compatibility shims for Wasmtime 13 CLI

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.

* Wordsmith messages

* Update messages

* More wording updates

* Fix grammar

* More updates
@alexcrichton
Copy link
Member

Wasmtime 14.0.3 is now released, and it should address concerns raised here. If there are still issues with it please let us know!

@alexcrichton
Copy link
Member

I'm going to close this now in favor of #7384 as I believe the concerns have been addressed. Please follow-up though if that's not the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing
Projects
None yet
Development

No branches or pull requests

4 participants