Expose build mode in environment of asset transformer processes #144752

andrewkolos · 2024-03-07T05:59:35Z

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…. The rest of the change is updating call sites with a new argument.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

… process

andrewkolos · 2024-03-11T17:46:04Z

packages/flutter_tools/lib/src/build_system/tools/asset_transformer.dart

+        _dartBinaryPath = dartBinaryPath,
+        _buildMode = buildMode;
+
+  static const String buildModeEnvVar = 'FLUTTER_BUILD_MODE';


I have an internal instinct telling me that these should be namespaced, but FLUTTER_ASSET_TRANFORMER__BUILD_MODE feels a bit verbose. I think we are fine here.

andrewkolos · 2024-03-11T20:17:18Z

I do think a better solution would be to allow users to define separate asset configurations per build mode. For purely demonstrative purposes, consider this example fictional pubspec:

flutter:
  assets:
    - path: assets/hello.png
      buildModes: ["debug"]
    - path: assets/hello.png
      buildModes: ["profile", "release"]
      transformers:
        - package: optimizer
          args: --quality 95

However, this would be 1) harder to implement and maintain and 2) would call for a bunch of design discussion. I think this PR is still useful as a stop gap. I imagine, for example, an image optimization transformer could provide an option like "--release-only" which tells the transformer to just copy the file as-is when not running in release mode.

flutter:
  assets:
    - path: assets/hello.png
      transformers:
        - package: optimizer
          args: --release-only --quality 95

christopherfujino · 2024-03-11T20:28:40Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

  Future<void> build(Environment environment) async {
+    final String? buildModeEnvironment = environment.defines[kBuildMode];
+    if (buildModeEnvironment == null) {
+      throw MissingDefineException(kBuildMode, name);


i wonder if we'll ever hit this. merging this and then seeing if we need to roll back sounds good to me, though.

christopherfujino

LGTM

auto-submit · 2024-03-11T20:57:46Z

A reason for requesting a revert of flutter/flutter/144752 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

andrewkolos · 2024-03-11T20:58:56Z

Reason for revert: compilation issue has turned the tree red

…es (#144752)" This reverts commit 83fad74.

…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.

christopherfujino · 2024-03-11T21:01:52Z

Reason for revert: compilation issue has turned the tree red

For posterity can you link a failing a build? (This is something I always check when reviewing re-lands)

andrewkolos · 2024-03-11T21:04:13Z

Reason for revert: compilation issue has turned the tree red

For posterity can you link a failing a build? (This is something I always check when reviewing re-lands)

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8753729103951007409/+/u/flutter_config_--clear-features/stdout

Building flutter tool...
Resolving dependencies...
Got dependencies.
flutter/packages/flutter_tools/lib/src/bundle_builder.dart:177:61: Error: Required named parameter 'buildMode' must be provided.
  final AssetTransformer assetTransformer = AssetTransformer(
                                                            ^
flutter/packages/flutter_tools/lib/src/build_system/tools/asset_transformer.dart:22:3: Context: Found this candidate, but the arguments don't match.
  AssetTransformer({

Jasguerrero · 2024-03-11T22:16:56Z

@CaseyHillers is it possible to get the presubmit google testing run for this PR? I see the check passed but it failed postsubmit

andrewkolos · 2024-03-11T22:20:22Z

@CaseyHillers is it possible to get the presubmit google testing run for this PR? I see the check passed but it failed postsubmit

The PR branch was stale—it branched off master before #144734 landed. That PR introduced a new AssetTransformer call site, while this PR modified the signature of AssetTransformer. I'm assuming this is why the checks initially passed.

Jasguerrero · 2024-03-11T22:30:11Z

The PR branch was stale—it branched off master before #144734 landed. That PR introduced a new AssetTransformer call site, while this PR modified the signature of AssetTransformer. I'm assuming this is why the checks initially passed.

Interesting. Thanks for the explanation @andrewkolos!

…es (flutter/flutter#144752)

Manual roll requested by [email protected] flutter/flutter@3bb2e59...1ca8873 2024-03-12 [email protected] Update integration tests regexes. (flutter/flutter#144847) 2024-03-11 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fail tests on exceptions raised after test completed (#144706)" (flutter/flutter#144970) 2024-03-11 [email protected] Make TabController communicating creation in constructor. (flutter/flutter#144912) 2024-03-11 [email protected] Fail tests on exceptions raised after test completed (flutter/flutter#144706) 2024-03-11 [email protected] Refactoring `if` chains into `switch` statements (flutter/flutter#144905) 2024-03-11 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Expose build mode in environment of asset transformer processes (#144752)" (flutter/flutter#144957) 2024-03-11 [email protected] Expose build mode in environment of asset transformer processes (flutter/flutter#144752) 2024-03-11 [email protected] Roll Flutter Engine from 9196947bc687 to 6745955bb49e (2 revisions) (flutter/flutter#144946) 2024-03-11 [email protected] Skip test temporarily until headingLevel is added in engine (issue 41â�¦ (flutter/flutter#135077) 2024-03-11 [email protected] Roll Flutter Engine from 3b0b59bb224d to 9196947bc687 (1 revision) (flutter/flutter#144934) 2024-03-11 [email protected] Roll Packages from 0badb43 to d489d84 (3 revisions) (flutter/flutter#144931) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…es" (#144958) Relands #144752, which had to be reverted because the branch was stale. The original branch branched off `master` before #144734 landed. That PR introduced a new `AssetTransformer` call site. This PR branch is identical to the original but with a new commit that addresses the new call site, [update new call sites](6bb5296).

…es" (flutter#144958) Relands flutter#144752, which had to be reverted because the branch was stale. The original branch branched off `master` before flutter#144734 landed. That PR introduced a new `AssetTransformer` call site. This PR branch is identical to the original but with a new commit that addresses the new call site, [update new call sites](flutter@6bb5296).

…es (flutter/flutter#144752)

andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 7, 2024

andrewkolos force-pushed the add-build-mode-env-variable-to-asset-transformer-environment branch from f030d69 to aadd25a Compare March 10, 2024 21:09

andrewkolos marked this pull request as ready for review March 11, 2024 17:41

andrewkolos added 2 commits March 11, 2024 10:41

make BuildMode a required parameter for writing assets

8d69875

set environment variable to build mode when running asset transformer…

579912d

… process

andrewkolos force-pushed the add-build-mode-env-variable-to-asset-transformer-environment branch from f5f85eb to 579912d Compare March 11, 2024 17:42

andrewkolos requested a review from christopherfujino March 11, 2024 17:44

andrewkolos commented Mar 11, 2024

View reviewed changes

christopherfujino reviewed Mar 11, 2024

View reviewed changes

christopherfujino approved these changes Mar 11, 2024

View reviewed changes

andrewkolos mentioned this pull request Mar 11, 2024

☂️ Implement pluggable transformation (via packages) of assets at build time #143348

Closed

13 tasks

andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 11, 2024

auto-submit bot merged commit 83fad74 into flutter:master Mar 11, 2024

andrewkolos added the revert Autorevert PR (with "Reason for revert:" comment) label Mar 11, 2024

auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Mar 11, 2024

andrewkolos added the revert Autorevert PR (with "Reason for revert:" comment) label Mar 11, 2024

auto-submit bot pushed a commit that referenced this pull request Mar 11, 2024

Revert "Expose build mode in environment of asset transformer process…

469b9f1

…es (#144752)" This reverts commit 83fad74.

auto-submit bot mentioned this pull request Mar 11, 2024

Reverts "Expose build mode in environment of asset transformer processes (#144752)" #144957

Merged

9 tasks

auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Mar 11, 2024

andrewkolos mentioned this pull request Mar 11, 2024

Reland "Expose build mode in environment of asset transformer processes" #144958

Merged

9 tasks

engine-flutter-autoroll mentioned this pull request Mar 12, 2024

Manual roll Flutter from 3bb2e5948e7a to 1ca88730a0c1 (11 revisions) flutter/packages#6304

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 12, 2024

83fad74 Expose build mode in environment of asset transformer process…

5b26270

…es (flutter/flutter#144752)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024

83fad74 Expose build mode in environment of asset transformer process…

6f16310

…es (flutter/flutter#144752)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Expose build mode in environment of asset transformer processes #144752

Expose build mode in environment of asset transformer processes #144752

Uh oh!

andrewkolos commented Mar 7, 2024 •

edited

Loading

Uh oh!

andrewkolos Mar 11, 2024 •

edited

Loading

Uh oh!

andrewkolos commented Mar 11, 2024 •

edited

Loading

Uh oh!

christopherfujino Mar 11, 2024

Uh oh!

christopherfujino left a comment

Uh oh!

auto-submit bot commented Mar 11, 2024

Uh oh!

andrewkolos commented Mar 11, 2024

Uh oh!

christopherfujino commented Mar 11, 2024 •

edited

Loading

Uh oh!

andrewkolos commented Mar 11, 2024

Uh oh!

Jasguerrero commented Mar 11, 2024

Uh oh!

andrewkolos commented Mar 11, 2024

Uh oh!

Jasguerrero commented Mar 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Expose build mode in environment of asset transformer processes #144752

Expose build mode in environment of asset transformer processes #144752

Uh oh!

Conversation

andrewkolos commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

andrewkolos Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewkolos commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christopherfujino Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Mar 11, 2024

Uh oh!

andrewkolos commented Mar 11, 2024

Uh oh!

christopherfujino commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewkolos commented Mar 11, 2024

Uh oh!

Jasguerrero commented Mar 11, 2024

Uh oh!

andrewkolos commented Mar 11, 2024

Uh oh!

Jasguerrero commented Mar 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andrewkolos commented Mar 7, 2024 •

edited

Loading

andrewkolos Mar 11, 2024 •

edited

Loading

andrewkolos commented Mar 11, 2024 •

edited

Loading

christopherfujino commented Mar 11, 2024 •

edited

Loading