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] Merge LinkMode and DynamicLinkingType #1001

Closed
dcharkes opened this issue Mar 12, 2024 · 1 comment · Fixed by #946
Closed

[native_assets_cli] Merge LinkMode and DynamicLinkingType #1001

dcharkes opened this issue Mar 12, 2024 · 1 comment · Fixed by #946

Comments

@dcharkes
Copy link
Collaborator

dcharkes commented Mar 12, 2024

#946 (comment)

We should consider nesting DynamicLinkingType inside LinkMode.dynamic (e.g. DynamicLinkingMode will be a subtype of LinkMode).

Pros:

  • NativeCodeAsset will not have two fields LinkMode get linkMode;, DynamicLoading get dynamicLoading;. Which means we don't have two arguments to the constructor.
  • If at some point want to support dynamic loading and dynamic linking, the dynamic linking one should not support DynamicLoading.executable and DynamicLoading.process, but it should support something like bundle. Possibly system should also be supported. And it should probably have something such as built-in.

Cons:

  • We lose the ability to talk about "link mode is dynamic, but we haven't yet specified what the dynamic loading method is".
  • LinkModePreference can no longer list preferredLinkMode and potentialLinkModes. (Losing some convenience for developers who write builders that consume the link mode preference.)

Sketch:

abstract final class LinkMode {}

/// Dynamic loading.
///
/// Supported in the Dart and Flutter SDK.
///
/// Note: Dynamic loading is not equal to dynamic linking. Dynamic linking
/// would have to run the linker at compile-time, which is currently not
/// supported in the Dart and Flutter SDK.
abstract final class DynamicLoading implements LinkMode {}

/// The asset file should be bundled by Dart/Flutter.
///
/// An asset with this dynamic loading method must provide a [Asset.file]. The
/// Dart and Flutter SDK will bundle this code in the final application.
abstract final class DynamicLoadingBundledDylib implements DynamicLoading {
  factory DynamicLoadingBundledDylib();
}

/// Asset is avaliable on the target system `PATH`.
abstract final class DynamicLoadingSystemDylib implements DynamicLoading {
  Uri get uri;

  factory DynamicLoadingSystemDylib(Uri uri);
}

/// Asset is loaded in the process and symbols are available through
/// `DynamicLibrary.process()`.
abstract final class LookupInProcess implements DynamicLoading {
  factory LookupInProcess() = LookupInProcessImpl;
}

/// Asset is embedded in executable and symbols are available through
/// `DynamicLibrary.executable()`.
abstract final class LookupInExecutable implements DynamicLoading {
  factory LookupInExecutable() = LookupInExecutableImpl;
}

abstract final class DynamicLinking implements LinkMode {}

abstract final class DynamicLinkingBundledDylib implements DynamicLinking {
  factory DynamicLinkingBundledDylib();
}

/// Static linking.
///
/// Not yet supported in the Dart and Flutter SDK.
// TODO(https://github.com/dart-lang/sdk/issues/49418): Support static linking.
abstract final class StaticLinking implements LinkMode {
  static LinkMode static() => StaticLinking.static();
}

Uses change accordingly. From: (#946 incorporated)

      NativeCodeAsset(
        package: 'my_package',
        name: 'foo3',
        linkMode: LinkMode.dynamicLoading,
        dynamicLoading: SystemDylib(Uri(path: 'libfoo3.so')),
        os: OS.android,
        architecture: Architecture.x64,
      ),

to:

      NativeCodeAsset(
        package: 'my_package',
        name: 'foo3',
        linkMode: DynamicLoadingSystemDylib(Uri(path: 'libfoo3.so')),
        os: OS.android,
        architecture: Architecture.x64,
      ),

I think it improves the API, but we do get a rather large subtype hierarchy that developers need to know to find the right subtype to pass. E.g. linkMode: does not auto-complete to linkMode: LinkMode.xxx. Maybe this a moot point, as currently dynamicLoading: also doesn't auto complete to dynamicLoading: SystemDylib(....

The protocol will change in a similar fashion (rendered in yaml). From: (#946 incorporated)

link_mode: dynamic_loading
dynamic_loading:
  type: system
  uri: /asdf/asdf/asdf.so
link_mode:
  type: dynamic_loading_system
  uri: /asdf/asdf/asdf.so

@mosuem I've taken a stab at #946 (comment).

cc @mkustermann

@mkustermann
Copy link
Member

mkustermann commented Mar 12, 2024

I think it improves the API, but we do get a rather large subtype hierarchy that developers need to know to find the right subtype to pass. E.g. linkMode: does not auto-complete to linkMode: LinkMode.xxx. Maybe this a moot point, as

Nothing prevents one from using factory constructors that create subtypes, right?

Though one can also have this as methods on the output and not expose classes:

class BuildOutput  {
  Map<String, dynamic> get json;
}

extension NativeBuildOutputExt on BuildOutput {
  NativeConfig get native => NativeConfig._(this.json.putIfAbsent('native', () => {}));
}

extension type NativeBuildOutput._(Map<String, dynamic> _json) {
  void addSystemInstalledLibrary(...);
  void addStaticLibrary(...);
  void addDyanamicLibrary(...);
}

// User

main() async {
  await build((BuildConfig config, BuildOutput output) {
    if (config.supportsNative) {
       final nativeConfig = config.native;
       final nativeOutput = output.native;

       buildCodeWith(nativeConfig.toolchain);
       nativeOutput.addDynamicLibrary(...);
    } 
  });
}

=> One would get very nice code completion from output.native.add*()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants