[flutter_tools] Cleanup of native asset related code (removes around 50% of the native asset related code) #155430

mkustermann · 2024-09-19T22:49:49Z

tl;dr Removes 50% (>1650 locs) of native asset related code in packages/flutter_tools

Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but sligthly different code. It also led to similarly duplicated test code.

Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations:

configuration of the build (e.g. android/macos/ios version, ios sdk, ...)
determining target locations & copying the final shared libraries

This PR unifies the implementation by reducing the code to basically two main functions:

runFlutterSpecificDartBuild which is responsible for
- obtain flutter configuration
- perform dart build (& link)
- determine target location & install binaries
runFlutterSpecificDartDryRunOnPlatforms which is responsible for a similar (but not same):
- obtain flutter configuration
- perform dart dry run
- determine target location

these two functions will call out to helpers for the OS specific functionality:

_assetTargetLocationsForOS for determining the location of the code assets
_copyNativeCodeAssetsForOS for copying the code assets (and possibly overriting the install name, etc)

=> Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes.

We also harden the building code by adding asserts, e.g.

the dry fun functionality should never be used by flutter test
the build/native_assets/<os>/native_assets.yaml should only be used by flutter test and the dry-run of flutter run

=> We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality)

We also rename {,Flutter}NativeAssetsBuildRunner to disambiguate it from the package:native_asset_builder's NativeAssetsBuildRunner.

We also reorganize the main code to make it readable from top-down and make members private where they can be.

mkustermann · 2024-09-20T11:15:30Z

/cc @mosuem That's preparation for adding support for data assets (which would be much harder without cleaning up this code first)

dcharkes

LGTM!

Thanks @mkustermann! 🚀

I'll refer to the Flutter team on unrelated code having been formatted by the formatter.

packages/flutter_tools/lib/src/isolated/native_assets/native_assets.dart

packages/flutter_tools/lib/src/isolated/native_assets/macos/native_assets_host.dart

packages/flutter_tools/test/integration.shard/isolated/native_assets_test.dart

dcharkes · 2024-09-23T19:33:29Z

Probably @christopherfujino or @andrewkolos should review this. (Or @bkonyi.)

mkustermann · 2024-09-23T19:52:45Z

Thanks, @dcharkes !

I'll refer to the Flutter team on unrelated code having been formatted by the formatter.

Apologies for that. This was unintentional: Sometimes I forgot to turn autoformat-on-save off in my editor ...

I could try to manually undo some of those if required (or try to re-format with the new dart formatter in long mode) - but I'm not sure if it's worthwhile (and recent summit discussions say we shouldn't spend time hand-formatting code :D)

mkustermann · 2024-09-24T10:39:49Z

@dcharkes I've rebased, tried to align old/new code to make diff smaller, tried to un-auto-format code. Please let me know if there's something else.

@bkonyi This is mainly a refactoring of native asset code (for which I'd consider @dcharkes the most knowledgeable person / owner) - do you also want to review this?

dcharkes

lgtm from my side!

packages/flutter_tools/test/integration.shard/isolated/native_assets_test.dart

bkonyi

LGTM with some nits.

packages/flutter_tools/lib/src/isolated/native_assets/macos/native_assets_host.dart

packages/flutter_tools/test/general.shard/isolated/android/native_assets_test.dart

packages/flutter_tools/test/general.shard/isolated/ios/native_assets_test.dart

packages/flutter_tools/lib/src/isolated/native_assets/native_assets.dart

mkustermann · 2024-09-25T19:28:14Z

Thanks, @bkonyi !

tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but sligthly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunBuild` these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

eyebrowsoffire · 2024-09-25T21:44:46Z

Reason for revert: Postsubmit failures closing the tree. See the following examples:

https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20native_assets_ios/5738/overview
https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_arm64_mokey%20native_assets_android/583/overview
https://ci.chromium.org/ui/p/flutter/builders/prod/Linux_pixel_7pro%20native_assets_android/4075/overview
https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20native_assets_ios_simulator/5287/overview

… around 50% of the native asset related code) (#155430)" This reverts commit 621e7ef.

…s around 50% of the native asset related code) (#155430)" (#155713) Reverts: #155430 Initiated by: eyebrowsoffire Reason for reverting: Postsubmit failures closing the tree. See the following examples: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20native_assets_ios/5738/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_arm64_mokey%20native_assets_android/583/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Linux_pixel_7pro%20native_assets_android/4075/overview https://ci.chromium.org/u Original PR Author: mkustermann Reviewed By: {bkonyi, dcharkes} This change reverts the following previous change: tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but sligthly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`. We also reorganize the main code to make it readable from top-down and make members private where they can be.

… around 50% of the native asset related code) (flutter#155430)" Changes to original CL: The code that issues an error on unsupported operating system in the dry-run case was missing a case for iOS and Android. Original CL description tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but slightly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

mkustermann · 2024-09-26T08:45:14Z

Reason for revert: Postsubmit failures closing the tree. See the following examples:

Can reproduce the failure of the test, it was missing a case for iOS/Android, fix is

diff --git a/packages/flutter_tools/lib/src/isolated/native_assets/native_assets.dart b/packages/flutter_tools/lib/src/isolated/native_assets/native_assets.dart
index b28543ba16..fd8952e7d9 100644
--- a/packages/flutter_tools/lib/src/isolated/native_assets/native_assets.dart
+++ b/packages/flutter_tools/lib/src/isolated/native_assets/native_assets.dart
@@ -145,7 +145,9 @@ Future<Uri?> runFlutterSpecificDartDryRunOnPlatforms({
     final OSImpl targetOS = _getNativeOSFromTargetPlatfrorm(targetPlatform);
     if (targetOS != OS.macOS &&
         targetOS != OS.windows &&
-        targetOS != OS.linux) {
+        targetOS != OS.linux &&
+        targetOS != OS.android &&
+        targetOS != OS.iOS) {
       await ensureNoNativeAssetsOrOsIsSupported(
         projectUri,

Made a reland CL in #155745

… around 50% of the native asset related code) (#155430)" (#155745) Changes to original CL: The code that issues an error on unsupported operating system in the dry-run case was missing a case for iOS and Android Original CL description tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but slightly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

… around 50% of the native asset related code) (flutter/flutter#155430)

…50% of the native asset related code) (flutter#155430) tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but slightly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

…s around 50% of the native asset related code) (flutter#155430)" (flutter#155713) Reverts: flutter#155430 Initiated by: eyebrowsoffire Reason for reverting: Postsubmit failures closing the tree. See the following examples: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20native_assets_ios/5738/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_arm64_mokey%20native_assets_android/583/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Linux_pixel_7pro%20native_assets_android/4075/overview https://ci.chromium.org/u Original PR Author: mkustermann Reviewed By: {bkonyi, dcharkes} This change reverts the following previous change: tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but sligthly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`. We also reorganize the main code to make it readable from top-down and make members private where they can be.

… around 50% of the native asset related code) (flutter#155430)" (flutter#155745) Changes to original CL: The code that issues an error on unsupported operating system in the dry-run case was missing a case for iOS and Android Original CL description tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but slightly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

… around 50% of the native asset related code) (flutter/flutter#155430)

…50% of the native asset related code) (flutter#155430) tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but slightly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

…s around 50% of the native asset related code) (flutter#155430)" (flutter#155713) Reverts: flutter#155430 Initiated by: eyebrowsoffire Reason for reverting: Postsubmit failures closing the tree. See the following examples: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20native_assets_ios/5738/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_arm64_mokey%20native_assets_android/583/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Linux_pixel_7pro%20native_assets_android/4075/overview https://ci.chromium.org/u Original PR Author: mkustermann Reviewed By: {bkonyi, dcharkes} This change reverts the following previous change: tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but sligthly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`. We also reorganize the main code to make it readable from top-down and make members private where they can be.

… around 50% of the native asset related code) (flutter#155430)" (flutter#155745) Changes to original CL: The code that issues an error on unsupported operating system in the dry-run case was missing a case for iOS and Android Original CL description tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but slightly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

… around 50% of the native asset related code) (flutter/flutter#155430)

flutter/flutter@538e742...fa402c8 2024-09-26 [email protected] Roll Flutter Engine from 6328a0597b68 to 9e6133e8d906 (1 revision) (flutter/flutter#155762) 2024-09-26 [email protected] Roll Packages from 7da2374 to f38b780 (2 revisions) (flutter/flutter#155760) 2024-09-26 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.4.1 to 4.5.0 (flutter/flutter#150229) 2024-09-26 [email protected] Roll Flutter Engine from 08f935236e45 to 6328a0597b68 (1 revision) (flutter/flutter#155750) 2024-09-26 [email protected] Roll Flutter Engine from 3a520a2a4399 to 08f935236e45 (1 revision) (flutter/flutter#155748) 2024-09-26 [email protected] Reland "[flutter_tools] Cleanup of native asset related code (removes around 50% of the native asset related code) (#155430)" (flutter/flutter#155745) 2024-09-26 [email protected] Roll Flutter Engine from 896208ee5828 to 3a520a2a4399 (1 revision) (flutter/flutter#155744) 2024-09-26 [email protected] Roll Flutter Engine from fc6c85292b57 to 896208ee5828 (2 revisions) (flutter/flutter#155743) 2024-09-26 [email protected] Roll Flutter Engine from 3719454a879f to fc6c85292b57 (1 revision) (flutter/flutter#155738) 2024-09-26 [email protected] added ability to configure shadow in banner (flutter/flutter#155296) 2024-09-26 [email protected] Roll Flutter Engine from d4850c1ae648 to 3719454a879f (1 revision) (flutter/flutter#155736) 2024-09-26 [email protected] Roll Flutter Engine from d6d5fdba6ae1 to d4850c1ae648 (18 revisions) (flutter/flutter#155733) 2024-09-25 [email protected] Move the Linux runner into a subdirectory (flutter/flutter#153812) 2024-09-25 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.7 to 4.2.0 (flutter/flutter#155711) 2024-09-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[flutter_tools] Cleanup of native asset related code (removes around 50% of the native asset related code) (#155430)" (flutter/flutter#155713) 2024-09-25 [email protected] [flutter_tools] Cleanup of native asset related code (removes around 50% of the native asset related code) (flutter/flutter#155430) 2024-09-25 [email protected] reduce warnings inside flutter.groovy file #2 (flutter/flutter#155628) 2024-09-25 [email protected] [Android] Update `SystemUiMode` and `setSystemChromeEnabledSystemUIMode ` docs to note targeting Android 15+ change (flutter/flutter#153466) 2024-09-25 [email protected] mark linux packages autoroller bringup: true (flutter/flutter#155705) 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] 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

…50% of the native asset related code) (flutter#155430) tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but slightly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

…s around 50% of the native asset related code) (flutter#155430)" (flutter#155713) Reverts: flutter#155430 Initiated by: eyebrowsoffire Reason for reverting: Postsubmit failures closing the tree. See the following examples: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20native_assets_ios/5738/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_arm64_mokey%20native_assets_android/583/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Linux_pixel_7pro%20native_assets_android/4075/overview https://ci.chromium.org/u Original PR Author: mkustermann Reviewed By: {bkonyi, dcharkes} This change reverts the following previous change: tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but sligthly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`. We also reorganize the main code to make it readable from top-down and make members private where they can be.

… around 50% of the native asset related code) (flutter#155430)" (flutter#155745) Changes to original CL: The code that issues an error on unsupported operating system in the dry-run case was missing a case for iOS and Android Original CL description tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but slightly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

…50% of the native asset related code) (flutter#155430) tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but slightly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

…s around 50% of the native asset related code) (flutter#155430)" (flutter#155713) Reverts: flutter#155430 Initiated by: eyebrowsoffire Reason for reverting: Postsubmit failures closing the tree. See the following examples: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20native_assets_ios/5738/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_arm64_mokey%20native_assets_android/583/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Linux_pixel_7pro%20native_assets_android/4075/overview https://ci.chromium.org/u Original PR Author: mkustermann Reviewed By: {bkonyi, dcharkes} This change reverts the following previous change: tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but sligthly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`. We also reorganize the main code to make it readable from top-down and make members private where they can be.

… around 50% of the native asset related code) (flutter#155430)" (flutter#155745) Changes to original CL: The code that issues an error on unsupported operating system in the dry-run case was missing a case for iOS and Android Original CL description tl;dr Removes 50% (>1650 locs) of native asset related code in `packages/flutter_tools` Before this PR the invocation of dart build/link/dry-run was implemented per OS. This lead to very large code duplication of almost identical, but slightly different code. It also led to similarly duplicated test code. Almost the entire dart build/link/dry-run implementation is identical across OSes. There's small variations: - configuration of the build (e.g. android/macos/ios version, ios sdk, ...) - determining target locations & copying the final shared libraries This PR unifies the implementation by reducing the code to basically two main functions: * `runFlutterSpecificDartBuild` which is responsible for - obtain flutter configuration - perform dart build (& link) - determine target location & install binaries * `runFlutterSpecificDartDryRunOnPlatforms` which is responsible for a similar (but not same): - obtain flutter configuration - perform dart dry run - determine target location these two functions will call out to helpers for the OS specific functionality: * `_assetTargetLocationsForOS` for determining the location of the code assets * `_copyNativeCodeAssetsForOS` for copying the code assets (and possibly overriting the install name, etc) => Since we get rid of the code duplication across OSes and have only a single code path for the build/link/dry-run, we can also remove the duplicated tests that were pretty much identical across OSes. We also harden the building code by adding asserts, e.g. * the dry fun functionality should never be used by `flutter test` * the `build/native_assets/<os>/native_assets.yaml` should only be used by `flutter test` and the dry-run of `flutter run` => We change the tests to also comply with these invariants (so the tests are not testing things that cannot happen in reality) We also rename `{,Flutter}NativeAssetsBuildRunner` to disambiguate it from the `package:native_asset_builder`'s `NativeAssetsBuildRunner`.

… around 50% of the native asset related code) (flutter/flutter#155430)

github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels Sep 19, 2024

mkustermann force-pushed the cleanup-native-assets-in-flutter-tools branch from a65c963 to 51b0a0d Compare September 20, 2024 08:43

mkustermann requested review from dcharkes and stuartmorgan-g September 20, 2024 11:11

dcharkes approved these changes Sep 23, 2024

View reviewed changes

mkustermann force-pushed the cleanup-native-assets-in-flutter-tools branch from 7b7f588 to f8b04ae Compare September 24, 2024 10:38

mkustermann requested review from bkonyi and removed request for stuartmorgan-g September 24, 2024 10:39

dcharkes approved these changes Sep 24, 2024

View reviewed changes

packages/flutter_tools/test/integration.shard/isolated/native_assets_test.dart Outdated Show resolved Hide resolved

bkonyi approved these changes Sep 25, 2024

View reviewed changes

mkustermann added 4 commits September 25, 2024 21:47

add trailing comma again

09c9ca8

address comments

8f9591f

Change StateError -> UnimplementedError

396b3d0

mkustermann force-pushed the cleanup-native-assets-in-flutter-tools branch from 608577d to 396b3d0 Compare September 25, 2024 19:47

mkustermann merged commit 621e7ef into flutter:master Sep 25, 2024

eyebrowsoffire added the revert Autorevert PR (with "Reason for revert:" comment) label Sep 25, 2024

auto-submit bot pushed a commit that referenced this pull request Sep 25, 2024

Revert "[flutter_tools] Cleanup of native asset related code (removes…

b222ba9

… around 50% of the native asset related code) (#155430)" This reverts commit 621e7ef.

auto-submit bot mentioned this pull request Sep 25, 2024

Reverts "[flutter_tools] Cleanup of native asset related code (removes around 50% of the native asset related code) (#155430)" #155713

Merged

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

engine-flutter-autoroll mentioned this pull request Sep 26, 2024

Roll Flutter from 538e7424d018 to 49873fd23606 (17 revisions) flutter/packages#7710

Closed

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

621e7ef [flutter_tools] Cleanup of native asset related code (removes…

7126265

… around 50% of the native asset related code) (flutter/flutter#155430)

engine-flutter-autoroll mentioned this pull request Sep 26, 2024

Roll Flutter from 538e7424d018 to 7ec6083bed3b (18 revisions) flutter/packages#7711

Closed

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

621e7ef [flutter_tools] Cleanup of native asset related code (removes…

b345153

… around 50% of the native asset related code) (flutter/flutter#155430)

engine-flutter-autoroll mentioned this pull request Sep 26, 2024

Roll Flutter from 538e7424d018 to fa402c8057a1 (19 revisions) flutter/packages#7713

Merged

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

621e7ef [flutter_tools] Cleanup of native asset related code (removes…

6050a0e

… around 50% of the native asset related code) (flutter/flutter#155430)

engine-flutter-autoroll mentioned this pull request Sep 26, 2024

Roll Flutter from 538e7424d018 to 20bc0a9cb8df (21 revisions) flutter/packages#7715

Closed

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

621e7ef [flutter_tools] Cleanup of native asset related code (removes…

420402d

… around 50% of the native asset related code) (flutter/flutter#155430)

engine-flutter-autoroll mentioned this pull request Sep 26, 2024

Roll Flutter from 538e7424d018 to feb9c592840a (22 revisions) flutter/packages#7717

Closed

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

621e7ef [flutter_tools] Cleanup of native asset related code (removes…

3be1b23

… around 50% of the native asset related code) (flutter/flutter#155430)

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

621e7ef [flutter_tools] Cleanup of native asset related code (removes…

c06729c

… around 50% of the native asset related code) (flutter/flutter#155430)

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

621e7ef [flutter_tools] Cleanup of native asset related code (removes…

e7bf27d

… around 50% of the native asset related code) (flutter/flutter#155430)

[flutter_tools] Cleanup of native asset related code (removes around 50% of the native asset related code) #155430

[flutter_tools] Cleanup of native asset related code (removes around 50% of the native asset related code) #155430

Uh oh!

Conversation

mkustermann commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkustermann commented Sep 20, 2024

Uh oh!

dcharkes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcharkes commented Sep 23, 2024

Uh oh!

mkustermann commented Sep 23, 2024

Uh oh!

mkustermann commented Sep 24, 2024

Uh oh!

dcharkes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkonyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkustermann commented Sep 25, 2024

Uh oh!

eyebrowsoffire commented Sep 25, 2024

Uh oh!

mkustermann commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mkustermann commented Sep 19, 2024 •

edited

Loading

mkustermann commented Sep 26, 2024 •

edited

Loading