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
Comments
Some rough notes:
|
@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 |
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. |
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. |
oh totally, i wasn't suggesting making a build system. the icon transformer is asset->asset, right? |
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
|
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. |
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. |
Ah, fair. That distinction makes sense. |
@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 |
It'd be cool if this feature provides a pluggable API that you can extend and run your own code. |
@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) |
The current scope of this feature is limited to pure asset-to-asset transformation, where "asset" refers to a file defined under |
Hi @sivang, thanks for your interest in contributing; however, I'm already working on this feature. |
Thanks @andrewkolos , I'm still interested in understanding the code behind this mechanism in the flutter build system, where to start reading? :) |
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! |
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. |
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. |
@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). |
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 One alternative is to have asset tranformer definitions remain in their own YAML section ( 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 Footnotes
|
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? |
Thanks for the questions!
Yes. In particular, they get run at
To put it more simply, I am comfortable having users needing to trust any dependency they add to their project.
Good idea.
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 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:
flutter:
# ...
asset-transformers:
- package: vector_graphics_compiler # Suppose vector_graphics_compiler has been updated to support this feature.
assets:
- **.svg
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. |
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); }, ) ```
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. |
) 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.
…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.
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
The text was updated successfully, but these errors were encountered: