[native_assets_cli] Add workspace path to input

[simolus3]

I'm adopting user-defines for my native_assets package (thanks for shipping that btw! 🚀 ). One of the options I'd like to provide is for users to specify a custom C source file that would be used instead of (or in addition to) the default sources used by my package.

I'm imagining that users would configure this like that:

hooks:
  user_defines:
    sqlite3_native_assets:
      source:
        local: my_sqlite.c # file in the package applying mine

My build hooks are then reading that option and can use that as an input. However, since all paths resolve against my own package (#1915), I need a way to figure out the root input package in some cases. At the moment, I'm reconstructing it from the output path but I'd like something more reliable than this:

  @override
  String inputPath(String path) => absolute(
    normalize(join(input.outputDirectory.path, '../../../../../../', path)),
  );

I couldn't find anything in HookInputs that provides that root input path.

[dcharkes]

Where is the source on your system? In another package?

So far we've been avoiding exposing the root package, because it would mean assets cannot be cached in a pub workspace across multiple packages.

Coming to think of it, maybe we should only allow user-defines in the pub workspace pubspec instead of the root package pubspec? We could conceivably expose a path to the workspace in the input.

cc @mosuem thoughts?

[mosuem]

I have a similar issue, and was just solving it by requiring absolute paths. But exposing the path to the pubspec/root folder sounds good to me.

[dcharkes]

I think absolute paths are fine if the library is somewhere on your system. (Note you should not commit such path most likely. So we'll likely want some non-commitable place for user-defines as well.)

However, if the file is somewhere in your workspace, then it's really annoying that you wouldn't be able to commit a relative path in the pubspec so that it would work for everyone checking out your git repo.

@mosuem @simolus3 do any of your use cases have the file in the workspace?

[blaugold]

I'm wondering how user-defines at the pub workspace level would work if I want to depend on the same package (that defines hooks) in multiple packages of workspace but with different user-defines. For example, there are two flutter apps in the workspace, and they want to pass different values to sqlite3_native_assets.source.local because they need different subset of SQLite features.

As an aside: To me, user_defines reads a bit native code compilation specific. 🤓 But it's a way to configure the hooks of dependencies, which don't necessarily deal with compilation. If I were new to hooks and didn't know what user-defines are, I don't think I would have an intuitive notion from just the name. Maybe config or options would be more widely understood and self-explanatory?

[dcharkes]

For example, there are two flutter apps in the workspace, and they want to pass different values to sqlite3_native_assets.source.local because they need different subset of SQLite features.

We have a trade-off to make here. Assuming you do have multiple apps that use the same sqlite subset, you'd want your native assets to be shared. (Similarly to how all the Dart dependencies are shared.) If you want to "override" a Dart dependency for one app, you cannot have it in the same workspace. So my suggestion would be to not have those apps in the same workspace in that case.

As an aside: To me, user_defines reads a bit native code compilation specific. 🤓 But it's a way to configure the hooks of dependencies, which don't necessarily deal with compilation. If I were new to hooks and didn't know what user-defines are, I don't think I would have an intuitive notion from just the name. Maybe config or options would be more widely understood and self-explanatory?

I'm thinking that we will also allow command-line arguments at some point from Dart and Flutter, which would mirror https://dartcode.org/docs/using-dart-define-in-flutter/ . (And this will break the caching.)

But I do agree it's kind of "code-centric" either C preprocessor or Dart.

Config is already an overloaded term. Buildinput.config is already config.

I do like the "user", because it's signalling that these values are coming from the "end-user" e.g. the author of the root package (and workspace).

If you have good suggestions, let me know, otherwise I think user-defines is a decent name for this concept.

[blaugold]

We have a trade-off to make here. Assuming you do have multiple apps that use the same sqlite subset, you'd want your native assets to be shared. (Similarly to how all the Dart dependencies are shared.)

Do I want them to be shared because that reduces build times?

If you want to "override" a Dart dependency for one app, you cannot have it in the same workspace.

I think dependency versioning and configuration can be seen as different planes. In a pub workspace, every package uses the same version of a package but can configure and use it differently. Configuration is much more use case dependent than versioning. Potentially even dynamic (Dart defines can be provided at build time). Is the design for user defines to always be static?

Something that feels related that I have been thinking about: Could Dart APIs be annotated with RecordUse, and its recorded usages be used to compile native libraries with only the features that app code uses?

So my suggestion would be to not have those apps in the same workspace in that case.

I definitely think people will run into this! 😅 They will use pub workspaces for monorepos that contain reusable components and multiple apps that use them. You can have multiple pub workspaces in the same monorepo, but that somewhat defeats the purpose of using pub workspaces.

---

I do like the "user", because it's signalling that these values are coming from the "end-user" e.g. the author of the root package (and workspace).

I think that's true from the perspective of a hook author. But for someone writing or reading user-defines in pubspec.yaml (the "user") it's implied and maybe even a bit confusing. The user qualifier can suggest that there are other sources for defines.

Config is already an overloaded term. BuildInput.config is already config.

To me, it would actually feel natural to have BuildInput.config for the native assets system provided config and next to it BuildInput.userConfig for the user provided config.

While it's usually nice to have symmetry in naming, I don't think the naming of the concept needs to be mirrored 1-to-1 in the pubspec.yaml. Then we could have:

hooks:
  config:
    sqlite3_native_assets:
      source:
        local: my_sqlite.c # file in the package applying mine

I don't think this is a huge DX issue, though, just my 2 cents. 😊

[dcharkes]

Do I want them to be shared because that reduces build times?

build-times and disk space

Is the design for user defines to always be static?

It is not. If you pass command-line arguments for user-defines it's expected that your cache is invalidated.
However, having it in the pubspec of the root package feels more static so it feels weird to invalidate the cache for that.

Maybe we add support for root package pubspec later (causing the cache to no longer be shared in workspaces).

I think for command-line argument user-defines it always invalidates caches.

For example, there are two flutter apps in the workspace, and they want to pass different values to sqlite3_native_assets.source.local because they need different subset of SQLite features.

Can the subset of features be seen in the Dart API? If yes, I think it would be better to try to model this with @RecordUse and link hooks. That way you can write your package so that it has all features in JIT mode. And in AOT mode multiple packages could use a different subsets of features. And then any code reachable from the different root packages (apps) can be analyzed and your package/sqlite3s hook/link.dart can emit a dylib with the required features. (The build hook will be a no-op if linking is enabled, and the link hook should then run the CBuilder for sqlite with the right feature flags.) @mosuem You have feature flags in the rust build as well right? But IIRC you actually strip symbols with the linker instead of building in the link hook.

What you are suggesting is that root-package authors need to configure the feature set. But this does not allow packages depending on you but depended on by the root package to declare that they require a feature.

If it's not currently visible in the API, we should consider having a class Sqlite const constructor with @mustBeConst parameters for every feature that can be toggled on and off.

wdyt?

[simolus3]

do any of your use cases have the file in the workspace?

So for context, while my package is downloading sources from sqlite3.org to compile them, there are ABI-compatible forks of sqlite3 that I know some users would like to use instead. I eventually want to move the native build hooks into package:sqlite3, and it would be a shame if users had to fork my package just to use a SQLite fork that would be entirely compatible with the upstream bindings.

So for this reason, I want to give users maximum flexibility to provide custom SQLite sources. And one of the options I had in mind was that users would vendor SQLite sources into their repository and then use user-defines towards package:sqlite3 to make it compile and link those instead of the copy downloaded from sqlite3.org. So yes, that would be a use case where the file is in the workspace.

However, I see that this sounds like a fairly unique use case, and I understand that this could make hermetic builds much harder. FWIW, I also allow users to completely skip my build hooks through user defines (with the intention that they would write their own build.dart adding a code asset for my package). That gives them complete flexibility to write their own builds, so I don't really need this option.
As an aside, it would be nice to have some kind of "build hooks overrides" feature for when you still want the unchanged Dart code of your dependencies, but need a completely custom build. But that's probably a different issue, and I'm happy to consider this one a wontfix if it's hard to integrate with caching.

[dcharkes]

there are ABI-compatible forks of sqlite3 that I know some users would like to use instead

That's an interesting use case. Reusing Dart code / bindings while dropping in another binary.

That feels very much like what you'd do in the linking step in native code. You'd point to a different libsqlite3.so to be linked.

We could use link hooks in the following way:

• Allow arbitrary packages to send a compiled dylib to the package:sqlite3 link hook.
• Also send a dylib from the build hook to the link hook in sqlite3 itself.
• If there is exactly 1 dylib send to the link hook, use that one (that's your own).
• If there are exactly 2 dylibs send to the link hook, use the other one (override from a user).
• If there are more than 2 dylibs send to the link hook, give an error.

This would enable someone to author a package that overrides the dylib and other packages to reuse such override. (Though maybe it's mostly the root package / app author that wants to override. So then the app / root package would have this in their build hook, and not publish a package.)

If someone tries to use two packages that override at the same time, it wouldn't work.

The build hook of such package would probably call a helper function from package:sqlite3 lib/hook.dart to build a dylib to send to the sqlite3 link hook to bundle.

This would enable such override-dylib-packages to use all the infrastructure of using the hooks as normal. That seems like a clearer separation of concerns. If the package overrides the C sources, it should also build the dylib in its hook.

That doesn't provide a solution for linking-not-enabled though.

(Just brainstorming here. Maybe there's better ways to solve this.)

[dcharkes]

RE: adding workspace directory

• HookInput.workspaceDirectory
  • It does not have to be HookInput.config.workspaceDirectory, as this value does not have to influence the checksum.
• If we do introduce a HookInput.systemDirectoryShared, users should not use any workspace specific config. Not this path that we introduce, no user-defines, etc. This is on the hook writer.

Edit: To support relative paths working properly, we want to distinguish where user-defines are coming from, cli arguments or workspace pubspec:

  "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/"
    }
  },

And then we can have a Dart interface that abstracts over the sources.

class UserDefines {
  Object? operator [] (String key) => // try command-line-args, if not populated try pubspec
  Uri? path(List<String> keys, {bool absolute: true}) => // resolve path against the base path, try commandline-args first, then pubspec.
}

(This intentionally hides from hook authors what the source of a user-define is.)

This also scopes the use of the workspace directory to be only used for user-defines, and makes it not available in the API.

@mosuem @simolus3 WDYT?

[simolus3]

For my use-cases that approach looks good to me 👍

[mosuem]

SGTM - but why a list of keys as argument for the path method?

[dcharkes]

SGTM - but why a list of keys as argument for the path method?

If you put a path not at the top level:

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

[goderbauer]

The List<String> for the keys argument of path was confusing me as well. I wonder how common the case of nested keys is going to be? Maybe the default path method should just accept a single String key (assuming that's going to be the common case) and then there could be another nestedPath method that covers the - presumably less common - nested use case?

[dcharkes]

Note: This needs to roll into Dart and Flutter first before it can be used.