Fix frameworks added to bundle multiple times instead of lipo #144688

knopp · 2024-03-06T14:00:06Z

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.

flutter-dashboard · 2024-03-06T14:00:09Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

knopp · 2024-03-06T16:28:38Z

@dcharkes, I updated test to actually check lipoing multiple architectures. I did have to introduce _FatNativeAssetsBuildRunner, which doesn't look very nice. Maybe FakeNativeAssetsBuildRunner could be modified to accept (target) => result getter as buildResult instead of plain result?

dcharkes · 2024-03-06T16:29:11Z

I believe the iOS implementation might have the same issue.

dcharkes · 2024-03-06T16:35:44Z

@dcharkes, I updated test to actually check lipoing multiple architectures.

❤️ (Thanks for doing what I should have done before! 🤓 )

I did have to introduce _FatNativeAssetsBuildRunner, which doesn't look very nice. Maybe FakeNativeAssetsBuildRunner could be modified to accept (target) => result getter as buildResult instead of plain result?

Both of these are fakes, I wouldn't worry about the setup.
If it doesn't complicate the other places we can fold it into the existing fake. (e.g. if the function is passed run it, and otherwise return the predefined list).
But it's also fine to have two fakes instead of one fake. In that case, maybe name the new one _FakeNativeAssetsBuildRunner and the other one ConstFakeXX (e.g. the constness is that the responses are hardcoded).

knopp · 2024-03-06T16:52:21Z

@dcharkes, I did that here, but I'm quite sure about it. Maybe the old one should have remained FakeNativeAssetsBuilder and the new one have different name? Not sure. Your call.

dcharkes · 2024-03-06T17:31:45Z

How does the code look like if you merge the functionality into one fake?

e.g. return (onBuild!=null) ? onBuild(...) : buildResult;

knopp · 2024-03-06T17:41:00Z

How does the code look like if you merge the functionality into one fake?

e.g. return (onBuild!=null) ? onBuild(...) : buildResult;

Quite a lot better I'd say.

knopp · 2024-03-07T12:52:35Z

@dcharkes, is anything else needed for the PR? should be ready for review.

dcharkes

Looks I left off halfway through the review.

LGTM!

dcharkes · 2024-03-06T18:17:51Z

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

-              target: native_assets_cli.Target.iOSArm64,
-              path: AssetAbsolutePath(Uri.file('libbar.dylib')),
-            ),
+            if (target == native_assets_cli.Target.iOSArm64)


Can we remove the if and return something with target: target and path: ... ${target.architecture}/libbar.dylib'?

ditto in iOS test

dcharkes · 2024-03-06T18:18:42Z

packages/flutter_tools/test/general.shard/isolated/fake_native_assets_build_runner.dart

    buildInvocations++;
    lastBuildMode = buildMode;
-    return buildResult;
+    return onBuild?.call(target) ?? buildResult;


Much cleaner indeed! 😄

dcharkes · 2024-03-07T15:12:19Z

Thanks @knopp!

…flutter/flutter#144688)

flutter/flutter@8f84f3f...471a828 2024-03-07 [email protected] [flutter_test] Use defaultTargetPlatform for key events simulation (flutter/flutter#143579) 2024-03-07 [email protected] Roll Packages from 9b88dbc to 6701c9e (3 revisions) (flutter/flutter#144772) 2024-03-07 [email protected] Fix frameworks added to bundle multiple times instead of lipo (flutter/flutter#144688) 2024-03-07 [email protected] Roll Flutter Engine from 40a42796b129 to f8c3b2db8cd1 (1 revision) (flutter/flutter#144766) 2024-03-07 [email protected] Roll Flutter Engine from 0246484d2bae to 40a42796b129 (1 revision) (flutter/flutter#144765) 2024-03-07 [email protected] Roll Flutter Engine from 6c1751bd774e to 0246484d2bae (1 revision) (flutter/flutter#144756) 2024-03-07 [email protected] [flutter_test] Change KeyEventSimulator default transit mode (flutter/flutter#143847) 2024-03-07 [email protected] Roll Flutter Engine from 03ebd6460b83 to 6c1751bd774e (2 revisions) (flutter/flutter#144747) 2024-03-07 [email protected] Roll Flutter Engine from 8a859c5b3a2d to 03ebd6460b83 (2 revisions) (flutter/flutter#144746) 2024-03-07 [email protected] Roll Flutter Engine from 4f6ea31d1f25 to 8a859c5b3a2d (2 revisions) (flutter/flutter#144743) 2024-03-07 [email protected] Fix memory leak in `editable_gesture_test.dart` (flutter/flutter#144691) 2024-03-07 [email protected] Roll Flutter Engine from 53ddbdfc24e5 to 4f6ea31d1f25 (2 revisions) (flutter/flutter#144741) 2024-03-07 [email protected] Roll Flutter Engine from b2adf7471d3d to 53ddbdfc24e5 (1 revision) (flutter/flutter#144735) 2024-03-06 [email protected] Roll Flutter Engine from 31bbe61dfa0d to b2adf7471d3d (1 revision) (flutter/flutter#144732) 2024-03-06 [email protected] Roll Flutter Engine from 5bbac1a5c576 to 31bbe61dfa0d (3 revisions) (flutter/flutter#144724) 2024-03-06 [email protected] Run macOS test on `dev/integration_tests/ui` (flutter/flutter#142735) 2024-03-06 [email protected] Use wasm-compatible conditional import in timeline.dart, avoid emitting timeline events in SchedulerBinding (flutter/flutter#144682) 2024-03-06 [email protected] Roll Flutter Engine from 44405aedba13 to 5bbac1a5c576 (2 revisions) (flutter/flutter#144714) 2024-03-06 [email protected] Bring back firebase tests to prod (flutter/flutter#144703) 2024-03-06 [email protected] Roll Flutter Engine from 20037e385bda to 44405aedba13 (3 revisions) (flutter/flutter#144710) 2024-03-06 [email protected] Fix code sample failing in smoke test (flutter/flutter#144709) 2024-03-06 [email protected] Remove deprecated `errorColor` from `ThemeData` (flutter/flutter#144078) 2024-03-06 [email protected] make DevFSContent descendants immutable (flutter/flutter#144664) 2024-03-06 [email protected] [Impeller] measure GPU memory usage. (flutter/flutter#144575) 2024-03-06 [email protected] Roll Flutter Engine from 9aad0e93899b to 20037e385bda (1 revision) (flutter/flutter#144707) 2024-03-06 [email protected] Roll Flutter Engine from b6efe0dd88fe to 9aad0e93899b (2 revisions) (flutter/flutter#144702) 2024-03-06 [email protected] Update android templates to use target sdk 34 (flutter/flutter#144641) 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

…flutter/flutter#144688)

Fix frameworks added to bundle multiple times

e306a3c

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

knopp changed the title ~~Fix frameworks added to bundle multiple times~~ Fix frameworks added to bundle multiple times instead of lipo Mar 6, 2024

Add test

73284d8

knopp requested a review from dcharkes March 6, 2024 16:25

knopp added 2 commits March 6, 2024 17:45

iOS test

15efd85

Rename FakeNativeAssetsBuildRunner to ConstFakeNativeAssetsBuildRunner

046bfdb

Revert previous change, add onBuild to FakeNativeAssetsBuildRunner

2616bd3

dcharkes approved these changes Mar 7, 2024

View reviewed changes

Remove condition

f559f5a

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

auto-submit bot merged commit de72832 into flutter:master Mar 7, 2024

knopp deleted the fix_frameworks_added_multple_times_instead_of_lipo branch March 7, 2024 15:13

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

Roll Flutter from 8f84f3f32ddc to de728320793d (25 revisions) flutter/packages#6280

Closed

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

de72832 Fix frameworks added to bundle multiple times instead of lipo (…

0a519f5

…flutter/flutter#144688)

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

Roll Flutter from 8f84f3f32ddc to 471a82856d86 (27 revisions) flutter/packages#6281

Merged

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

de72832 Fix frameworks added to bundle multiple times instead of lipo (…

2b322ca

…flutter/flutter#144688)

knopp mentioned this pull request Mar 25, 2024

[macOS] framework gets added twice dart-lang/native#978

Closed

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

de72832 Fix frameworks added to bundle multiple times instead of lipo (…

0a5e114

…flutter/flutter#144688)

knopp mentioned this pull request Jun 6, 2024

[native_assets] Fix framework name deduplication #149761

Merged

9 tasks

Fix frameworks added to bundle multiple times instead of lipo #144688

Fix frameworks added to bundle multiple times instead of lipo #144688

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Mar 6, 2024

Uh oh!

knopp commented Mar 6, 2024

Uh oh!

dcharkes commented Mar 6, 2024

Uh oh!

dcharkes commented Mar 6, 2024

Uh oh!

knopp commented Mar 6, 2024

Uh oh!

dcharkes commented Mar 6, 2024

Uh oh!

knopp commented Mar 6, 2024

Uh oh!

knopp commented Mar 7, 2024

Uh oh!

dcharkes left a comment

Choose a reason for hiding this comment

Uh oh!

dcharkes Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

dcharkes Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

dcharkes commented Mar 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

knopp commented Mar 6, 2024 •

edited

Loading