[native_assets_cli] Nest CodeConfig OS-specific config

[dcharkes]

Dart and Flutter have been passing the minimum OS SDK versions for a while (in non-dry-run), which means we can mark these fields non-optional if the target OS is used.

This PR changes the CodeConfig to nest the OS versions (and iOS target SDK: device or simulator) to be nested under the OS and required.

Other side effects of this PR:

• Many unit tests where missing these (now) mandatory fields
• native_toolchain_c can no longer test without the minimum OS versions, which changed the iOS test otool output.

Addressing:

• [native_assets_cli] Move BuildConfig.targetOS to BuildConfig.codeConfig.targetOS, remove it entirely or extend to cover web #1738 (comment)

[coveralls]

coverage: 87.965% (+0.007%) from 87.958%
when pulling ca6eace on codeconfig-nesting
into acc5534 on main.

[mkustermann]

One quirk this PR uncovers is that the validators were written to work on the parsed config, rather than the JSON.

This is by design and IMHO the right thing to do. The validators are intended to validate semantics, not syntactic things (this separation of syntax and semantic was done by my refactoring).

Imagine we choose protobuf instead of JSON as the format. Then proto decoding would fail if a required field wasn't passed.

The reason this surfaces now is that it's a breaking change to the "syntax" (if you will) to make an optional thing a mandatory thing.

(That being said, there's an escape hatch. The data structures that view the config are intentionally providing access to the underlying json. So one can (if one really wants) use BuildConfig.json and operate on that)

the unit tests creating invalid configs that are supposed to trigger these validators can no longer work on the partial parsed config and serialize it

Well, it turned from a semantic error into a syntactic error. So what had to be maintained with checking semantic variants is now automatically checked via syntax.

It's unfortunate that some of the JSON serialization/deserialization has to be duplicated in the validator and unit tests.

We probably don't have unit tests for all the different syntax errors one could make (e.g. providing strings where numbers are expected, arrays where maps are expected, etc). Maybe we don't have to have such tests in the first place (would we have these tests if the format was protobuf? would we craft invalid protobuf messages and check proto decoding fails with exceptions?).

(One could imagine generating the "syntax parser code" from a json schema specification)

[dcharkes]

Imagine we choose protobuf instead of JSON as the format. Then proto decoding would fail if a required field wasn't passed.

That means we would never ever be able to upgrade a nullable field to a non-nullable field. However, we still want to provide our users with an API that makes semantically the most sense. So if it's never null (after some SDK version), a non-nullable accessor.

Well, it turned from a semantic error into a syntactic error. So what had to be maintained with checking semantic variants is now automatically checked via syntax.

I like the distinction between syntax and semantics. However, we don't want syntactic errors to simply lead to an Error being thrown in the native assets builder and dartdev or flutter_tools crashing. (Or are you suggesting syntactic errors should crash dartdev and flutter_tools?) We'd like to report both syntactic and semantic errors in a way to users that doesn't break the world. Unless we somehow ensure syntactic errors never ever happen, not even in the face of version skew.

would we have these tests if the format was protobuf?

If we never ever allow breaking changes, and we validate that the protobuf schema evolution ensures this, then no.

If we follow that logic, we cannot change the field in the syntax to be required. But we'd still want a nicer API to our users than the protobuf. E.g. if the field is now non-nullable the users should get a non-nullable API and the deserialized protobuf has a nullable API. Are you suggesting we should have two representations? One for our users (non-nullable) and one that has only done the deserialization?

(One could imagine generating the "syntax parser code" from a json schema specification)

I guess that was my last question. Then we could write the validator and the unit tests on the intermediate data structure.

• String --json.decode--> Map --syntax-parse--> generated classes --semantic-parse--> user API classes
• generated classes --validator--> error messsages

We could do such refactoring at this point (import json_serializable). However, as long as we don't use a schema evolution validation tool, we might still accidentally introduce breaking syntax changes. So, I'd still want to catch those and report them to the users in dartdev/flutter_tool as well. WDYT, should we do that now? Maybe we should postpone such refactoring until after the 3.7 branch cut.

[mkustermann]

lgtm with comments addressed

[mkustermann]

I disagree, the validation should do semantic validation (not syntactic things) - imagine we choose Protobuf instead of Json, would we do validation on the raw protobuf bytes instead of the parsed protobuf?

Also the BuildConfig does allow accessing the JSON itself (you even take advantage of it). So there's really no reason why we should be changing the validation APIs to work on JSON.

[mkustermann]

I'm not particularly fond of these changes here. These changes occur because

• the underlying json schema makes the macos/android/ios/... configs optional, but they have to be present depending on OS
• we chose an API that makes them non-nullable

The second point: This pattern with non-nullable sub-fields (which are nullable on the protocol level) that should only be used under guard conditions usually have has* getters. If we followed that style here as well, we'd have user code like this:

final codeConfig = buildConfig.codeConfig;
if (codeConfig.hasMacOSConfig) { // <-- could also use `codeConfig.targetOS == OS.macos`
  use(codeConfig.macOSConfig);  // <-- use non-nullable API
}

The validation would then look like this

switch (codeConfig.targetOS) {
  case OS.macOS:
    if (!codeConfig.hasMacOSConfig)) {
      errors.add('...');
    }
    break;
  ...
}

That would make us not have to deal with JSON here at all. It would all be encapsulated in the CodeConfig/... objects.

[mkustermann]

I like the distinction between syntax and semantics. However, we don't want syntactic errors to simply lead to an Error being thrown in the native assets builder and dartdev or flutter_tools crashing. (Or are you suggesting syntactic errors should crash dartdev and flutter_tools?)

It's a choice the flutter tools and dartdev have to make. The file could not be valid json, the json could not comply with the schema or there could be semantic issues (e.g. duplicate dylib names).

In general there's a contract between the Flutter/Dart SDK and the hook. If the hook violates the contract, then something will probably go wrong at some point. Though the question is: how likely is it going to violate a contract. For example:

• How likely is it the output is not valid json? I don't see this really happening (maybe if there's a corrupt RAM or HardDisc, ...). Let's say it happens in 0.00001% of cases. Would it be so bad that flutter crashes with an exception being thrown by a json decoder with a meaningful stack trace?

• How likely is it that two packages output a shared library with the same name? I'd argue that is much more likely that two package authors choose the same name for a shared library. We may want to have checks in place for it.

There's an infinite of things that can go wrong and one cannot guard against all of them: For example we don't actually verify that a CodeAsset emitted by the app is in valid ELF or MacO format. It could be random bytes. We could validate it or we could just bundle it and it will fail at runtime. It's a choice.

We should be checking common things that hook writers may do wrong or errors that occur for end users. Checking more than that is nice - and I'm not opposed to it - but wouldn't say it's absolutely neccesary.

[dcharkes]

How likely is it the output is not valid json?

Yeah, that's not the thing I'm worried about. It's about us (me?) making a mistake in evolving the schema, that being a syntax error instead of a semantic error, and that then leading to an uncaught error in dartdev and flutter_tools.

Anyway, I'll address the comments. 👍 Thanks for the review!

[dcharkes]

I've changed the PR to not make it a syntactic error when the field is missing. E.g. constructing the CodeConfig will succeed.

[mkustermann]

@dcharkes I see you didn't take my suggestion but implemented it like this:

  IOSConfig.fromHookConfig(HookConfig config)
      : _targetVersion = config.json.optionalInt(_targetIOSVersionKey),
        _targetSdk = switch (config.json.optionalString(_targetIOSSdkKey)) {
          null => null,
          String e => IOSSdk.fromString(e)
        };
}
extension IOSConfigSyntactic on IOSConfig {
  IOSSdk? get targetSdkSyntactic => _targetSdk;
  int? get targetVersionSyntactic => _targetVersion;
}

This means accessing config.codeConfig no longer checks everything, it delays errors until one accesses e.g. config.codeConfig.iOSConfig.targetSdk. This is a little bit suboptimal. It can also hide issues because code often may not access the target api levels, so errors wouldn't trigger, but if the protocol mandates it, I'd prefer the error triggers.

I view config.codeConfig as the protocol extension and accessing it should "handle all the syntax errors".

Similarly

class AndroidConfig {
  int get targetNdkApi => _targetNdkApi!;
  final int? _targetNdkApi;
  AndroidConfig({
    required int targetNdkApi,
  }) : _targetNdkApi = targetNdkApi;
}

If we have an AndroidConfig then it should have a valid, non-nullable int targetNdkAApi. It seems wrong to make this nullable.

Another comment: We have now inconsistency between the class's constructors

class CodeConfig {
  CodeConfig(HookConfig config);
}
class IOSConfig {
   IOSConfig.fromHookConfig(HookConfig config); // <-- uses named .fromHookConfig
}

Could we keep constructor naming consistent?

The CodeConfig is the extension to the protocol, the subfields are usually done based on the json. Could we use the same as in e.g.

final class CCompilerConfig {
  late final Uri compiler;
  late final Uri linker;
  late final Uri archiver;

  CCompilerConfig({
    required this.archiver,
    required this.compiler,
    required this.linker,
    this.envScript,
    this.envScriptArgs,
  });

  factory CCompilerConfig.fromJson(Map<String, Object?> json) => CCompilerConfig(...);
  Map<String, Object> toJson() => { ... }

?

[dcharkes]

This means accessing config.codeConfig no longer checks everything

Yes, I've changed the fields being null to be a semantic error rather than a syntactic error. In order to be able to run the validate methods on config.codeConfig it should not throw in the constructor. Rather the validate method taking the CodeConfig should report an error.

Since we don't have a way to check for syntactic errors, I want to avoid introducing new possible syntactic errors. And you don't want the validate methods to operate on the json so that they could potentially validate syntactic errors.

If we have an AndroidConfig then it should have a valid, non-nullable int targetNdkApi. It seems wrong to make this nullable.

It's non-nullable for the semantic API. AndroidConfigs constructor takes a non-nullable int, and the getter in the public API is also non-nullable.

[mkustermann]

It's non-nullable for the semantic API. AndroidConfigs constructor takes a non-nullable int, and the getter in the public API is also non-nullable.

I imagine the json looking something like this

{
  'config' {
    'code': {
      'target_os': ...,
      'android' : ...;  // present if target_os == OS.android
      'ios' : ...;  // present if target_os == OS.ios
    }
  },
}

Now if the 'android' key is present we should have an AndroidConfig object in the API. If the 'android' key is not present there should be no AndroidConfig object (as there is no android config in the json). But right now it seems we have an AndroidConfig object but the json doesn't have an android config. That seems not right.

It can be avoided by either making codeConfig.android a AndroidConfig? or making codeConfig.android a AndroidConfig that throws if target os is not android (and possibly a codeConfig.hasAndroidConfig getter as I originally suggested. I'd be even fine if it's nullable. Code like this looks reasonable to me

if (codeConfig.targetOS == codeConfig.android) {
  use(codeConfig.android!);
}

That's somewhat similar to codeConfig.cCompiler being CCompilerConfig? (i.e. nullable)

[dcharkes]

Now if the 'android' key is present we should have an AndroidConfig object in the API. If the 'android' key is not present there should be no AndroidConfig object (as there is no android config in the json). But right now it seems we have an AndroidConfig object but the json doesn't have an android config. That seems not right.

One step at the time. We want to get there. But I want to restructure the API how we want it to be first, before tackling the JSON. I don't want 5 JSON format breaking changes.

It can be avoided by either making codeConfig.android a AndroidConfig?

That's not the semantic API we want to give to users.

or making codeConfig.android a AndroidConfig that throws if target os is not android

The CodeConfig.androidConfig does exactly that. 🤷 It throws if the target OS is not android.

That's somewhat similar to codeConfig.cCompiler being CCompilerConfig? (i.e. nullable)

The CodeConfig.cCompiler config is semantically nullable, CodeConfig.androidConfig is semantically non-nullable and is guarded by CodeConfig.targetOS == OS.android.

[dcharkes]

Another comment: We have now inconsistency between the class's constructors

class CodeConfig {
  CodeConfig(HookConfig config);
}
class IOSConfig {
   IOSConfig.fromHookConfig(HookConfig config); // <-- uses named .fromHookConfig
}

Could we keep constructor naming consistent?

👍

We never had a CodeConfig constructor that took it's constituents, because protocol extensions have a setup method. The things nested in a protocol extension do have a constructor with named fields because they are arguments to this setup method. That seems somewhat weird, Maybe the setup method should just take a single CodeConfig argument and we add a named argument constructor and a toJson.