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

[native_assets_cli] User defines #39

Open
dcharkes opened this issue May 12, 2023 · 16 comments
Open

[native_assets_cli] User defines #39

dcharkes opened this issue May 12, 2023 · 16 comments
Labels
P2 A bug or feature request we're likely to work on package:native_assets_cli

Comments

@dcharkes
Copy link
Collaborator

dcharkes commented May 12, 2023

We'd like users to be able to configure the build of their native dependencies.

We'd probably want to support multiple ways of providing this user-defined config, similar to package:cli_config. However, in this case we also need to reason about the command-line API of the launcher scripts (dartdev and flutter_tools).

One downside of the package:cli_config standard is that it requires passing --config= to pass the config file. Instead (or in addition) we might want to consider a default place to pass configuration. This could be the pubspec.yaml to have a single location for configuration.

Two options would be (1) a top level

# other entries

build_config:
  some_native_asset_package:
    some_global_flag: foobar
    release:
      optimize: true
    debug:
      optimize: false

[...]

I think keeping it as a single config, or allowing package authors to chose where to read from, would be nice. Otherwise there might be a ton of files in a repo to keep track of, and I would imagine most Dart developers think of pubspec.yaml when they think of dependencies and their configuration. A similar statement is true for Rust, where you often configure features and settings alongside your dependencies.

Originally posted by @GregoryConrad in dart-lang/sdk#50565 (comment) (modified by me)

@jonasfj probably has something to say about listing arbitrary key-values under the packages in dependencies.

Alternatively, we could make a toplevel build_config.yaml (which corresponds in naming to build.dart).

(Priority: not part of MVP, but will add this soon after.)

@dcharkes dcharkes added P3 A lower priority bug or feature request package:native_assets_cli labels May 12, 2023
@mkustermann
Copy link
Member

My original thought was that the configuration system is based on hierarchical json and modification/patching of it:

  • build configuration has abi, linking-preference, ...
  • users can modify this configuration by e.g. specifying values on the command line or from config files
  • users can modify this configuration per-package

The final build configuration for a particular package's build.dart invocation is the default json configuration from the build/bundle tool, modified with the user-defined global values, modified with user-defined per-package values.

Those user-defined global or per-package settings can come from command line arguments to the build (e.g. --config build.cxx.optlevel=3 for global setting, --config foo:build.cxx.optlevel=3 for package level or put those into a file via --config native-build-config.json).

The actual build/bundling tool (e.g. dart build) would only know about very few config settings (e.g. target abi, ...) and blindly pass-through all other settings to the individual package's build.dart invocation.

The Dart code that is operating on such json can use wrappers on the json that view into things they care about (e.g. via inline classes):

class BuildConfig {
   final Map<String, Object> config;
}

extension on BuildConfig {
  CxxConfig get cxxConfig => CxxConfig(config['build.cxx']);
}

inline class CxxConfig {
  final Map<String, Object> config;

  int get optimizationLevel => ...;
}

Then we have flexible configuration mechanism:

  • the user can pass arbitrary configuration to the build/bundling tool
  • the build/bundling tool passes such configuration transparently (without knowing keys/values) down to build.dart invocations (it will only add specific things, e.g. target abi, linking preference)
  • the individual build.dart implementations can use helper packages that can have structured view on json-like config. (And one can pass sub-parts of the hierarchy down to those helper packages (e.g. BuildConfig.cxxConfig) - so package:c_compiler doesn't get a config containing rust related things)

@dcharkes
Copy link
Collaborator Author

dcharkes commented May 12, 2023

Having configuration that can be used by multiple targets such as optimization level is a good idea indeed. Then if some package wants some specific configuration, they can just use their own package-name as the key. (That at least rules out using dependencies in pubspec yaml.)

My original thought was that the configuration system is based on hierarchical json and modification/patching of it:

  • build configuration has abi, linking-preference, ...
  • users can modify this configuration by e.g. specifying values on the command line or from config files
  • users can modify this configuration per-package

I was thinking that instead of modification it would be in non-conflicting. The final config:

out_dir: ...
target: ...
dependency_metadata:
  my_package: // some meta data set by the build.dart of my_package, used in later native asset builds
    key: value
c_compiler:
  ar: ...
  cc: ...
  ld: ...
user_defines:
  cxx: // generic which can be used by any package
    optlevel: 3
  my_package: // likely only used during the build.dart of my_package.
    key: value

The user_defines is populated from the dart run&flutter run defines, --build_config=..., pubspec.yaml -> build_config.
I'm not sure yet about the env vars from dart run&flutter run, see #32

But maybe nesting them in user_defines is the wrong thing to do and we should put this type of config in the top-level, so that it can be used to override whatever dart run and flutter run is trying to pass to the various build.dart builds. (One would be able to override out_dir? c_compiler? do we want that? What about overriding dependency_metadata? Overriding target will most likely break the build.)

If we nest in user_defines (or user_metadata, or ...). Then it's up to the packages to decide whether they consider something as overridable.

@jonasfj
Copy link
Member

jonasfj commented May 16, 2023

For what it's worth, I think it's fine to add a top-level key in pubspec.yaml like:

build_config:
  <package>:
    <config-key>: <config-value>

  my_package_with_native_assets:
    my_config_option: true
    optlevel: 3
    ...

It's sometimes nice not to have a lot of top-level config files.

Nesting it under dependencies is very bad idea.

Do consider that a package published on pub.dev might have a build_config section, but author of said package should be a aware that build_config will be ignored when someone depends on their package. If foo contains build_config and bar depends on foo, the build_config in foo will be ignored when building bar (otherwise, you'll have a hard time merging it).

@dcharkes
Copy link
Collaborator Author

dcharkes commented Sep 29, 2023

My current thinking based on the above.

End user specification

Pubspec

End users can specify config in the pubspec

native_assets:
  defines:
    <package>:
      <config-key>: <config-value>

    my_package_with_native_assets: # Only passed to my_package_with_native_assets's build.dart
      enable_some_feature: true

    _all: # Passed to every build.dart   (`package:_all` should never exist.)
      optlevel: 3

Skip for now: conditional user-defines

If we would like to support branching on build_mode / architecture / other already predefined keys, we could consider adding something like filters:

native_assets:
  defines:
    my_package_with_native_assets: # Only passed to my_package_with_native_assets's build.dart
      _where:
        _build_mode: release
        optimize: true

However, the downside is that this kind of logic doesn't scale. What if we add more predefined things to the config passed in to build.dart, we'd need to add it.

We kind of want to opposite of the metadata here, metadata is output by build.dart and flows from dependencies to dependents. "Inverse metadata" would be a programmatic way to have the root package build.dart give user-defines to all dependency build.dart invocations. However, sticking it into build.dart is a bad idea, because then it looks like that the package itself would want to bundle native assets. So maybe it would be better to introduce something like a user_defines.dart.

I think for now we should skip conditional user-defines in the pubspec, and do these via command-line args.

@GregoryConrad What is your use case for conditional user-defines?

Command-line args

The config in the pubspec.yaml can be overridden in invocations of dart / flutter similar to how user-defines are specified for dart code.

The defines for dart code are with -D

$ dart --help -v
[...]
--define=<key>=<value> or -D<key>=<value>
  Define an environment declaration. To specify multiple declarations,
  use multiple instances of this option.

(And for flutter --dart-define.)

Since these arguments are already taken, we could use --native-assets-define=<key>=<value>.

optional: command-line args from file

We could follow Flutters --dart-defines-from-file and support --native-assets-defines-from-file as well.

Not: Environment variables

We should stop passing through environment variables to prevent builds accidentally depending on them and breaking build caching that way.

Implementation

native_assets_cli

The BuildConfig gets a user-defines key:

out_dir: ...
target: ...
dependency_metadata:
  my_package: // some meta data set by the build.dart of my_package, used in later native asset builds
    key: value
c_compiler:
  ar: ...
  cc: ...
  ld: ...
user_defines:
  optlevel: 3
  enable_some_feature: true

These are defines just for the build for the native assets of this package.

native_assets_builder

Needs an extra argument for build and dryRun, probably something like:

class UserDefines {
  final Map<String, Object> all;
  final Map<String, Map<String, Object>> perPackage;
}

And it gets the right information per package to put in the BuildConfig for that package.

(Note that if we ever add conditional defines in pubspec or a script, they should not be able to depend on the fields not passed to dry-run such as architecture and API levels.)

Launchers

dartdev (the implementation of dart run and dart build) and flutter_tools (the implementation of flutter run and flutter build) take the info from the pubspec and --native-assets-define=

Design questions

native-assets defines vs build defines

What naming should we use. Should we settle on native-assets or on build?

We settled on build.dart as the name for the top-level file. The idea was that we could add other "builds" besides native-assets later.

The question is whether these other types of "builds" would benefit from defines as well. If they do, we could settle on the "build" name here:

build_defines:
  <package>:
    <config-key>: <config-value>

dart run --build-define=<key>=<value>
flutter run --build-define=<key>=<value>

cc @mkustermann @jonasfj @bkonyi @stuartmorgan @mit-mit

@dcharkes dcharkes added P2 A bug or feature request we're likely to work on and removed P3 A lower priority bug or feature request labels Sep 29, 2023
@dcharkes
Copy link
Collaborator Author

cc @simolus3

@simolus3
Copy link
Contributor

The yaml-based end-user specification covers my use-cases, but given that there are more complex ideas mentioned here (like conditional user defines), I want to bring up the idea of allowing users to configure their dependencies in Dart? E.g. if I was able to put say a build_config.dart in my application with content like:

void main(List<String> args) async {
  final global = await BuildConfig.fromArgs(args);

  global.configure(allPackages, (config) {
    config.addToList('c_compiler.flags', '-Wall');
    // There could also be type-safe extensions on config to describe common options
    config.linker.preferredMode = Linker.staticLibrary;
  });

  // Or configure a specific package
  global.configure(package('sqlite3'), (config) async {
    config['source'] = 'url';
    config['url'] = 'https://sqlite.org/2023/sqlite-amalgamation-3430100.zip';

    // Since this is code, I could make some options conditional
    if (global.buildMode == BuildMode.release) {
      // Read additional options we want from a file
      final optionsFile = global.trackInput('assets/release_options.txt');
      final content = await optionsFile.readAsString();
      config.cCompiler.addDefine('foo': content);
    }
  });
}

With a nice DSL and typed extensions to configure common things like the C toolchain, this may be as convenient as a declarative configuration in yaml. A benefit is that it can scale to complex configuration options much better.

@GregoryConrad
Copy link

GregoryConrad commented Sep 29, 2023

@GregoryConrad What is your use case for conditional user-defines?

I can't remember what my exact use case was, but a big one is enabling Rust cargo features based on a user's preferences/needs. In C/C++ land, another might be to supply a value to clang -D MY_DEFINE=xyz

@dcharkes
Copy link
Collaborator Author

dcharkes commented Oct 7, 2023

RE: build_config.dart. The user-defines are a special case of a more general problem, namely inverse meta-data: data flowing from dependencies to source.

One other use case would be package:messages, where build.dart does resource shaking, but the usages of that package declare message-files that need to be resource shaken. A package dependency graph could look something like:

  • package:my_foo depends on package:messages, and package:my_foo has a file my_foo/data/messages.json
  • package:my_bar depends on package:messages, and has a file my_bar/data/messages.json
  • package:my_app depends on my_bar and my_foo, and also defines a my_app/data/messages.json
  • package:messages has a build.dart which takes these message files and combines them so that localization works at runtime. That build.dart needs access to these three message files.

The three message files could be defined in the build_config.dart of the three packages.

User defines are basically the build_config.dart for the root package. So user-defines are a special case.

(Having two files build.dart and build_config.dart is not that pretty, so we should consider having one file instead. But then it would be better to have a single file that implements an interface rather than having a main function. #152)

Related:

cc @mosuem

@jonasfj
Copy link
Member

jonasfj commented Oct 9, 2023

E.g. if I was able to put say a build_config.dart in my application with content like...

Then any change to build rules easily becomes a breaking change for the entire ecosystem.

I'm generally very concerned if using native-assets requires using an API that isn't specified in a dart: library. Because changing such an API easily becomes a breaking change for the entire ecosystem -- while side-stepping established process for breaking changes in the Dart SDK.

Of course build_config.dart could write out a JSON file with the configuration and one or more packages could provide a high-level API for configuring stuff.


The user-defines are a special case of a more general problem, namely inverse meta-data: data flowing from dependencies to source.

@dcharkes, I like your example, but when you say: "user-defines" do you mean:

  • Inverse meta-data from the root package ONLY to its dependencies, or,
  • Inverse meta-data from any package to its direct dependencies.

Is it only the root package that is allowed to have "user-defines", and are they "user-defines" specified in dependencies and transitive dependencies ignored?
(like how dev_dependencies from dependencies and transitive dependencies are ignored).


@sigurdm this is a bit similar to what is proposed in dart-lang/pub#3917

Should we seriously consider making a generic configuration mechanism?


@dcharkes, I would suggest avoiding magic values like _all. I'm sure we'll enable people to make the _all package, and things will be bad 🙈 🙈 🙈

    _all: # Passed to every build.dart   (`package:_all` should never exist.)
      optlevel: 3

If you must just do:

native_assets:
  defines:
    <package>:
      <config-key>: <config-value>

    my_package_with_native_assets: # Only passed to my_package_with_native_assets's build.dart
      enable_some_feature: true

  define_for_all: # Passed to every build.dart
    optlevel: 3

But it might also be reasonable to ship the initial MVP without a _all or define_for_all keys, people will be fine copy/pasting. You also want features you can add later 🤣

In fact, it might be worth down scoping and not shipping with "user-defines" initially, is there any reason it can't be added at a later stage?

@dcharkes
Copy link
Collaborator Author

@dcharkes, I like your example, but when you say: "user-defines" do you mean:

  • Inverse meta-data from the root package ONLY to its dependencies, or,
  • Inverse meta-data from any package to its direct dependencies.

The latter. The former would not support package:localization, because the root package is package:my_app.

A third option would be: Inverse meta-data from any package to its transitive dependencies. Some conceptual use case that would benefit from that is a bunch of packages standardizing on some config keys, and then some package similar to flutter_lints would provide a bunch of default "inverse meta data" (we should probably just call it config). That being said, it's a bit of a contrived example, so only showing it to direct dependencies would probably be better.

I would suggest avoiding magic values like _all. I'm sure we'll enable people to make the _all package, and things will be bad 🙈 🙈 🙈

If the config is a config.dart or build_config.dart script, and not a yaml, we have an API, so we don't have to resort to magic config keys in a yaml file.

In fact, it might be worth down scoping and not shipping with "user-defines" initially, is there any reason it can't be added at a later stage?

The native assets (dynamic library bundling), can be shipped without indeed.
I was pushing on this to unblock "data assets" to unblock @mosuem on the localization package.

@jonasfj
Copy link
Member

jonasfj commented Oct 11, 2023

From options:

  • (A) Inverse meta-data from the root package ONLY to its dependencies, or,
  • (B) Inverse meta-data from any package to its direct dependencies.
  • (C) A third option would be: Inverse meta-data from any package to its transitive dependencies.

I thought you wanted (A). I see little point in doing (C). Because if you know that package:foo is in your transitive dependencies and you want to supply configuration for it, then you can simply add a dependency on package:foo.

I suppose (C) makes some sense if (C) is an optional dependency, but we don't really have that concept. And if you supply config to package foo, you should probably have a dependency constraint on foo -- otherwise, you package will break if the next major version of foo requires config on a different format / shape.

@dcharkes
Copy link
Collaborator Author

dcharkes commented Jan 30, 2025

Now that we have introduced pub workspaces and share the native asset builds across all packages in the workspace, the user-defines should be workspace wide. Otherwise the cache will be invalidated.

If we would have user defines per root package, you'd end up in the following scenario.

Suppose your workspace is:

  • my_app1/ (contains pubspec with user-defines to build intl4x locally)
  • my_app2/ (contains pubspec with user-defines to fetch intl4x)
  • intl4x/

And now you do:

  1. my_app1$ dart build bin/my_app1.dart (builds assets for intl4x)
  2. my_app2$ dart build bin/my_app2.dart (rebuilds assets for intl4x, because same BuildConfig, but different user-defines, so invalidated cache)
  3. my_app1$ dart build bin/my_app1.dart (rebuilds assets for intl4x, same reason.)

This leads me to believe that we should have user-defines in the workspace pubspec.yaml (if we have them in the pubspec.yaml).

This will lead to:

  1. my_app1$ dart build bin/my_app1.dart (builds assets for intl4x)
  2. my_app2$ dart build bin/my_app2.dart (uses cache, same user-defines)
  3. my_app1$ dart build bin/my_app1.dart (uses cache, same user-defines)

(Side note: If we enable passing in user-defines on command-line, that will necessarily also lead to cache invalidations. I don't see an obvious way around that.)

@GregoryConrad
Copy link

@dcharkes could you key the cache of the build output by using a hash of the user defines? That way you can have multiple different user defines (and CLI wouldn’t be a problem either for cache invalidation).

@dcharkes
Copy link
Collaborator Author

@dcharkes could you key the cache of the build output by using a hash of the user defines? That way you can have multiple different user defines (and CLI wouldn’t be a problem either for cache invalidation).

@GregoryConrad That has two significant downsides:

  1. It would lead to a very long list of output directories filling up disk space. (This could be mitigated by deleting after a week.)
  2. A hook can no longer internally cache. If you compile some c library or download some c library from a CDN, and you pass in some different user define which is ignored by the hook, then you'd be in a completely different output directory, and have no access to the previous build or download of the C library. (I don't believe this can be mitigated.)

The design approach with the recent refactoring with BuildConfig nested in BuildInput is that BuildConfig defines the output directory uniquely and that you can rely on that being reusable so that you can cache things.

Do you have any use cases in which user-defines (a) per pub workspace or (b) per invocation is not sufficient?

@GregoryConrad
Copy link

Do you have any use cases in which user-defines (a) per pub workspace or (b) per invocation is not sufficient?

No use cases at the moment; just figured I’d float the idea. Main concern would be a mono repo where multiple dependents reuse the same native asset differently, but I can’t say I have that need at the moment.

@dcharkes
Copy link
Collaborator Author

From #1962 (comment): We want to minimize the impact of user-defines on cache invalidation. So we want to make the user-defines scoped per package, and not give access to the user-defines meant for other packages.

(If some packages want to collaborate on a user-define. They could pick one package that consumes it, and makes it available as metadata for the other packages.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on package:native_assets_cli
Projects
Status: No status
Development

No branches or pull requests

5 participants