dart-lang / native

Dart packages related to FFI and native assets bundling.
BSD 3-Clause "New" or "Revised" License
93 stars 33 forks source link

[native_assets_cli] Dependencies should be per asset #1208

Open dcharkes opened 2 weeks ago

dcharkes commented 2 weeks ago

Currently, the dependencies is a list of dependencies for all assets together. (This was modeled after the Rust build.rs.) But in the context of hot-reload/hot-restart we might want to only rebuild some assets (https://github.com/dart-lang/native/issues/1207). So then we'd need to know the dependencies per asset.

Some open questions:

  1. What if multiple assets are produced by the same list of dependencies? Should we try to deduplicate such list of dependencies?
  2. We've been considering making assetId optional. E.g. an assetId would only be needed for things referenced from Dart code. And not for JARs for example. But we'll likely need some way to uniquely identify an asset.

Version skew:

dcharkes commented 3 days ago

We should probably merge the addition of dependencies and assets into a single add function.

Merging in addAsset, optional named arg

  /// Adds [Asset] produced by this build or dry run.
  ///
  /// If provided, [dependencies] adds files used to build this [asset]. If any
  /// of the files are modified after [timestamp], the build will be re-run. If
  /// omitted, and [Asset.file] is outside the [BuildConfig.outputDirectory], a
  /// dependency on the file is added implicitly.
  ///
  /// If the [linkInPackage] argument is specified, the asset will not be
  /// bundled during the build step, but sent as input to the link hook of the
  /// specified package, where it can be further processed and possibly bundled.
  void addAsset(
    Asset asset, {
    Iterable<Uri>? dependencies,
    String? linkInPackage,
  });

  /// Adds [Asset]s produced by this build or dry run.
  ///
  /// If provided, [dependencies] adds files used to build these [assets]. If
  /// any of the files are modified after [timestamp], the build will be re-run.
  /// If omitted, and [Asset.file] is outside the [BuildConfig.outputDirectory],
  /// a dependency on the file is added implicitly.
  ///
  /// If the [linkInPackage] argument is specified, the assets will not be
  /// bundled during the build step, but sent as input to the link hook of the
  /// specified package, where they can be further processed and possibly
  /// bundled.
  void addAssets(
    Iterable<Asset> assets, {
    Iterable<Uri>? dependencies,
    String? linkInPackage,
  });

Usage:

    output.addAsset(
      NativeCodeAsset(
        package: 'add_asset_link',
        name: 'dylib_add_link',
        linkMode: builtDylib.linkMode,
        os: builtDylib.os,
        architecture: builtDylib.architecture,
        file: builtDylib.file,
      ),
      dependencies: [
        config.packageRoot.resolve('hook/link.dart'),
      ],
    );

Should the dependencies param be optional? In dryRun no dependencies have to be passed. And data assets which are verbatim in an assets/ dir which could be skipped copying to the output dir, could have an implicit dependency on themselves by default.

If it should be optional, it needs to be a named param.

Alternatively, we can make it a positional param.

wdyt @mosuem ?

(bad option) Keeping addDependencies separate from addAsset

  /// Adds file used by this link hook for creating [asset].
  ///
  /// If any of the files are modified after [timestamp], the link will be
  /// re-run.
  void addDependency(Asset asset, Uri dependency);

  /// Adds files used by this link hook for creating [asset].
  ///
  /// If any of the files are modified after [timestamp], the link will be
  /// re-run.
  void addDependencies(Asset asset, Iterable<Uri> dependencies);

Keeping addAsset and addDependency separate leads to two issues.

  1. An asset might be used in addDependency but never added with addAsset.
  2. addAsset and addDependency leads to awkward code where an asset always needs to be in a variable to be used in both:
    final asset = NativeCodeAsset(
      package: 'add_asset_link',
      name: 'dylib_add_link',
      linkMode: builtDylib.linkMode,
      os: builtDylib.os,
      architecture: builtDylib.architecture,
      file: builtDylib.file,
    );
    output
      ..addAsset(asset)
      ..addDependency(asset, config.packageRoot.resolve('hook/link.dart'));
  });
dcharkes commented 3 days ago

Nesting dependencies under assets will not fully work.

A build hook can also compile an executable that is then stored in the metadata, which is then used other build hooks.

For example (along the lines of https://github.com/flutter/flutter/issues/144259).

If we want to support hot-reload for those compiled shaders, the question is whether the my_shader_compiler.exe should be rebuilt or not. In this case likely not.

However, if we're cold starting, most likely we should check if any of the dependencies for the my_shader_compiler.exe was modified. (E.g. pub upgrade pulled in a new version of package:my_3d_engine.)

So we need to also store dependencies that are not tied to an asset, but tied to the hook itself. We could specify that these dependencies are ignored when hot-reloading/hot-restarting. But maybe there are use cases where something from the meta-data should actually be rebuilt in a hot-reload?

(If we want to go really wild, we could rethink whether storing an executable in meta data in an unstructured way is the right way to go. Then we could possibly track the dependencies per file that we possible store in metadata. But now we're building a full fledged build system. 😆 )

Thoughts @mkustermann @mosuem @HosseinYousefi?