Allow custom build/link configuration, decouple core CLI infrastructure from code assets, data assets, ... #1643
Conversation
…iguration
This PR allows users of `package:native_assets_builder` to supply custom
build/link configuration. This allows e.g. flutter to add additional
configuration to `hook/{build,link}.dart` scripts that require flutter
specific things.
As opposed to earlier PRs that merged `Foo` and `FooImpl` we have a
different approach for build/link config/output as they are something
different:
The current API (even before the recent refactoring to it) allows read
and write access to e.g. assets, ...). We remove this capability as this
is conceptually problematic:
- Currently those API classes are both mutable and at the same time
support operator==/hashCode:
=> This is problematic as inserting such an object into a set/map and
then modifying it means one can later on not find it anymore. Mutable
objects can have operator==/hashCode iff it doesn't change (e.g. the
default implementation based on identity). Otherwise objects should be
immutable if they want to support operator==/hashCode.
=> For our puroses we have no need for operator==/hashCode and
therefore remove this (problematic) capability.
- Currently those API classes are serving both the hook writers and the
bundling tool. The bundling tool would use Impl versions of those, but
in the end operate on the same structures.
We now change this to be using the builder pattern: The code that
* creates build/link config/output will use a builder object that allows
mutationonly
* consumes build/link config/output will uses a view object that only
allows read access
We then make those build/link config/output objects flexible in the
sense that
* a bundling tool can add more configuration to build/link
configuration
* a hook can consume this additional configuration
To support this we
a) Make the builders operate on a json map that allows incrementally add more
things to the build/link config.
=> The bundling tool gives `package:native_assets_builder` a function
that creates the initial configuration which allows it to add
bundling-tool specific configs (e.g. flutter specific things).
=> Initializing the configs is split up into groups that belong together
=> Named with `{Build,Link}ConfigBuilder.setup*()` methods which
initialize things that conceptually belong together.
=> A bundling tool can then e.g. add code asset specifics via
`configBuilder.setupCodeConfig(...)`
b) Make the hooks operate on a json map that allows viewing this
additional information via extension methods.
=> A hook can use e.g. `config.codeConfig.cCompiler`
Since not all bundling tools may want support code assets (web builds),
the code asset specific configuration is now moved out of the core packages
and is one such bundling-tool specific configuration.
=> Hook writers can now access it via `config.codeConfig.*`
=> This sets up the stage to change the CLI protocol to move
those code-asset specific things into a subtree of the config
(e.g. json['code_asset_config'])
=> Then we can allow hook writers to easily detect it by introducing a
`config.hasCodeConfig` getter.
We make various smaller other changes, e.g.
* The `package:native_assets_builder` APIs now either return a result on
successfull build or `null` in case of error.
=> This makes callers have to check (due to nullable type of result)
whether build/link succeeded or not (easy to forget to check for a
`result.success` boolean - as evidenced a number of tests that
didn't check this `success` boolean)
=> It avoids returning a result that is only partial (i.e. has some
assets but not all due to some builds failing)
* The `Architecture` is now moved to be a code-asset specific
configuration
=> It makes sense for code assets.
=> It wouldn't make sense for e.g. web builds where there's no code
assets available.
=> For now we keep the target operating system, but even that's
somewhat problematic for web builds.
* We no longer need the (temporary) `output.{code,data}Assets.all`
getters
=> This is now natural due to the seperation via builder pattern.
Overall:
* The changes on the bundling tool side are rather minimal:
```diff
final buildResult = await nativeAssetsBuildRunner.build(
+ configCreator: () => BuildConfigBuilder()
+ ..setupCodeConfig(
+ linkModePreference: LinkModePreference.dynamic,
+ targetArchitecture: target.architecture,
+ targetMacOSVersion: targetMacOSVersion,
+ cCompilerConfig: _getCCompilerConfig(),
+ ),
workingDirectory: workingDirectory,
- target: target,
- linkModePreference: LinkModePreference.dynamic,
+ targetOS: target.os,
buildMode: BuildMode.release,
includeParentEnvironment: true,
- targetMacOSVersion: targetMacOSVersion,
linkingEnabled: true,
supportedAssetTypes: [
CodeAsset.type,
@@ -160,7 +168,7 @@ class BuildCommand extends DartdevCommand {
...await validateCodeAssetsInApplication(assets),
],
);
- if (!buildResult.success) {
+ if (buildResult == null) {
stderr.writeln('Native assets build failed.');
return 255;
}
@
```
* The changes on the hook writer side are rather minimal as well, e.g.:
```
- final iosVersion = config.targetIOSVersion;
+ final iosVersion = config.codeConfig.targetIOSVersion;
```
The main changes are all encapsulated within the
`package:native_assets_builder` and `package:native_assets_cli` and the
*many* tests that need updating.
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
|
This is how the changes in the standalone bundling tool look like: cl/389500: As we pull the code-asset specific configuration out into bundling tool, we have now (as a nice side-effect) more of the Dart CI testing code living in dart-lang/sdk. |
|
@dcharkes I haven't polished it yet, but as you're soon OOO I decided to upload it now, so you can have a look. |
There was a problem hiding this comment.
High level concerns:
versioning of protocol, versioning of extensions
A very high level question that I have now that the protocol is no longer monolithic: How do we deal with version skew between the Dart/Flutter SDK and hooks?
Should all extensions rock their own versioning? And then the versioning failure should occur when an extension is accessed if it's incompatible. I think that's probably the way to go.
This PR ignores versioning (and removes most/all support for v1.4.0 and v1.5.0 of the protocol). It's probably fine to break our current users 😢 🤷 . However we really need a solution for the long term for the evolution of the protocol. If we don't come up with something now, we paint ourselves in a corner.
For reference:
- [native_assets_cli][native_assets_builder] Supporting version skew between SDK and build.dart #93
- [native_assets_cli]
BuildConfigBuildInfoversioning #24
If we rock a version per extension, then:
- Extensions can provide default values for config not provided in an older version of the extension in the hook.
- Extensions can provide default values for output not provided in an older version of the extension in the Dart/Flutter SDK.
- Extensions can error on access on config values not being able to be defaulted in the hook.
- Extensions can error on adding output on things not supported by the older version of the SDK in the hook. (This is now
addXrather thantoJson.) - Extensions can error on things it cannot parse in the Dart/Flutter SDK, whether it's from an old hook or not, since we don't do validation inside the hooks anymore anyway. However, extensions might give better error messages if there's a version number output: "upgrade your hook dependencies" rather than some vague parsing error.
As a side note, this PR could probably be extended to support protocol v1.4 and v1.5 based from the CHANGELOG.md's
Validation / correct by construction
Extensibility seems to open up more opportunities for partial instantiation of a config/output. Things cannot be correct by construction anymore. Also we can't really run validators of extensions we don't know. So we'll end up with state errors on access (on access by hook writers in the hook and on access by Dart/Flutter SDK by reading the output).
I guess this is the price to pay for extensibility, which is unfortunate.
Eco system fragmentation
hook/{build,link}.dartscripts that require flutter specific things.
This sounds like an anti-pattern to me. Did you mean "hooks that can use flutter-specific things"?
E.g. I'd want packages that if they are on Flutter Android they use some Java-specific config provided by flutter to produce a jar, but if they are on Linux (standalone or Flutter) they produce a dylib. E.g. sqlite is available in Java and in C and probably easier to use in Java.
We should be trying to avoid ecosystem fragmentation.
No objection against this PR in general, we still want the flexibility. But I'd really want to discourage hook writers from writing hooks that only work on Flutter. Instead, the hook authors should write hooks that do something special if some special config is present. This also leads to the idea that probably all extensions on the config should always be nullable or have a getter isAvailable as you suggested somewhere.
(If you have use cases where you really want to have a hook that only works in Flutter and not in Dart standalone, we should discuss this.)
Small things
- Currently those API classes are both mutable and at the same time support operator==/hashCode:
=> This is problematic as inserting such an object into a set/map and then modifying it means one can later on not find it anymore.
Good point. 🤓
We now change this to be using the builder pattern: The code that
- creates build/link config/output will use a builder object that allows mutationonly
- consumes build/link config/output will uses a view object that only allows read access
Right, especially for the output, we need the mutation.
And we'd like the operator == for users writing unit tests.
And it kind of makes sense we mirror that in the config.
This sets up the stage to change the CLI protocol to move those code-asset specific things into a subtree of the config (e.g. json['code_asset_config'])
That will be a breaking change to the protocol. If you want to avoid breaking users, you should introduce a duplicate of the config under the subkey, and then wait until a new Dart stable comes out and then release a new version that removes the top level keys. Or, commit to tracking down our users and migrating them eagerly.
pkgs/native_assets_cli/lib/src/code_assets/code_asset_bundling.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_cli/example/build/local_asset/test/build_test.dart
Outdated
Show resolved
Hide resolved
I've done a first pass and tried to formulate some high level concerns. |
Absolutely anticipated those :) |
Breaking changes Right now we are in a situation where the packages are checking compatibility with one version and will throw if it’s not compatible. That means if anything in the protocol changes in a breaking way that makes us increase the major version number, it breaks all packages using hooks out there. Assuming we’re out of experimental and many packages use this, this would be massively breaking for our ecosystem. But it will break much more than it actually has to: Example: we make a breaking change to code asset encoding or code configuration - it will also break packages that only use data assets (and aren’t actually affected by this change). By breaking up the protocol into individual orthogonal pieces, we have the flexibility (if we decide to) to version things individually. That means e.g. if we break code asset config or output encoding, we only break packages actually using code assets. Backwards compatible changes Currently the version (minor) number is being continuously increased because many things in the protocol aren’t stable yet - that’s because it’s an experimental feature that is in-development and things are changing. By breaking it up into orthogonal pieces we can make individual things stable when they’re ready before other things. The new structure allows handling backwards compatible changes just like the old structure. There’s nothing fundamentally different that would prevent this. For compatible changes to the build/link config one doesn’t even strictly need a version number as the json reader can just look for new encoding and if it doesn’t find it fallback to old encoding. Build/Link hook outputs would always use the oldest encoding so it works on older SDKs. New features & versioning If we introduce new features in the configuration (or in the output) which is required by a package, then the hook packages should make it clear they need that new feature and won’t work otherwise. The best way to solve this would be via our existing versioning mechanism: pub’s version solving. If a user is on an old flutter/dart SDK and wants to use a package that requires these new hook features, then ideally that user is told to upgrade the SDK (via an incompatible version constraint error). => We’d much rather have pub report incompatible version error (or select a better version of a dependency) over a hook invocation to fail at runtime due to major version changing and it throwing. So if we introduce new features in bundling tools (be it configuration or output support) we’d want the support packages to have a lower bound dart/flutter sdk constraint on that version, making every user of the feature require a newer SDK. Performing actual breaking changes If we wanted to do a breaking change, then we’ll be forced to do this via transition periods. We announce the breaking change, we offer an option to use the new mechanism and after a while we remove the old mechanism - similar to how we do breaking changes in How exactly we do this may be different from case to case. An example could be: We could offer a new If we have to make a breaking change, it's much better to do that in a smoother way that allows migrating ecosystem vs increasing a major version number in the json and make all the hooks throw. If we wanted to have stronger guarantees we could make the support packages have narrower SDK constraints (in particular narrow upper bound) but that would require us re-publishing the packages every few months with higher SDK bounds. Not sure this is worthwhile. /cc @jonasfj opinions? |
Not sure what “correct by construction” means here. Yes, these changes allow e.g. flutter to support additional asset types & configuration compared to standalone dart. The bundling tool decides what assets & config it supports - and is “correct by construction” when it builds the configurations. If a hook wants to add flutter fonts, then it needs flutter SDK constraint which ensures that it gets this capability - it's an agreement between the two.
The bundling tool will ensure the configs it gives to hooks is correct (it could “validate” it) and ensure the output of the hooks is correct (it can “validate” it). It knows all the configuration, all asset types it supports - anything that matters it can validate. The hook itself could (redundantly) validate that the config it gets is what was promised. But of course only for things the hook cares about. e.g. if a hook wants to add code assets why on earth would it want to validate anything about flutter font asset config (which doesn't affect it at all)? Even if we wanted to "double check" that the encoding/semantic agreement is uphold, it only makes sense to do that "double checking" on things that the hook relies on. It also depends what “validation” here means: Let’s break it apart:
Before my changes these two things were intermingled. Now we decouple these two aspects more and more (e.g. the json builder & json view classes in this PR focus on the former, i.e. encoding). Here’s a thought experiment about the former: Imagine we chose protobuf as a format instead of json. Then there would be a proto message format (aka schema), the bundling tool would construct it and the hook would consume it (and vice-versa for output). There will be no “validation” of the encoding happening - except for the fact that the proto decoder will throw an encoding error if it gets a message that isn’t a valid encoding of the proto it expects. The json equivalent of this could be throwing StateError/ArgumentError. We can extend a proto in a backwards compatible way by adding new optional fields and we have the same capability in json. Similarly for the semantics: If e.g. if a I'm not quite sure I understand the concrete concern here. |
We have to do this gradually anyway: We don't want to carry all this baggage forever. When we go out of experimental we should have one way of encoding things in json - not X different ways. Since we have to do it anyway: the earlier the better - as it reduces complexity. |
That is very beneficial indeed! Should we already be adding a
We need the version in the config to know what version to use in the output. (Hooks cannot output an older version if they rely on a newer feature though.)
Completely agreed! We need to be careful to avoid having a different version in the Dart SDK then in the Flutter SDK during branch alignment. Otherwise the Dart language constraint is useless in Flutter. (Note that we just passed a branch alignment period while changes are continuously rolled into Dart SDK and we have not rolled them into Flutter yet.) We'll probably need to add a step to the branch alignment process to cover this. (Maybe the sanity check in the branch alignment process could be a test in the Flutter SDK that tries to run hooks for both the dart and flutter executable and inspects the versions of the known extensions: base, data assets, and code assets, that these are identical.) |
I mean flutter can provide more asset types, more configuration that standalone sdk will not provide. Packages can take advantage of those. This is something we want, not an antipattern. If flutter is the only one supporting such asset types, such configuration, the package may start out being flutter specific. Once other bundling tools may also want to support such asset types / configuration, the package may be made independent of flutter. That's backwards compatible.
Correct, we we can provide both encodings in the |
I meant something simple: You can only have an object of
Yes, so this responsibility of correctness moves from the As I mentioned, this is just the price of extensibility. (Builders can only validate their own sub-parts.) As a side note, every responsibility that we move to the bundling tool / embedder gets duplicated between dartdev and flutter_tools. This is toilsome. Maintenance is going to require almost identical PRs in two places. My goal with native_assets_builder was to have as much code shared as possible. |
Yes, we want the technical capability. However, what I was trying to convey is that we should encourage package authors to not make their packages unseccarily only work on Flutter:
Yes, it's okay if package authors make a hook that requires Flutter for some specific use case that really cannot work in standalone. But we should not go ahead and encourage package authors to do so from the get-go. Again, I agree with the technical capabilities in this PR. I'm saying we should be careful on what guidance we give to package-with-hooks-authors. |
So yes, embedders can have custom configuration, custom asset types - so naturally only the embedder can know the full picture - not
This PR doesn't really cause any meaningful duplication in embedders, it just moves parameters around to different place: final buildResult = await nativeAssetsBuildRunner.build(
+ configCreator: () => BuildConfigBuilder()
+ ..setupCodeConfig(
+ linkModePreference: LinkModePreference.dynamic,
+ targetArchitecture: target.architecture,
+ targetMacOSVersion: targetMacOSVersion,
+ cCompilerConfig: _getCCompilerConfig(),
+ ),
workingDirectory: workingDirectory,
- target: target,
- linkModePreference: LinkModePreference.dynamic,
+ targetOS: target.os,
buildMode: BuildMode.release,
includeParentEnvironment: true,
- targetMacOSVersion: targetMacOSVersion,
linkingEnabled: true,
supportedAssetTypes: [
CodeAsset.type,
@@ -160,7 +168,7 @@ class BuildCommand extends DartdevCommand {
...await validateCodeAssetsInApplication(assets),
],
);
- if (!buildResult.success) {
+ if (buildResult == null) {
stderr.writeln('Native assets build failed.');
return 255;
}
@It's already embedder-specific how to produce values for those things (e.g. how flutter finds android ndk compiler etc) What "duplication" are you concretely concerned about? What is "toilsome"? (The |
Very Good! Yes, that's the main trickiness around versioning. Though I disagree a bit about the goal: Ideally flutter and standalone SDKs can be developed independently - we may introduce an asset type only working in Dart SDK, flutter may introduce one that only works in Flutter SDK and we may have one that both support (possibly at different versions). The fundamental problem issue that would be nice to solve is the following: A package may work in flutter and non-flutter. It can use conditional imports to do different things in flutter vs non-flutter.
Right now this is not expressible. (I guess the "federated flutter plugins" is somewhat related to this) /cc @jonasfj any chance we can this problem solved? |
|
There is an issue here: We have no good proposals, but we definitely need to solve it. |
|
Can't an SDK constraint Dart be enough? That'll pretty much guarantee that you get at-least the corresponding version of the Flutter SDK. Do we think people will want to conditionally import |
Do we really want to enable this? It sounds inconsistent for a ffi-plugin to behave in a different way on flutter vs. non-flutter. I can see behaving differently depending on the platform where you are running (ie where to search for the dynamic library). But that should not really have to do with the embedder (eg. other embedders than Flutter might exist for Android: A native plugin should behave in an "android" way, not in a "flutter" way). I believe we have https://pub.dev/packages/os_detect for this. That might all be too idealistic, and there might be technical reasons I don't see for giving flutter a special treatment. |
Let's say we want to have one package our users should use for cryptography - it's optimized on all environments & target platforms. Users use it and it will internally make sure it's fast, small and secure wherever it runs. Let's call it
=> This allows the entire ecosystem to use The issue already exists today: We cannot have such a |
That's what we'll do for now, yes. (Though conceptually it creates creates unnecessary coupling and assumptions: It assumes Flutter SDK is a strict superset of Dart SDK - in terms of API functions, functionality, asset types supported in the new hooks proposal, etc - which isn't true even today, e.g.
One could imagine some packages doing conditionally flutter specifics - relying on flutter plugins or using |
But should this not be android-specific instead of flutter specific? It sounds like
In that case it should almost certainly be a flutter only plugin I think. |
Theoretically, yes. But practically on the lowest level there's some flutter dependencies (e.g. package:jni)
But that means anything that directly or indirectly depends on it is flutter specific. So no package can support both flutter and non-flutter via conditional imports. That in return creates problems we have e.g. in |
Amongst the changes: - require `configValidator` that can ensure the final hook configs are valid before using them to invoke hooks - update changelogs - add test wrappers - split out verification & config/output tests: move code/data asset specifics into their own tests - ...
mkustermann
changed the title
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
|
Thanks for the initial round of feedback, @mosuem! PTAL |
This PR allows users of
package:native_assets_builderto supply custom build/link configuration. This allows e.g. flutter to add additional configuration tohook/{build,link}.dartscripts that require flutter specific things.As opposed to earlier PRs that merged
FooandFooImplwe have a different approach for build/link config/output as they are something different:The current API (even before the recent refactoring to it) allows read and write access to (e.g. assets, ...). We remove this capability as this is conceptually problematic:
Currently those API classes are both mutable and at the same time support operator==/hashCode:
=> This is problematic as inserting such an object into a set/map and then modifying it means one can later on not find it anymore. Mutable objects can have operator==/hashCode iff it doesn't change (e.g. the default implementation based on identity). Otherwise objects should be immutable if they want to support operator==/hashCode.
=> For our purposes we have no need for operator==/hashCode and therefore remove this (problematic) capability.
Currently those API classes are serving both the hook writers and the bundling tool. The bundling tool would use Impl versions of those, but in the end operate on the same structures.
We now change this to be using the builder pattern: The code that
We then make those build/link config/output objects flexible in the sense that
To support this we
a) Make the builders operate on a json map that allows incrementally add more
things to the build/link config.
=> The bundling tool gives
package:native_assets_buildera functionthat creates the initial configuration which allows it to add
bundling-tool specific configs (e.g. flutter specific things).
=> Initializing the configs is split up into groups that belong together
=> Named with
{Build,Link}ConfigBuilder.setup*()methods whichinitialize things that conceptually belong together.
=> A bundling tool can then e.g. add code asset specifics via
configBuilder.setupCodeConfig(...)b) Make the hooks operate on a json map that allows viewing this
additional information via extension methods.
=> A hook can use e.g.
config.codeConfig.cCompilerSince not all bundling tools may want support code assets (web builds), the code asset specific configuration is now moved out of the core packages and is one such bundling-tool specific configuration.
=> Hook writers can now access it via
config.codeConfig.*=> This sets up the stage to change the CLI protocol to move
those code-asset specific things into a subtree of the config
(e.g. json['code_asset_config'])
=> Then we can allow hook writers to easily detect it by introducing a
config.hasCodeConfiggetter.We make various smaller other changes, e.g.
The
package:native_assets_builderAPIs now either return a result on successfull build ornullin case of error.=> This makes callers have to check (due to nullable type of result) whether build/link succeeded or not (easy to forget to check for a
result.successboolean - as evidenced a number of tests that didn't check thissuccessboolean)=> It avoids returning a result that is only partial (i.e. has some assets but not all due to some builds failing)
The
Architectureis now moved to be a code-asset specific configuration=> It makes sense for code assets.
=> It wouldn't make sense for e.g. web builds where there's no code assets available.
=> For now we keep the target operating system, but even that's somewhat problematic for web builds.
We no longer need the (temporary)
output.{code,data}Assets.allgetters=> This is now natural due to the seperation via builder pattern.
We remove remaining validation from the config / output constructors (e.g. that files exists)
=> Instead we have explicit validation methods for that.
=> The bundling tool will pass down a configuration validator function that verifies the final configuration
We separate code/data asset things from the base support
=> Hook authors will import
package:native_asset_cli/{code,data}_assets.dart=> Bundling/Building tools (as well as tests) may import
package:native_asset_cli/{code,data}_assets_builder.dartOverall:
The main changes are all encapsulated within the
package:native_assets_builderandpackage:native_assets_cliand the many tests that need updating.