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

Provide a pluggable method for asset transformation #101077

Open
dnfield opened this issue Mar 30, 2022 · 25 comments
Open

Provide a pluggable method for asset transformation #101077

dnfield opened this issue Mar 30, 2022 · 25 comments
Assignees
Labels
a: assets Packaging, accessing, or using assets c: new feature Nothing broken; request for a new capability P2 Important issues not at the top of the work list team-tool Owned by Flutter Tool team tool Affects the "flutter" command-line tool. See also t: labels. triaged-tool Triaged by Flutter Tool team

Comments

@dnfield
Copy link
Contributor

dnfield commented Mar 30, 2022

The tool already does transformation of icon fonts at build time.

It would be good to make an interface like that open and pluggable, so that packages external to the tool could decide to transform assets. For example, package:vector_graphics will want to transform SVG files into a different kind of file at build time by invoking a Dart program to process them.

@christopherfujino @jonahwilliams for input

@dnfield dnfield added c: new feature Nothing broken; request for a new capability tool Affects the "flutter" command-line tool. See also t: labels. a: assets Packaging, accessing, or using assets P3 Issues that we currently consider unimportant labels Mar 30, 2022
@dnfield
Copy link
Contributor Author

dnfield commented Mar 31, 2022

Some rough notes:

  • The icon font shaker currently operates by getting fed all the files going into the asset bundle and sniffing the mime type. It then transforms the file and outputs a file with the same name and extension.
  • Some transformers might want to replace existing files with brand new ones that have a different file name, but I feel like there's something I haven't thought all the way through here. Maybe keeping the file name the same is a good thing.
  • We'll probably want ways to allow the manifest to specify which files get transformed and by which transformers, perhaps with regexps or globs for files?

@dnfield
Copy link
Contributor Author

dnfield commented Mar 31, 2022

@jonahwilliams says he might be able to write a onepager on this. We talked some more this morning and agreed we don't need to make this work for the icon shaking, but it might help support some other features the tool has had requests for like platform specific assets.

@christopherfujino
Copy link
Member

@jonahwilliams says he might be able to write a onepager on this. We talked some more this morning and agreed we don't need to make this work for the icon shaking, but it might help support some other features the tool has had requests for like platform specific assets.

SGTM

@andrewkolos andrewkolos self-assigned this Oct 20, 2022
@Hixie
Copy link
Contributor

Hixie commented Jan 19, 2023

It would be great if we could make both the icon font transformer and the license file munger components that just plug into this more generic framework.

@Hixie Hixie added P2 Important issues not at the top of the work list and removed P3 Issues that we currently consider unimportant labels Jan 19, 2023
@jonahwilliams
Copy link
Member

one of these ways I tried to make this tractable was to massively simplify the scope of what we could run over. By just allowing asset -> asset transformations, we allow for a very simple API with almost no configuration required.

We must avoid making a generic build system, otherwise we'd probably be better taking something off the shelf. These usually come with severe tradeoffs and don't work in the same out of the box way that Flutter does today.

@Hixie
Copy link
Contributor

Hixie commented Jan 20, 2023

oh totally, i wasn't suggesting making a build system. the icon transformer is asset->asset, right?
i agree that the license is more assets->assets, which is less of a good fit if the goal is simplicity.

@jonahwilliams
Copy link
Member

its asset + compiled dart kernel -> asset, which relies on const finder and some tricky assumptions about how and when tree shaking applies to Dart code.

Potentially a better fit would be something like the shader features which use impellerc to do shader compilation. The complications there have to do with

  • communicating the right set of configuration to the tooing (i.e. which backend to target, which features to add) which will change over time
  • managing the (likely) breaking changes we'll need to make internal to impellerc in order to add support for more backends and to fix bugs once the feature is used more.

@Hixie
Copy link
Contributor

Hixie commented Jan 20, 2023

Ah if you mean asset -> asset with literally no other inputs, not even inputs that would be the same for all assets, then yeah, the icon transformation doesn't fit either. A clean asset -> asset transformation with no dependencies at all is a very interesting subset of the problem space, because it can be parallelized very efficiently, so that makes sense to me.

Shader compilation seems like it has the same kind of needs (i.e. additional build-wide information) as the icon transformation.

@jonahwilliams
Copy link
Member

Yeah, it has global configuration requirements, but it doesn't depend on the outputs of other build steps. So definitely a step in complexity from the asset -> asset transformations but not as complex as icons.

@Hixie
Copy link
Contributor

Hixie commented Jan 20, 2023

Ah, fair. That distinction makes sense.

@andrewkolos
Copy link
Contributor

but it might help support some other features the tool has had requests for like platform specific assets.

@dnfield Do you recall where to find such requests? I'm interested in known potential use cases for asset transformation outside of easier use of package:vector_graphics.

@marandaneto
Copy link

It'd be cool if this feature provides a pluggable API that you can extend and run your own code.
An example that comes to mind is associating the generated obfuscation-map (via save-obfuscation-map) file when sending eg error reports such that those reports can symbolicate obfuscated class names, types, etc, because the final app knows its obfuscation map id that was generated and uploaded during build time, but the event should be sent at runtime so it should know its id.
We already do this for Android and Proguard, but that requires hooking up into Gradle directly, or, for each platform, ideally there'd be hooks in the Flutter build process that you can do such things.

@sivang
Copy link

sivang commented Mar 28, 2023

@christopherfujino @jonahwilliams @dnfield If this work is still up for grabs, I'm interested investigating if I can take on this "Good Starter Project" but would require some guidance as to what&where to read code pertaining to the enhancement discussed here. In particular, I'm keen on devising up a generic use case that would ideally be platform-independent. (@marandaneto use case is one such case I'd think could be catered for)

@andrewkolos
Copy link
Contributor

It'd be cool if this feature provides a pluggable API that you can extend and run your own code...

The current scope of this feature is limited to pure asset-to-asset transformation, where "asset" refers to a file defined under assets in a project's pubspec.yaml (see Adding assets and images).

@andrewkolos
Copy link
Contributor

If this work is still up for grabs, I'm interested investigating if I can take on this "Good Starter Project" but would require some guidance as to what&where to read code pertaining to the enhancement discussed here.

Hi @sivang, thanks for your interest in contributing; however, I'm already working on this feature.

@sivang
Copy link

sivang commented Mar 30, 2023

Thanks @andrewkolos , I'm still interested in understanding the code behind this mechanism in the flutter build system, where to start reading? :)

@flutter-triage-bot flutter-triage-bot bot added team-tool Owned by Flutter Tool team triaged-tool Triaged by Flutter Tool team labels Jul 8, 2023
@flutter-triage-bot flutter-triage-bot bot added the Bot is counting down the days until it unassigns the issue label Aug 16, 2023
@flutter-triage-bot
Copy link

This issue is assigned to @andrewkolos but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!

@flutter-triage-bot flutter-triage-bot bot removed the triaged-tool Triaged by Flutter Tool team label Oct 25, 2023
@flutter-triage-bot
Copy link

This issue was assigned to @andrewkolos but has had no status updates in a long time. To remove any ambiguity about whether the issue is being worked on, the assignee was removed.

@flutter-triage-bot flutter-triage-bot bot removed the Bot is counting down the days until it unassigns the issue label Oct 25, 2023
@andrewkolos
Copy link
Contributor

FYI I still plan on working on this, but I have delayed it until after #132985 since that PR and the one that would close this would both heavily modify pubspec parsing code. Trying to merge both concurrently would result in a lot of duplicated effort and/or merge conflicts.

@christopherfujino christopherfujino added the triaged-tool Triaged by Flutter Tool team label Oct 31, 2023
@NashIlli
Copy link

@andrewkolos I just noticed that #132985 is already merged. Do you plan work on this in 2024? Thanks a lot! :)

@andrewkolos
Copy link
Contributor

andrewkolos commented Jan 2, 2024

@andrewkolos I just noticed that #132985 is already merged. Do you plan work on this in 2024? Thanks a lot! :)

Yes, this is what I plan on working on next (other than routine maintenance work).

@andrewkolos andrewkolos self-assigned this Jan 8, 2024
@andrewkolos
Copy link
Contributor

andrewkolos commented Jan 8, 2024

For anyone watching this issue or interested in how users will utilize this feature,

I'm currently drafting a PR for this. To help inform the design, I'm interested in any potential use case you might have for it.

Currently, I intend to provide a separate YAML section in the pubspec, where the user lists their transformers and what assets they will be run against:

flutter:
  # ...
  assets:
    - assets/splash/logo.svg
    - assets/chatbot/
    - path: assets/integrations/free/
      flavor: free
    - path: assets/integrations/premium/
      flavor: premium
    # ...
  # ...
  asset-transformers:
    - package: vector_graphics_compiler
      args:
        - "--tesselate"
        - "--use-half-precision-control-points"
      assets:
        - "**/*.svg"
      # Globs are not normally supported for asset lists for technical reasons.
      # However, we can support them here since this doesn't *add* any
      # files—the glob is only run against the existing assets list.

Normally, I'd be hesitant to have something outside of the assets affect assets1, but I'm assuming that the vast majority of use cases of this feature are going to involve running a transformer against all assets of a specific file type (like in the example scenario above, where I want to run a transformer against all bundled SVG files, regardless of where they are being used in the app).

One alternative is to have asset tranformer definitions remain in their own YAML section (asset-transformers), but to not have them define an assets list. Instead, they would be given a name, and assets in the main asset list would provide any relevant transformations by name:

flutter:
  # ...
  assets:
    - path: assets/splash/logo.svg
      transformers:
        - compileSvg # Refer to later transformer definition by name.
    - assets/chatbot/
    - path: assets/integrations/free/
      flavor: free
      # new:
    - path: assets/integrations/free/logos/
      flavor: free
      transformers:
        - compileSvg
    - path: assets/integrations/premium/
      flavor: premium
      # new:
    - path: assets/integrations/premium/logos/
      flavor: free
      transformers:
        - compileSvg
    # ...
  # ...
  asset-transformers:
    # This transformer definition no longer provides its own `assets` list, but it has a
    # name that can be referred to in the main `assets` section.
    - name: compileSvg
      package: vector_graphics_compiler
      args:
        - "--tesselate"
        - "--use-half-precision-control-points"

This keeps the assets section more complete in regards to containing all information related to assets, but it also makes the task of running a transformer against all files with a given type tedious.

Footnotes

  1. This makes pubspecs slightly harder to read for folks onboarding to a project and troubleshooting asset-related issues. They need to notice that some separate YAML section to have any clue that this feature is being used while all other asset-related info is kept together in assets.

@ueman
Copy link
Contributor

ueman commented Jan 13, 2024

Do packages with entries in that section automatically executed? I imagine that being useful, although it's a bit scare with regard to security, since it basically allows arbitrary code execution. But it would be certainly nice from a DX experience. Just add a transformer as a dev dependency and it just works™️

Do transformers know whether they get executed in debug or release mode? I kinda imagine that being useful. Ex: There's a transformer that optimizes the file size of images. Depending on the amount and size of the images, that could take a significant amount of time. Therefore, I might want to only execute it in release builds.

Another use case is the automatic creation of resolution-aware images. Imagine just the high resolution image (3x) gets added. Can a transformer create the 2x and 1x version of the image?

Another question is do transformer also transform assets included by other packages?

@andrewkolos
Copy link
Contributor

andrewkolos commented Jan 17, 2024

Thanks for the questions!

Do packages with entries in that section automatically executed? I imagine that being useful, although it's a bit scare with regard to security, since it basically allows arbitrary code execution. But it would be certainly nice from a DX experience. Just add a transformer as a dev dependency and it just works™️

Yes. In particular, they get run at flutter run/flutter build, and only if the user has configured their project to use them. Indeed, this could theoretically be exploited, but also consider the following:

  1. Dart packages already have these privileges if imported in user code (or explicitly run through dart run <package-name>).
  2. The upcoming Native Assets feature will (likely) allow code execution even from packages even more subtly (in a way similar to many existing ecosystems today do—Rust's Cargo and Node Package Manager (NPM) scripts come to mind).

To put it more simply, I am comfortable having users needing to trust any dependency they add to their project.

Do transformers know whether they get executed in debug or release mode? I kinda imagine that being useful. Ex: There's a transformer that optimizes the file size of images. Depending on the amount and size of the images, that could take a significant amount of time. Therefore, I might want to only execute it in release builds.

Good idea. flutter could make this available to the transformer either through an argument or through a new well-known environment variable (assuming it doesn't already exist).

Another use case is the automatic creation of resolution-aware images. Imagine just the high resolution image (3x) gets added. Can a transformer create the 2x and 1x version of the image?

Right now, the feature is designed with one-to-one asset transformation in mind. Technically, there is nothing stopping a transformer from doing such a thing since it has access to the filesystem (though there are caveats here). Still, this use case would not explicitly supported.

If anyone has design ideas for how one-to-many (or many-to-one) asset transformation might work from the user's perspective, I would be interested—not necessarily because I would want to implement this in the initial feature, but because I'd like to make the sure the current design would not be incompatible with such an idea.

(cc: @bdero, who may be interested in this)

Another question is do transformer also transform assets included by other packages?

Another good question. This is something I failed to consider carefully. As things are, transformers would apply to any asset that matches one of its globs. As a result, it's possible that users may unintentionally transform assets from package dependencies. Let me illustrate this with a hypothetical scenario:

  1. A flutter project has some SVG assets. To make these usable by the vector_graphics package, the user sets up a transformer in their pubspec like so:
flutter:
  # ...
  asset-transformers:
    - package: vector_graphics_compiler # Suppose vector_graphics_compiler has been updated to support this feature.
      assets:
        - **.svg
  1. The same project uses some widget that comes from a package dependency. We'll call this package my_dependency and the widget PackageWidget. PacketWidget depends on an SVG within the same package using this code: SvgPicture.asset('packages/my_dependency/assets/file.svg'). This uses the flutter_svgpackage to render its SVGs, not vector_graphics.
  2. The user runs flutter run. The flutter tool transforms packages/my_dependency/assets/file.svg because it matches the **.svg glob. SvgPicture.asset will fail, presumably with a vague error message about packages/my_dependency/assets/file.svg being formatted incorrectly. This is both surprising and confusing to the end user, who was unaware that this file even existed, and they might not be able to figure out why this is happening.

We could certainly add some sort of guard against a user accidentally doing something like this, but it is unclear if it is worth the effort.

auto-submit bot pushed a commit that referenced this issue Jan 25, 2024
Part of work on [#101077](#141194). This is done as a separate PR to avoid a massive diff.

## Context
1. The `FakeCommand` class accepts a list of patterns that's used to match a command given to its `FakeProcessManager`. Since `FakeCommand` can match a list of patterns, not just specifically strings, it can be used to match commands where the exact value of some arguments can't (easily) known ahead of time. For example, a part of the tool may invoke a command with an argument that is the path of a temporarily file that has a randomly-generated basename.
2. The `FakeCommand` class provides on `onRun` parameter, which is a callback that is run when the `FakeProcessManager` runs a command that matches the `FakeCommand` in question.

## Issue
In the event that a `FakeCommand` is constructed using patterns, the test code can't know the exact values used for arguments in the command. This PR proposes changing the type of `onRun` from `VoidCallback?` to `void Function(List<String>)?`. When run, the value `List<String>` parameter will be the full command that the `FakeCommand` matched.

Example:
```dart
FakeCommand(
  command: <Pattern>[
    artifacts.getArtifactPath(Artifact.engineDartBinary),
    'run',
    'vector_graphics_compiler',
    RegExp(r'--input=/.*\.temp'),
    RegExp(r'--output=/.*\.temp'),
  ],
  onRun: (List<String> command) {
    final outputPath = (() { 
      // code to parse `--output` from `command`
    })();
    testFileSystem.file(outputPath).createSync(recursive: true);
  },
)
```
@andrewkolos
Copy link
Contributor

I'm planning to land support for this in pieces, see #143348 (perhaps I should have hijacked this issue instead 🤷). The only other thing to note at the moment is that I've tweaked the design slightly—asset transformations are now declared together with assets, rather than having a separate section in the pubspec.

auto-submit bot pushed a commit that referenced this issue Mar 11, 2024
)

In service of #143348

When invoking a package to transform an asset, we set `FLUTTER_BUILD_MODE` to the CLI name of the build mode being used. Inspired by #101077 (comment):

> Do transformers know whether they get executed in debug or release mode? I kinda imagine that being useful. Ex: There's a transformer that optimizes the file size of images. Depending on the amount and size of the images, that could take a significant amount of time. Therefore, I might want to only execute it in release builds.

Note for the reviewer: the interesting part of this change can be found in the commit [set environment variable to build mode when running asset transformer�](579912d). The rest of the change is updating call sites with a new argument.
auto-submit bot added a commit that referenced this issue Mar 11, 2024
…ses (#144752)" (#144957)

Reverts: #144752
Initiated by: andrewkolos
Reason for reverting: compilation issue has turned the tree red
Original PR Author: andrewkolos

Reviewed By: {christopherfujino}

This change reverts the following previous change:
In service of #143348

When invoking a package to transform an asset, we set `FLUTTER_BUILD_MODE` to the CLI name of the build mode being used. Inspired by #101077 (comment):

> Do transformers know whether they get executed in debug or release mode? I kinda imagine that being useful. Ex: There's a transformer that optimizes the file size of images. Depending on the amount and size of the images, that could take a significant amount of time. Therefore, I might want to only execute it in release builds.

Note for the reviewer: the interesting part of this change can be found in the commit [set environment variable to build mode when running asset transformer…](579912d). The rest of the change is updating call sites with a new argument.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: assets Packaging, accessing, or using assets c: new feature Nothing broken; request for a new capability P2 Important issues not at the top of the work list team-tool Owned by Flutter Tool team tool Affects the "flutter" command-line tool. See also t: labels. triaged-tool Triaged by Flutter Tool team
Projects
Status: Future tool team projects
Development

Successfully merging a pull request may close this issue.

9 participants