Skip to content

Conversation

@vashworth
Copy link
Contributor

@vashworth vashworth commented Aug 25, 2025

This reverts commit 0c0148b.

Fix for #174393.

Pre-launch Checklist

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

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

@vashworth vashworth requested a review from a team as a code owner August 25, 2025 19:25
@github-actions github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. team-ios Owned by iOS platform team labels Aug 25, 2025
@vashworth vashworth requested review from sstrickl and removed request for a team August 25, 2025 19:25
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request reverts a previous change that aimed to generate Mach-O dynamic libraries directly from gen_snapshot. The reverted code restores the two-step process where gen_snapshot first creates an assembly file, which is then compiled and linked into a framework using Xcode's command-line tools (cc and clang). The changes are applied consistently across the build logic and corresponding tests. My review includes a suggestion to improve the maintainability of a new test constant.

Comment on lines +19 to +38
const kDefaultClang = <String>[
'-miphoneos-version-min=13.0',
'-isysroot',
'path/to/sdk',
'-dynamiclib',
'-Xlinker',
'-rpath',
'-Xlinker',
'@executable_path/Frameworks',
'-Xlinker',
'-rpath',
'-Xlinker',
'@loader_path/Frameworks',
'-fapplication-extension',
'-install_name',
'@rpath/App.framework/App',
'-o',
'build/foo/App.framework/App',
'build/foo/snapshot_assembly.o',
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For better reusability, consider separating the output file paths from the kDefaultClang constant. This would make the constant more flexible for tests that might use different output directories. You would then need to append the output path arguments at the call sites, for example:

const FakeCommand(command: <String>[
  'xcrun', 'clang', '-arch', 'arm64',
  ...kDefaultClang,
  '-o',
  'build/foo/App.framework/App',
  'build/foo/snapshot_assembly.o',
]),
const kDefaultClang = <String>[
  '-miphoneos-version-min=13.0',
  '-isysroot',
  'path/to/sdk',
  '-dynamiclib',
  '-Xlinker',
  '-rpath',
  '-Xlinker',
  '@executable_path/Frameworks',
  '-Xlinker',
  '-rpath',
  '-Xlinker',
  '@loader_path/Frameworks',
  '-fapplication-extension',
  '-install_name',
  '@rpath/App.framework/App',
];

@chinmaygarde chinmaygarde linked an issue Aug 25, 2025 that may be closed by this pull request
@vashworth vashworth added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Aug 26, 2025
Merged via the queue into flutter:master with commit f9382a4 Aug 26, 2025
146 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 26, 2025
flutter/flutter@a4cb00a...c65f01d

2025-08-26 engine-flutter-autoroll@skia.org Roll Packages from fe66130 to 1ef712e (4 revisions) (flutter/flutter#174442)
2025-08-26 15619084+vashworth@users.noreply.github.com Revert "Directly generate a Mach-O dynamic library using gen_snapshot (#171626) (flutter/flutter#174392)
2025-08-26 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 5.4.0 to 5.5.0 in the all-github-actions group (flutter/flutter#174436)
2025-08-26 engine-flutter-autoroll@skia.org Roll Skia from 9daab16abbf9 to 21214d63fc40 (1 revision) (flutter/flutter#174431)
2025-08-26 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from UiY8gj468PZUj6QTm... to L5zGzsIWIS8N36AFQ... (flutter/flutter#174430)
2025-08-26 engine-flutter-autoroll@skia.org Roll Dart SDK from f1f90d413dd3 to 9054cd8af73c (2 revisions) (flutter/flutter#174428)
2025-08-26 engine-flutter-autoroll@skia.org Roll Skia from afb5c22d9ba0 to 9daab16abbf9 (2 revisions) (flutter/flutter#174429)
2025-08-26 engine-flutter-autoroll@skia.org Roll Skia from 2227187dbdcf to afb5c22d9ba0 (1 revision) (flutter/flutter#174425)
2025-08-26 87506348+jingshao-code@users.noreply.github.com [iOS][Secure Paste] Custom edit menu actions (flutter/flutter#171825)
2025-08-26 huy@nevercode.io Make SystemUiOverlayStyle to be diagnosticable (flutter/flutter#174018)
2025-08-26 34465683+rkishan516@users.noreply.github.com Fix: Active step fully colored in vertical mode (flutter/flutter#173152)
2025-08-26 32538273+ValentinVignal@users.noreply.github.com Migrate to use `WidgetStateProperty` (flutter/flutter#174323)
2025-08-26 engine-flutter-autoroll@skia.org Roll Skia from ed42a94ee066 to 2227187dbdcf (3 revisions) (flutter/flutter#174417)
2025-08-26 engine-flutter-autoroll@skia.org Roll Dart SDK from a0e39d9b4a58 to f1f90d413dd3 (1 revision) (flutter/flutter#174409)
2025-08-26 jason-simmons@users.noreply.github.com [Impeller] Flush the data written to the device buffer by RoundSuperellipseGeometry (flutter/flutter#174316)
2025-08-26 flar@google.com Remove obsolete vulkan_window source files (flutter/flutter#174087)
2025-08-25 1961493+harryterkelsen@users.noreply.github.com [web] Migrate non-CanvasKit-specific tests to ui/ (flutter/flutter#174396)
2025-08-25 jessy.yameogo@gmail.com Create Hot Restart over websocket test (flutter/flutter#173852)
2025-08-25 engine-flutter-autoroll@skia.org Roll Dart SDK from e283a9e88242 to a0e39d9b4a58 (1 revision) (flutter/flutter#174383)
2025-08-25 matanlurey@users.noreply.github.com Update `master` CHANGELOG for 3.35.2 (flutter/flutter#174399)
2025-08-25 engine-flutter-autoroll@skia.org Roll Skia from da724d312e65 to ed42a94ee066 (4 revisions) (flutter/flutter#174394)
2025-08-25 58529443+srujzs@users.noreply.github.com Update dwds to 25.0.3 (flutter/flutter#174379)
2025-08-25 38427679+xVemu@users.noreply.github.com Fix logic statements in year2023 documentation (flutter/flutter#174120)
2025-08-25 jason-simmons@users.noreply.github.com Release thread-local resources when submitting a Flutter GPU command buffer (flutter/flutter#173663)
2025-08-25 1961493+harryterkelsen@users.noreply.github.com [web] Refactor LayerScene out of CanvasKit (flutter/flutter#174375)
2025-08-25 15619084+vashworth@users.noreply.github.com Stream logs from `devicectl` and `lldb` (flutter/flutter#173724)
2025-08-25 jmccandless@google.com NavigatorPopScope examples no longer use deprecated onPop. (flutter/flutter#174291)
2025-08-25 mohellebiabdessalem@gmail.com fix typo in test documentation function name (flutter/flutter#174297)

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 stuartmorgan@google.com 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
mboetger pushed a commit to mboetger/flutter that referenced this pull request Sep 18, 2025
…flutter#171626) (flutter#174392)

This reverts commit 0c0148b.

Fix for flutter#174393.

## Pre-launch Checklist

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

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

**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
korca0220 pushed a commit to korca0220/flutter that referenced this pull request Sep 22, 2025
…flutter#171626) (flutter#174392)

This reverts commit 0c0148b.

Fix for flutter#174393.

## Pre-launch Checklist

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

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

**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Jaineel-Mamtora pushed a commit to Jaineel-Mamtora/flutter_forked that referenced this pull request Sep 24, 2025
…flutter#171626) (flutter#174392)

This reverts commit 0c0148b.

Fix for flutter#174393.

## Pre-launch Checklist

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

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

**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-ios Owned by iOS platform team tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Severe application size regression in benchmarks.

2 participants