Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

macOS: Clean up create_macos_framework.py (#54543) #54555

Merged
merged 1 commit into from
Aug 14, 2024
Merged

macOS: Clean up create_macos_framework.py (#54543) #54555

merged 1 commit into from
Aug 14, 2024

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Aug 14, 2024 •
edited
Loading

Over time, this script and others in sky/tools have accumulated a lot of additional and sometimes duplicate code.

This is a first pass cleanup of create_macos_framework.py to extract common utility code to sky/utils.py and refactor for better readability.

The iOS analogue of this patch was #54500.

This is a reland of #54543 (reverted in #54550), which failed to set the -y option (preserve symlinks) when zipping FlutterMacOS.xcframework but did so for the other zip steps where required. Since there is no case where we want to archive dereferenced symlinks, and because it's required for macOS frameworks, we now set this option for all callers in the sky_tools.create_zip() function to avoid future cases where someone working on this code misses one. If we ever need it, we can extract a parameter. This patch also references the correct script in its description (the original incorrectly mentioned create_ios_framework.py).

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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@cbracken
macOS: Clean up create_macos_framework.py (#54543)
807c2a9 
Over time, this script and others in sky/tools have accumulated a lot of additional and sometimes duplicate code.

This is a first pass cleanup of create_macos_framework.py to extract common utility code to utils.py and refactor for better readability.

The iOS analogue of this patch was #54500.

This is a reland of #54543, which failed to set the `-y` option on zip when zipping `FlutterMacOS.xcframework` but did so for the other zip steps where required. Since there is no case where we *want* to archive dereferenced symlinks, and because it's required for macOS frameworks, we now set this option for all callers in the `sky_tools.create_zip()` function to avoid future cases where someone working on this code misses one. If we ever need it, we can extract a parameter. This patch also references the correct script in its description (the original incorrectly mentioned `create_ios_framework.py`).

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@cbracken cbracken requested review from zanderso and jmagman August 14, 2024 17:48
cbracken added a commit to cbracken/flutter that referenced this pull request Aug 14, 2024
@cbracken
TEST ROLL: DO NOT SUBMIT
64414b5 
This is a test roll to verify changes in flutter/engine#54555.

DO NOT SUBMIT
@cbracken cbracken mentioned this pull request Aug 14, 2024
Closed
9 tasks
cbracken added a commit to cbracken/flutter that referenced this pull request Aug 14, 2024
@cbracken
TEST ROLL: DO NOT SUBMIT
af3b474 
This is a test roll to verify changes in flutter/engine#54555.

DO NOT SUBMIT
@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 14, 2024
@auto-submit auto-submit bot merged commit 76a1c64 into main Aug 14, 2024
27 checks passed
@auto-submit auto-submit bot deleted the reland-macos-refactor branch August 14, 2024 19:55
@cbracken cbracken mentioned this pull request Aug 14, 2024
Merged
8 tasks
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 14, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 14, 2024
@engine-flutter-autoroll
76a1c64f4 macOS: Clean up create_macos_framework.py (flutter#54543) (f…
723ef86 
…lutter/engine#54555)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 14, 2024
@engine-flutter-autoroll
Roll Flutter Engine from 9b8421662ec9 to 76a1c64f4e63 (1 revision) (#…
fafe3df 
…153465)

flutter/engine@9b84216...76a1c64

2024-08-14 chris@bracken.jp macOS: Clean up create_macos_framework.py (#54543) (flutter/engine#54555)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: 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
@cbracken
Copy link
Member Author

Rolled successfully to the framework this time in flutter/flutter#153465.

auto-submit bot pushed a commit that referenced this pull request Aug 14, 2024
@cbracken
macOS: Refactor create_macos_framework.py (#54557)
7949ab3 
This is a refactoring with no semantic changes.

This refactors the macOS framework creation code to be more readable, and extracts it to `sky_utils.py`.

While I was pulling this out, also generalised the code to not hardcode `FlutterMacOS.framework` in case we one day manage to generate the iOS and macOS frameworks with the same name.

This is a reland of #54546 (reverted in #54549), the original was reverted in order to revert #54543 (reverted in #54550), which was reverted because it failed to preserve symlinks while zipping the macOS framework. That patch has been relanded with a fix in #54555. This patch has been rebased to tip-of-tree for attempt two.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@auto-submit auto-submit bot mentioned this pull request Aug 15, 2024
Merged
8 tasks
auto-submit bot added a commit that referenced this pull request Aug 15, 2024
@auto-submit
Reverts "macOS: Refactor create_macos_framework.py (#54557)" (#54566)
971ddd9 
Reverts: #54557
Initiated by: zanderso
Reason for reverting: Failing on framework tests on the roll https://ci.chromium.org/ui/p/flutter/builders/try/Mac_arm64%20run_debug_test_macos/9841/overview
Original PR Author: cbracken

Reviewed By: {jmagman}

This change reverts the following previous change:
This is a refactoring with no semantic changes.

This refactors the macOS framework creation code to be more readable, and extracts it to `sky_utils.py`.

While I was pulling this out, also generalised the code to not hardcode `FlutterMacOS.framework` in case we one day manage to generate the iOS and macOS frameworks with the same name.

This is a reland of #54546 (reverted in #54549), the original was reverted in order to revert #54543 (reverted in #54550), which was reverted because it failed to preserve symlinks while zipping the macOS framework. That patch has been relanded with a fix in #54555. This patch has been rebased to tip-of-tree for attempt two.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
DBowen33 pushed a commit to DBowen33/flutter that referenced this pull request Aug 16, 2024
@engine-flutter-autoroll @DBowen33
Roll Flutter Engine from 9b8421662ec9 to 76a1c64f4e63 (1 revision) (f…
ce63430 
…lutter#153465)

flutter/engine@9b84216...76a1c64

2024-08-14 chris@bracken.jp macOS: Clean up create_macos_framework.py (flutter#54543) (flutter/engine#54555)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: 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
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
@engine-flutter-autoroll @Buchimi
Roll Flutter Engine from 9b8421662ec9 to 76a1c64f4e63 (1 revision) (f…
e1b72e5 
…lutter#153465)

flutter/engine@9b84216...76a1c64

2024-08-14 chris@bracken.jp macOS: Clean up create_macos_framework.py (flutter#54543) (flutter/engine#54555)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jmagman jmagman jmagman approved these changes

@zanderso zanderso zanderso approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cbracken @jmagman @zanderso