Wasmtime GitHub notifications bot (Oct 06 2021 at 18:36): Wasmtime GitHub notifications bot (Oct 06 2021 at 18:36):alexcrichton opened PR #3420 from exact-version-requirements to main:
This commit is a preparation for the release process for Wasmtime. The
specific changes here are to delineate which crates are "public", and
all version requirements on non-public crates will now be done with
=A.B.Cversion requirements instead of today'sA.B.Cversion
requirements.The purpose for doing this is to assist with patch releases that might
happen in the future. Patch releases of wasmtime are already required to
not break the APIs of "public" crates, but no such guarantee is given
about "internal" crates. This means that a patch release runs the risk,
for example, of breaking an internal API. In doing so though we would
also need to release a new major version of the internal crate, but we
wouldn't have a great hole in the number scheme of major versions to do
so. By using=A.B.Crequirements for internal crates it means we can
safely ignore strict semver-compatibility between releases of internal
crates for patch releases, since the only consumers of the crate will be
the corresponding patch release of thewasmtimecrate itself (or other
public crates).The
publish.rsscript has been updated with a check to verify that
dependencies on internal crates are all specified with an=
dependency, and dependnecies on all public crates are without a=
dependency. This will hopefully make it so we don't have to worry about
what to use where, we just let CI tell us what to do. Using this
modification all version dependency declarations have been updated.Note that some crates were adjusted to simply remove their
version
requirement in cases such as the crate wasn't published anyway (publish = falsewas specified) or it's in thedev-dependenciessection which
doesn't need version specifiers for path dependencies.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Oct 06 2021 at 18:39):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Oct 06 2021 at 18:39):bjorn3 created PR review comment:
I think this will break testing of cranelift-object with crater.
Wasmtime GitHub notifications bot (Oct 18 2021 at 17:37):alexcrichton requested cfallin for a review on PR #3420.
Wasmtime GitHub notifications bot (Oct 18 2021 at 18:06):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Oct 18 2021 at 18:19):alexcrichton updated PR #3420 from exact-version-requirements to main.
Wasmtime GitHub notifications bot (Oct 18 2021 at 18:33):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Oct 18 2021 at 18:58):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Oct 18 2021 at 18:58):bjorn3 created PR review comment:
As per https://github.com/bytecodealliance/wasmtime/pull/3420#issuecomment-936886434 can you please unpin all cranelift crates except cranelift-codegen-meta, cranelift-codegen-shared for cranelift-codegen?
Wasmtime GitHub notifications bot (Oct 19 2021 at 15:27):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Oct 19 2021 at 15:27):alexcrichton created PR review comment:
Doing so defeats the purpose for this change and I don't believe reflects how cranelift is developed right now. In the future when cranelift-codegen has a stability story and is designed to be stable that seems like a reasonable change to make, but I'm led to believe that that's not how it works today. This means that a vulnerability may involve a breaking change in cranelift-codegen or one of these crates so to uphold Wasmtime's stability story we would need to have a patch release of all the crates here and not use different versions.
Wasmtime GitHub notifications bot (Oct 19 2021 at 15:39):bjorn3 created PR review comment:
The dependencies of cranelift-codegen are already re-exported by cranelift-codegen. Removing or changing any api from them would be a breaking change of cranelift-codegen and thus possibly break existing users of cranelift, which isn't allowed for patch releases. Adding new api's would continue to work as the version specifier would then simply specify the new patch release as minimum version.
Wasmtime GitHub notifications bot (Oct 19 2021 at 15:39):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Oct 19 2021 at 16:27):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Oct 19 2021 at 16:27):cfallin created PR review comment:
@bjorn3 The intent of @alexcrichton's change is to give us a bit more control when we possibly have to make security-related patches in the future. While the "letter of the law" might indicate that all exported symbols must strictly obey semver for patch releases, in practice we don't guarantee stability of Cranelift interfaces beyond perhaps the instruction-builder interface. The project admittedly has not reached the point where we have a written-down stability policy and/or where we have carefully trimmed our exports just to the things we actually guarantee, but that should be the direction that we aim toward. I can say that it seems unlikely to me we would break
InstBuilderfor a security patch.All of this is indicating to me that we need an API stability policy for Cranelift, which would need to be an RFC in its own right, so maybe I will work on that at some point in the near-to-medium-term.
Wasmtime GitHub notifications bot (Oct 19 2021 at 17:48):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Oct 19 2021 at 17:48):bjorn3 created PR review comment:
All of this is indicating to me that we need an API stability policy for Cranelift, which would need to be an RFC in its own right, so maybe I will work on that at some point in the near-to-medium-term.
That is something different from semver compatibility. I am talking about how semver compatible releases need to be api compatible. An api stability policy however is how the api changes between non-semver compatible versions. In addition security fixes in cranelift are very likely to be in the implementation of a function and not in the interface or if in the interface, in the interface of a private function. I don't think there is any way that cranelift-entity and cranelift-bforest could cause a security issue that needs a change in the api. cranelift-bforest hasn't had any meaningful changes for >2 years and especially not any api breaking changes in that time.
Wasmtime GitHub notifications bot (Oct 19 2021 at 17:49):bjorn3 edited PR review comment.
Wasmtime GitHub notifications bot (Oct 25 2021 at 20:24):alexcrichton updated PR #3420 from exact-version-requirements to main.
Wasmtime GitHub notifications bot (Oct 25 2021 at 20:39):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Oct 25 2021 at 21:22):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Oct 26 2021 at 14:06):alexcrichton merged PR #3420.
Last updated: Nov 22 2024 at 17:03 UTC