Skip to content

[native_assets_cli] User defines #39

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

Closed
dcharkes opened this issue May 12, 2023 · 26 comments
Closed

[native_assets_cli] User defines #39

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

Comments

@dcharkes
Copy link
Collaborator

dcharkes commented May 12, 2023

2025-04-17 User-defines in the pubspec are support (example, example). Command-line args are not supported yet.

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:hooks 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.

@fzyzcjy
Copy link

fzyzcjy commented Mar 9, 2025

Hi, is there any updates? When upgrading internal tests of flutter_rust_bridge (fzyzcjy/flutter_rust_bridge#2606), I find the new hook/build.dart does not seem to get env var anymore, thus looking forward to having an alternative way, thanks!

@rainyl
Copy link
Contributor

rainyl commented Mar 9, 2025

Also waiting this feature.

I find the new hook/build.dart does not seem to get env var anymore

@fzyzcjy It may because newer version of dart will not pass full environment variable to Platform.environment, refer to #2077 (comment)

@fzyzcjy
Copy link

fzyzcjy commented Mar 9, 2025

@rainyl Yes, I come to this issue from link in #2078 when searching

@mosuem
Copy link
Member

mosuem commented Mar 10, 2025

My current workaround in package:intl4x (after environment variables were not an option any more) is to read the pubspec of the root package. This is a bit hacky, but works so far, see https://github.com/dart-lang/i18n/blob/bdeec2590a1fda55d8ca95616aa8ff4c101765de/pkgs/intl4x/hook/build.dart#L20.

@dcharkes
Copy link
Collaborator Author

dcharkes commented Mar 10, 2025

My current workaround in package:intl4x (after environment variables were not an option any more) is to read the pubspec of the root package. This is a bit hacky, but works so far, see https://github.com/dart-lang/i18n/blob/bdeec2590a1fda55d8ca95616aa8ff4c101765de/pkgs/intl4x/hook/build.dart#L20.

hook:
  intl4x:
    buildMode: fetch

As noted in designs earlier in the discussion, indeed we'd like to be able to have user-defines in the root package pubspec.

As for @mosuem's design: This is great if (a) we want to share user-defines between all types of hooks, and (b) we never want something else than user-defines in the pubspec. Otherwise we should nest a bit deeper.

RE a: I think probably we want to always share defines across all hooks, thinking that they would mostly work in tandem (build, link, fetch). So having to repeat a define for different hooks would be painful. So no need for adding a build or link key under hook.

RE b: I can't currently think of a need for other things in the pubspec, but we'd probably want to design pubspec use in a way that we don't conflict with something in the future. So maybe we should settle on:

hook:
  defines: # map from package name to key-value map.
    intl4x: # package name
      build_mode: fetch # key -> arbitrary JSON value. Keys should be snake_cased.

(And CLI defines should override any root package pubspec defines as discussed earlier in the thread.)

@simolus3

This comment has been minimized.

@dcharkes
Copy link
Collaborator Author

@simolus3 In the design sketches earlier in this issue we were thinking of having individual defines on the CLI rather than referring to a yaml file. They are functionally equivalent but I'd want to avoid having both. Having a yaml file path requires users to write a file if they want to set a single define for a single invocation. Also using a file path requires some other build system invoking Flutter or Dart to write a file before doing the invocation. What's your use case for having a separate yaml instead of passing the individual defines on the command line?

@simolus3
Copy link
Contributor

I don't have a preference for the yaml files over CLI flags, I just didn't see the earlier discussion introducing them. As long as we have a convenient way for users to configure defines as well as an easy way for us package authors to override them for tests, I'm happy :)

@dcharkes dcharkes self-assigned this Apr 3, 2025
@dcharkes dcharkes moved this to In Progress in Native Assets Apr 3, 2025
auto-submit bot pushed a commit that referenced this issue Apr 4, 2025
Bug: #39

Note passing in the user-defines should be done by the Dart and Flutter SDK, they need to decide where to take the command-line arguments, and how to read from the `pubspec.yaml`.

The `NativeAssetsBuildRunner` provides a suggestion about where to put user-defines in the `pubspec.yaml`:

```yaml
hooks:
  user_defines:
    my_package:
      user_define_key: user_define_value
      user_define_key2:
        foo: bar
    some_other_package:
      user_define_key3: user_define_value3
```

Moreover, it provides a helper function that can be reused in SDKs after the pubspec yaml is parsed.

### Design choices

(According to the discussion on the bug.)

* User-defines are name-spaced per package, we don't want new user-defines to invalidate the cache for other packages.
* User-defines can be provided for any package in the dependency graph.
* If user-defines need to be shared across packages, the common dependency package can export it as metadata.

### Test

This change is tested by invoking the build with user-defines, and in the hook failing if the defines are not available.
github-merge-queue bot pushed a commit to flutter/flutter that referenced this issue Apr 11, 2025
This PR is the dual of
https://dart-review.googlesource.com/c/sdk/+/420700 in the Dart SDK.

This PR adds support for user-defines for hooks in the root package
`pubspec.yaml`.

```yaml
hooks:
  user_defines:
    hook_user_defines: # package name
      magic_value: 1000
```

For more design considerations see:

* dart-lang/native#39

### Testing

This PR is covered by
`test/integration.shard/isolated/native_assets_test.dart`.

In this PR, we add a new test project
`dev/integration_tests/hook_user_defines/` and add a dependency on that
project in the mentioned test.

The integration test already covers `flutter run`, `flutter test`, and
`flutter build`.
auto-submit bot pushed a commit that referenced this issue Apr 11, 2025
Changes the test-data to have user-defines in the workspace pubspec, such that `dart test` etc. work on that project.

Keep the (ineffective) user-defines in the project pubspec, because the integration tests copy that pubspec and remove the `resolution: workspace` entry.

* #39
* #2187
github-merge-queue bot pushed a commit to flutter/flutter that referenced this issue Apr 11, 2025
If pub workspaces are used, then native assets are shared across the
whole pub workspace. Using a different user-define, invalidates the
cache. Therefore, we want to avoid having different user-defines in
different pubspecs belonging to the same workspace. So, only use the
workspace pubspec for user-defines.

Analogous Dart SDK CL:
https://dart-review.googlesource.com/c/sdk/+/422101

### Testing

Covered by the integration tests added in
#166940 for user-defines.

### Design discussion

We could allow adding user-defines in root package pubspecs later, but
this will likely deteriorate caching behavior (either invalidating the
cache often, or requiring a cache directory per root package, which
increases latency for new root packages).

Also, we'd likely add command-line user-defines later, these are
expected to deteriorate caching behavior.

For design discussion, refer to:

* dart-lang/native#39
* dart-lang/native#2187
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Apr 11, 2025
Native assets are cached for the whole pub workspace. So to avoid
invalidating caches for running hooks from different root packages,
only read user-defines from the workspace `pubspec.yaml`.

Bug: dart-lang/native#39
Bug: dart-lang/native#2187

Change-Id: I3aeb91455a418004d3e28c231dc1f5d002c15739
Cq-Include-Trybots: luci.dart.try:pkg-linux-debug-try,pkg-linux-release-arm64-try,pkg-linux-release-try,pkg-mac-release-arm64-try,pkg-win-release-arm64-try,pkg-mac-release-try,pkg-win-release-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/422101
Auto-Submit: Daco Harkes <[email protected]>
Reviewed-by: Hossein Yousefi <[email protected]>
Commit-Queue: Daco Harkes <[email protected]>
@dcharkes
Copy link
Collaborator Author

dcharkes commented Apr 14, 2025

We've added support for user-defines in the pubspec.

However, this means such user-defines will be automatically committed to git.

We do have use-cases where we want user-defines to point to a path on the system (e.g. libsqlite3 sources that are git-cloned into some directory).

One way to have non-git-committed user-defines would be to a hook read a file in the workspace (#2187), add that file as dependency, and have such file git-ignored.

  • .hook_user_defines.yaml (.gitignored user-defines)
  • pubspec.yaml (committed user-defines)

The only downside is that every hook that wants to support non-committed user-defines needs to define it's own place where it's looking for user-defines.

If we'd like to standardize non-git-committed disk-persisted user-defines, we could let the SDKs read it, or have it be read in our hook helper package. Possible locations to standardize:

  • hook/.user_defines.yaml (If pub workspaces are used, this means we'd get a hook/ directory next to pkgs/)
  • .hook_user_defines.yaml

(Thinking about it, if we'd have #2187, we could implement user-defines inside the hooks instead of having it part of the protocol. If we'd want to only support file-based user-defines and no command-line arguments user-defines. I'm thinking we might have use cases for command-line argument user-defines, those will always need to come from flutter/Dart. But we could still opt to read the pubspec user-defines in the hooks instead of in the SDKs.)

If we do end up having a git-ignored user-defines file such as hook/.user_defines.yaml, should we consider having the git-committed user-defines in hook/user_defines.yaml instead of inside pubspec.yaml?

Edit: Thinking about relative paths:

(Thinking about it, if we'd have #2187, we could implement user-defines inside the hooks instead of having it part of the protocol. If we'd want to only support file-based user-defines and no command-line arguments user-defines. I'm thinking we might have use cases for command-line argument user-defines, those will always need to come from flutter/Dart. But we could still opt to read the pubspec user-defines in the hooks instead of in the SDKs.)

If the command-line CLIs of Dart/Flutter take user-defines and users pass in relative paths there, the protocol should have a current-working-directory field to resolve those paths against. This field should be null if there are no command-line user-defines. Even better we should consider making the user defines in the JSON:

  "user_defines": {
    "command_line_arguments": {
      "user_defines": {
        "my_data": "data/my_data.txt"
      },
      "working_directory":  "~/src/foo/bar/pkgs/my_app/"
    },
    "workspace_pubspec": {
      "user_defines": {
        "foo_data": "pkgs/some_package/data/my_data.txt"
      },
      "workspace": "~/src/foo/bar/"
    }
  },

Then it would be clear how to resolve relative paths.

Maybe we should even consider making the user-defines source-agnostic:

  "user_defines": [
    {
      "defines": {
        "my_data": "data/my_data.txt"
      },
      "base_path":  "~/src/foo/bar/pkgs/my_app/"
    },
    {
      "defines": {
        "foo_data": "pkgs/some_package/data/my_data.txt"
      },
      "base_path": "~/src/foo/bar/"
    }
  ],

auto-submit bot pushed a commit that referenced this issue Apr 16, 2025
Bug: #39
Closes: #2187
Closes: #2201

### Relative paths in user-defines accessible as absolute paths in hooks

This PR enables resolving relative paths in user-defines as absolute paths. The `input.json` now is structured as follows:

```json
{
  "user_defines": {
    "workspace_pubspec": {
      "base_path": "/private/var/folders/2y/mngq9h194yzglt4kzttzfq6800klzg/T/QqYcWN/user_defines/pubspec.yaml",
      "defines": {
        "user_define_key": "user_define_value",
        "user_define_key2": {
          "foo": "bar"
        },
        "some_file": "assets/data.json"
      }
    },
  },
}
```

We don't want to expose the workspace/pubspec path (except for resolving user-defines), so this can be done in the Dart API as follows:

```dart
extension type HookInputUserDefines._(HookInput _input) {
  /// The value for the user-define for [key] for this package.
  ///
  /// This can be arbitrary JSON/YAML if provided from the SDK from such source.
  /// If it's provided from command-line arguments, it's likely a string.
  Object? operator [](String key) => // already present before this PR

  /// The absolute path for user-defines for [key] for this package.key
  ///
  /// The relative path passed as user-define is resolved against the base path.
  /// For user-defines originating from a JSON/YAML, the base path is this
  /// JSON/YAML. For user-defines originating from command-line aruments, the
  /// base path is the working directory of the command-line invocation.
  ///
  /// If the user-define is `null` or not a [String], returns `null`.
  Uri? path(String key) => // added in this PR.
}
```

This PR does _not_ yet add support for command-line user-defines. But the JSON has been set up so that it's extensible.

### Move reading user-defines from flutter_tools/dartdev to `NativeAssetsBuildRunner`

This PR changes the responsibility to read the user-defines from the pubspec to the `NativeAssetsBuildRunner`.

1. This ensures all SDKs have the same logic for reading user-defines
2. This ensures all SDKs have the same caching behavior for files containing user-defines changing on disk.

This PR adds the pubspec.yaml in the `HookOutput.dependencies` to ensure the build runner is reinvoked if the pubspec changes.

The API for the `NativeAssetsBuildRunner` changes to take the `pubspec.yaml` uri rather than user-defines. Consequently, this PR will need a manual roll into dartdev and flutter_tools.
@dcharkes
Copy link
Collaborator Author

User-defines in the pubspec are support (example, example).

Let's track adding command-line arguments in a different issue:

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:hooks
Projects
Status: Done
Development

No branches or pull requests

8 participants