-
Notifications
You must be signed in to change notification settings - Fork 68
[native_assets_cli][native_assets_builder] Supporting version skew between SDK and build.dart #93
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
Comments
In short, I would suggest that you version the protocol based on the default So specify input/output like:
If name: my_package
environment:
sdk: ^3.1.0 # This will make the default languageVersion in package_config `3.1` Then my build script would :
Thus, if Similarly, if It's a bit of work, but a table with inputs and output, field name, description and minimum language version would go far. |
After more discussions with @jonasfj, relying on the language versions of a package doesn't work. A package with language version 3.4 could use a helper package to parse If we would like to support version skew, we should do protocol negotiation. If we would like to completely avoid version skew, we could consider not exposing a JSON protocol, but only A poor mans version of a |
Update on the approach:
This means maintaining an implementation for older versions of A changelog for both can be found in:
If we ever want to support breaking "1.", we should implement a handshake. E.g. |
We run into a similar issue with versioning Ways to handle this more gracefully:
For all three options, we still want to aim to not do major version bumps, so these are all in case we really need to in the future. From an encapsulation POV, I believe option 3 to be the cleanest. The package itself deals with version skew internally, it doesn't leak out into the API, and the callers don't have to know about it. @mosuem Did I miss any pros/cons for the different options? You were advocating for option 2, some pros I missed there? |
Actually, this could probably be made to work. But it might not be easy. Suppose we have a world with:
When the build system has to build
For this to work, then
Obviously not all versions of
Maybe, I'm missing something, and maybe it's too complicated. But it kind of feel like writing the native build protocol version in the And the best way to write the native protocol version in the Or maybe the native build protocol version should be a completely different field in |
Talking to Daco, I can certainly see how (3) is just much simpler all around 😄 |
Some notes from discussion with @jonasfj: Exploring tying protocol version to Dart SDK version
|
Ways to handle this more gracefully:
For both option 3 and 4, the Then the question is should these formats have a version at all? Shouldn't they simply be versioned with the SDK? I believe it's always safe to have a version. The question is more what kind of logic is tied to that version. Should users ever have to worry about it? Does it show up in error messages etc? I think our goal could be for Dart & Flutter stable releases that these versions are never really visible for users. On dart dev releases and Flutter master channel they might show up while things are rolling. (Version skew between (From an implementation point of view, writing the parser and serializer from a certain version is easier than just assuming everything is nullable. It simplifies writing unit tests for checking that version skew works. Of course this is not really a concern for users.) |
I believe we have explored enough alternatives and will stick with the current approach:
|
We started breaking older versions of config.json input under the following conditions:
It's unsafe to remove support for older versions in the config.json in the SDK, but we do so anyway, breaking any packages that have not updated their dependency on Note that this way of working could potentially break other embedders, because they might not update the protocol in time while still updating Dart. |
Let's continue the discussion in the existing bug.
|
👍 Yes we should!
These already have a meaning, the public Dart API. We'll still need to also satisfy that meaning. Also we could do minior/patch version bumps to change the SDK constraint. So we don't need have the semantic version actually mean the protocol version. We only need to bump the SDK constraint afaics. If we want to use a package semantic version to specify the protocol version we should have it separately from the Dart API version. (e.g.
The lower bound will need to double check that both Dart and Flutter (with some version of Dart) support that protocol. If there's a 3rd embedder that embeds Dart, but lacks behind in adopting a newer version of the protocol, we can't meaningfully use the Dart lower bound anymore. Because our Dart and Dart in Flutter might support a version of the protocol but that new embedder might not. This smells like it's in the wrong place. The base protocol version depends on the embedder/launcher. The launcher should declare it. Not a package trying to approximate it via a a Dart version constraint. If the SDK should declare it, it should be more like Flutter's pinned packages. But again, we don't want to pin the protocol version, that's not graceful. We want to support a range. Basically, you want the SDK to have it's own # Dart / Flutter / other embedders:
dependencies:
hooks_protocol: >=1.5.0 <= 1.9.0
code_assets_protocol: ^1.0.0
data_assets_protocol: ^1.0.0 # package:hook
version: 4.3.2
dependencies:
hooks_protocol: >=1.5.0 <= 1.9.0 # package:code_assets
version: 1.0.0
dependencies:
code_assets_protocol: ^1.0.0 # package with a hook
dependencies:
hooks: ^4.3.2
code_assets: ^1.0.0 This would cover the constraints more properly. And basically This approach means:
The only issue that I see is that |
cc @jonasfj We've got more schemes going on trying to use pub to this versioning. 😄 |
Firstly my suggestion is to rely on existing version constraint solving instead of custom version numbers in json or C header files, etc. That's what I care about most and it does seem there's agreement around that, which is great! The second part is how to piggy back on version constraint solving exactly. The benefit of the suggestion I made above is that we don't have to change pub's version constraint mechanism. But I'm entirely open to other ways to piggy back on our semantic constraint solving mechanism -- iff it can be done soon - as we don't want to artificially delay CLI. @jonasfj Would the pub team be willing to change version solving logic to have a magic list of Dart SDK dependencies on packages? If so, could that be done within ~ 3-4 months?
Remembering what rolled into what is needed in any case. We should have always transition periods if we do breaking changes, so it's necessary to think when a new api rolled out, when the old one got deprecated and when we can remove the old thing. The publishing of the packages with higher sdk bounds would be automatic - as part of Dart SDK release process. So I'm not quite sure I see the problem with this. The only manual intervention would be to increase minimum sdk versions when new things get introduced vs updating numbers in a hard coded list - which seem somewhat similar in effort. |
Basically @jonasfj already proposed in dart-lang/pub#3962 what I prosed in #93 (comment). 🔥 ❤ Notes from discussion with @jonasfj:
Notes from discussion with @jonasfj and @mkustermann:
This would make me a happy man! 😄 🙏 Thanks @mkustermann @jonasfj! 🚀 |
Notes from a discussion with @jonasfj and @sigurdm.
|
The basic asset mechanism could still be in the dart sdk (which is included in the flutter sdk) and the consumption of asset types is specific to each sdk and should be located there. The only thing that IMO should live on pub.dev, is helper packages for constructing these assets. |
That could work 👍 We should give some thought to if other SDKs can introduce hooks that are not in the Dart standalone SDK. Because in that case If we expect to author the list of permissible hooks, and to have all of them in Dart (not have Flutter-only hooks), then we can indeed do it as an SDK package. (I'd feel somewhat uncomfortable committing to that, as I don't have a crystal ball.) |
Some general thoughts about the idea of a package_incompatibilities.yaml file somewhere in the sdk:
If we feel we really need something like this, I'd rather like some kind of generalization of the "retraction" feature, where package owners could specify incompatibilities after they are found. (I agree this feature would be more expensive to build - but it is also more generally useful). |
Notes from another round of discussion with @mkustermann @jonasfj and @sigurdm: Alternative currently pursuing
Downsides of this approach:
TODO:
Alternatives considered
|
Small update from my explorations of using a JSON schema (#95) and dealing with version skew: We'll need separate schemas for hook writers and SDK authors. Consider the following scenario: We start with Initial situation: {
"$schema": "http://json-schema.org/draft-07/schema#",
"title": "Input Schema (Writer and Reader - Start)",
"type": "object",
"properties": {
"output_dir": {
"type": "string",
"format": "uri",
"description": "Output directory"
}
},
"required": ["output_dir"],
"additionalProperties": true
} Transition period for hook writers, assume that one of the two output directories is available. {
"$schema": "http://json-schema.org/draft-07/schema#",
"title": "Input Schema (Reader - Transition) -> Compatible with both the Start and End schema for writers",
"type": "object",
"properties": {
"output_dir": {
"type": "string",
"format": "uri",
"description": "Output directory (deprecated: use output_dir_v2 instead)",
"deprecated": true
},
"output_dir_v2": {
"type": "string",
"format": "uri",
"description": "Output directory v2"
}
},
"oneOf": [
{ "required": ["output_dir"] },
{ "required": ["output_dir_v2"] }
],
"additionalProperties": true
} Transition period for SDK authors, provide both output directories. {
"$schema": "http://json-schema.org/draft-07/schema#",
"title": "Input Schema (Writer - Transition) -> Compatible with both the Start and End schema for readers",
"type": "object",
"properties": {
"output_dir": {
"type": "string",
"format": "uri",
"description": "Output directory (deprecated: use output_dir_v2 instead)",
"deprecated": true
},
"output_dir_v2": {
"type": "string",
"format": "uri",
"description": "Output directory v2"
}
},
"required": ["output_dir", "output_dir_v2"],
"additionalProperties": true
} Final breaking change, after 2 years. {
"$schema": "http://json-schema.org/draft-07/schema#",
"title": "Input Schema (Writer and Reader - End)",
"type": "object",
"properties": {
"output_dir_v2": {
"type": "string",
"format": "uri",
"description": "Output directory"
}
},
"required": ["output_dir_v2"],
"additionalProperties": true
} The scenario is identical for if the hook output changes in a breaking way. @jonasfj You've been saying it's madness that we use the same serialization/deserialization for the hook-helper-package and the SDK-implementation. Now that I've been trying to express the contract in a schema, this becomes obvious. The implementation was written in a way that it dealt with the version skew inside the Dart code. However, we can't smooth over that difference when explicitly stating the contract in a schema. For our documentation for hook writers, Dart+Flutter SDK consumers, we're mostly concerned about the schemas for the input-reader and output-writer. However, since we want to enable other SDKs to implement the hook protocol as well, we should be explicit about the contract that these implementations must uphold as well. (And our own implementation in Dart and Flutter must uphold it.) This is the schemas for the input-writer and output-reader. |
Luckily we have refactored the code and separated the reading and writing part of the json for this exact reason: The SDKs use the builders to create json and the hook authors use the readers to read it (and vice versa for the output). |
Notes a discussion with @mkustermann:
Notes from a discussion with @mosuem:
|
It's important to disentangle this a bit. If one changes the protocol to
Yes, the second point means that the increased lower bound SDK constraint may still not guarantee that the package works in a 3rd party SDK (a rare case in the first place) - but it's strictly better than the alternative: Users on older sdks, getting package resolutions to versions that won't work and getting weird build or runtime errors - instead of pub solving for an older version of the package that just works. Imagine a newer version of
So if we introduce some new support and packages rely on it (cannot function without) they should have a corresponding lower bound SDK constraint. It's not perfect, but better than not having this constraint. Yes 3rd party SDKs may have trouble with the selected versions - but for the 99% case of Dart&Flutter SDK users it will be highly beneficial to have that lower bound SDK constraint and in the future we may eventually get support from Pub to do this in a better/cleaner way (**) Now I can see that @dcharkes main point may be that a package wants to work in older and newer SDKs but optionally take advantage of features of newer SDKs. So it's hook would want to check if new features are available and use them and otherwise use some kind of fallback mechanism. Phrased in another way: The hook requires protocol
tl;dr I wouldn't optimize (or make things much more complicated) for this particular use case of a hook author wanting to talk multiple protocol versions. (**) As discussed many times - this could be solved by having a separate package representing the protocol and SDKs (be flutter/dart sdks or package-based sdks) constraining versions of that protocol package - but it seems it was agreed to rule this out for now) |
Agreed, for these the lower SDK constraint should be bumped in such package. This would be the SDK constraint inside the package with the hook that requires this feature.
What I'm proposing is to not eagerly bump
I don't believe this to be the case. We haven't introduced features that don't have an almost trivial fallback for months. And almost all of these will be taken care of in the hook helper packages such as We'd adopt the new features in |
Bug: #93 Remove any reads of `version` in the protocol. We're moving towards a world where deprecations and breaking changes are per property and documented via the JSON schemas. So we don't use version numbers anymore. * Change the syntax to make `version` nullable. * Still write the last known version (to avoid breaking older hooks and older SDKs with newer SDKs and hooks). * Remove the `version` from the semantic API. * Remove all checks that were checking for versions. * Note: this regresses error messages, and this is intended.
Updates:
We've started having an Dart dev version lower bound on
|
Updates:
New status quo:
Things considered and not adopted:
Now that this is the new status quo, I'm going to close this issue (again). |
With #26 we added a version number to to the
BuildConfig
andBuildOutput
.The way this works is that the SDK passes the version of the protocol that has rolled in to the Dart (and Flutter) SDK. If a user's
build.dart
cannot deal with that, the build is supposed to fail.Similarly, the
build.dart
passes its value of the protocol in the build output. The Dart (and Flutter) SDK check the version and error if the output cannot be used.@jonasfj suggested that we could use the Dart SDK version instead of the a version number in the protocol and use the SDK lower bound on the package being build to determine what version of the
BuildConfig
to provide to a package. The main benefit of this approach would be that if we need to do a breaking change to the build configuration, we don't end up in a situation where all dependencies with native assets need to be migrated before developers can update their Dart / Flutter SDK.If the resolved
package:native_assets_cli
dependency in the package uniquely determines the protocol version (e.g. all protocol version bump also are a package version bump) we can also support the same behavior by looking at version of that package a package requires.(With both approaches, the implementation in
package:native_asset_cli
needs to have a version number in the build config construction so it can keep constructing older versions.)The text was updated successfully, but these errors were encountered: