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] Sharing work between build hooks #1379

Open
dcharkes opened this issue Jul 30, 2024 · 4 comments
Open

[native_assets_cli] Sharing work between build hooks #1379

dcharkes opened this issue Jul 30, 2024 · 4 comments
Labels
P2 A bug or feature request we're likely to work on package:native_assets_cli

Comments

@dcharkes
Copy link
Collaborator

Build hooks are per OS/arch. However, if assets are identical across multiple invocations, and the work to produce those assets is non-trivial, such work should be shared.

Examples:

  • Data assets which are "compiled" (svg -> tessellated, 3d model -> bytes, translation json).
  • Native code assets which are downloaded as fat binaries (e.g. containing already MacOS arm64 and MacOS x64)

Non-examples:

  • Native code assets different per OS and per arch.
  • Data assets that are on disk already and are simply reported.

We have multiple avenues of sharing the work:

  1. Add documentation that the actual work should happen in .dart_tool/<your-package-name>.
  2. Make the output directory per asset type, and give the same output directory for data assets for all invocations.

Both of these options require hook writers to think about locking due to concurrent invocations of the build hook for different OSes/architectures. So whatever we come up with for #1319, should be made available in an API to hook writers.

Option (2) doesn't support sharing native code assets, so in for the second example, hook writers would still use option (1) even if we go for option (2).

@dcharkes dcharkes added P2 A bug or feature request we're likely to work on package:native_assets_cli labels Jul 30, 2024
@dcharkes
Copy link
Collaborator Author

A variant of 1. that's more user-friendly:

BuildConfig.sharedOutputDirectory that automatically lives in .dart_tool/native_assets_builder/<the-users-package-name>

@dcharkes
Copy link
Collaborator Author

dcharkes commented Aug 5, 2024

API design:

// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:io';

import 'package:native_assets_cli/locking.dart';
import 'package:native_assets_cli/native_assets_cli.dart';

const assetName = 'asset.txt';

Future<void> main(List<String> args) async {
  await build(args, (config, output) async {
    final sharedDirectory = config.outputDirectoryShared;
    final assetPath = sharedDirectory.resolve(assetName);
    await runUnderDirectoryLock(
      Directory.fromUri(sharedDirectory),
      () async {
        final packageName = config.packageName;

        if (!config.dryRun) {
          // Pretend we're downloading assets here.
          await File.fromUri(assetPath).writeAsString('Hello world!');
        }

        output.addAsset(
          DataAsset(
            package: packageName,
            name: assetName,
            file: assetPath,
          ),
        );
      },
    );
  });
}

We'll need to add outputDirectoryShared to the BuildConfig and LinkConfig.

auto-submit bot pushed a commit that referenced this issue Aug 6, 2024
This makes the native assets builder lock the build directories so that it can be invoked concurrently from multiple `dart` and `flutter` processes.

Closes: #1319

The implementation of the locking is in `package:native_assets_cli/locking.dart` contains `runUnderDirectoryLock`, so that we can reuse it for #1379. (#1379 Requires more changes though, it needs to also give users access to a shared directory via the `BuildConfig`.)
HosseinYousefi pushed a commit that referenced this issue Aug 19, 2024
This makes the native assets builder lock the build directories so that it can be invoked concurrently from multiple `dart` and `flutter` processes.

Closes: #1319

The implementation of the locking is in `package:native_assets_cli/locking.dart` contains `runUnderDirectoryLock`, so that we can reuse it for #1379. (#1379 Requires more changes though, it needs to also give users access to a shared directory via the `BuildConfig`.)
@dcharkes
Copy link
Collaborator Author

await runUnderDirectoryLock(

Thinking about this, the shared directory should be locked in the native assets builder, due to #1534. Otherwise, the native assets builder could be copying the assets while another hook invocation is trying to delete them at the same time. The native assets builder should only be deleting the hook after it has copied the necessary files to a temp/target location.

@dcharkes
Copy link
Collaborator Author

Note from discussion with @HosseinYousefi:

config.outputDirectoryShared is shared across all invocations of the same hook, config.outputDirectory is shared across only invocations that are 100% identical config. But we might have some cases in which things are not identical in a subset of configurations: for example debug/release doesn't make a difference in some native code, or images are only different per OS. The minimally shared approach is a safe default because users don't accidentally share the output dir when they shouldn't (for example if debug/release does make a difference but they only considered os/arch). However, the safe default could also be unnecessarily slow/wasteful. So we want to provide an easy way for users to go somewhere in between minimally and maximally shared.

The API could look something like:

Directory HookConfig.getSharedOutputDirectory({
  bool splitOnTargetOS: false, 
  bool splitOnTargetArchitecture: false,
  bool splitOnAndroidNdkVersion: false,
  // ...
});

This feel natural because one has to last what config aspects are important to branch on. However, users are more likely to accidentally share and have weird behavior.

The opposite would be quite weird:

Directory HookConfig.getOutputDirectory({
  bool shareAcrossTargetOS: false, 
  bool shareAcrossTargetArchitecture: false,
  bool shareAcrossAndroidNdkVersion: false,
  // ...
});

It would require users to list all the config params they don't care about. And this doesn't work with (a) us adding new parameters, and (b) some other embedder adding config parameters that we don't even know about.

To make things work for params that we don't know about:

Directory HookConfig.getOutputDirectory(List<Object> splitOn);

config.getOutputDirectory([config.targetOS, config.targetArchitecture, ...]);

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
None yet
Development

No branches or pull requests

1 participant