Clean up create_macos_gen_snapshots.py options #53954

cbracken · 2024-07-16T23:55:08Z

This patch updates create_macos_gen_snapshots.py to take the path to the input gen_snapshot binaries rather than just the path to the buildroot, and hardcoding the binary names and buildroot-relative paths internally.

This has a couple advantages:

Those grepping the source for gen_snapshot to see where it's bundled will immediately find it in the recipe JSON as a parameter to this script.
In future (the next patch I send out) we can pass different input paths that point to gen_snapshot binaries that are universal binaries. The current binaries are single-architecture x64 host binaries.

This is a refactoring with no semantic change, therefore no tests have changed.

Background

create_macos_gen_snapshot.py is used to:

Generate a named copy of gen_snapshot for the specified target architecture: e.g. gen_snapshot_arm64, gen_snapshot_x64
Strip bitcode, if present. Today, bitcode is no longer enabled by default.
Create a zip archive containing the (two) gen_snapshot binaries and bundle with an entitlements.txt file.

Issue: flutter/flutter#151848

This is refactoring to support:
Issue: flutter/flutter#101138
Issue: flutter/flutter#69157

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've dealt with this script more than any one person should ever have to deal with such a thing in one lifetime.
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.

flutter-dashboard · 2024-07-16T23:55:11Z

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.

ci/builders/mac_host_engine.json

This patch updates `create_macos_gen_snapshots.py` to take the path to the input `gen_snapshot` binaries rather than just the path to the buildroot, and hardcoding the binary names and buildroot-relative paths internally. This has a couple advantages: * Those grepping the source for gen_snapshot to see where it's bundled will immediately find it in the recipe JSON as a parameter to this script. * In future (the next patch I send out) we can pass different input paths that point to gen_snapshot binaries that are universal binaries. The current binaries are single-architecture x64 host binaries. Background ---------- `create_macos_gen_snapshot.py` is used to: * Generate a named copy of gen_snapshot for the specified target architecture: e.g. gen_snapshot_arm64, gen_snapshot_x64 * Strip bitcode, if present. Today, bitcode is no longer enabled by default. * Create a zip archive containing the (two) gen_snapshot binaries and bundle with an entitlements.txt file. This is a refactoring with no semantic change, therefore no tests have changed. Issue: flutter/flutter#151848 This is refactoring to support: Issue: flutter/flutter#101138 Issue: flutter/flutter#69157

christopherfujino

LGTM

stuartmorgan-g · 2024-07-17T13:35:48Z

test-exempt: code refactor with no semantic change

…ine#53954)

…151905) flutter/engine@45b722b...39ee1a5 2024-07-17 [email protected] Clean up create_macos_gen_snapshots.py options (flutter/engine#53954) 2024-07-17 [email protected] Roll Skia from def61bdd977a to 4d2047aa3c8d (1 revision) (flutter/engine#53960) 2024-07-17 [email protected] Roll Dart SDK from 7368b1d084b0 to d679a4c98193 (3 revisions) (flutter/engine#53961) 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 [email protected],[email protected] 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

This is only ever used for gen_snapshot_arm64 where the value will only ever be 'clang_x64'. If we were to migrate builds to arm64 hosts, the gen_snapshot_x64 target would be broken as the script stands today. This flag is never passed by recipe infra or outside callers, and can thus be safely inlined. The same refactoring was performed for generating universal gen_snapshots in #53954. A followup patch will update the location where this gen_snapshot binary is generated (and adds an arm64 host binary build), and the merged universal gen_snapshot_${target_cpu} will be written to the root output directory. This is minor pre-factoring to simplify the followup patch, which migrates to universal binaries for iOS. Issue: flutter/flutter#101138 Issue: flutter/flutter#69157

…lutter#151905) flutter/engine@45b722b...39ee1a5 2024-07-17 [email protected] Clean up create_macos_gen_snapshots.py options (flutter/engine#53954) 2024-07-17 [email protected] Roll Skia from def61bdd977a to 4d2047aa3c8d (1 revision) (flutter/engine#53960) 2024-07-17 [email protected] Roll Dart SDK from 7368b1d084b0 to d679a4c98193 (3 revisions) (flutter/engine#53961) 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 [email protected],[email protected] 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 requested review from christopherfujino, jmagman and zanderso July 16, 2024 23:55

zanderso reviewed Jul 16, 2024

View reviewed changes

ci/builders/mac_host_engine.json Show resolved Hide resolved

zanderso approved these changes Jul 17, 2024

View reviewed changes

christopherfujino approved these changes Jul 17, 2024

View reviewed changes

cbracken merged commit 39ee1a5 into flutter:main Jul 17, 2024

cbracken deleted the create-macos-gen-snapshot-explicit-path branch July 17, 2024 16:40

engine-flutter-autoroll mentioned this pull request Jul 17, 2024

Roll Flutter Engine from 45b722b661f0 to 39ee1a549581 (3 revisions) flutter/flutter#151905

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 17, 2024

39ee1a549 Clean up create_macos_gen_snapshots.py options (flutter/eng…

037f7c1

…ine#53954)

cbracken mentioned this pull request Jul 19, 2024

[iOS] Eliminate --clang-dir flag #54008

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Clean up create_macos_gen_snapshots.py options #53954

Clean up create_macos_gen_snapshots.py options #53954

Uh oh!

cbracken commented Jul 16, 2024 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Jul 16, 2024

Uh oh!

Uh oh!

christopherfujino left a comment

Uh oh!

stuartmorgan-g commented Jul 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Clean up create_macos_gen_snapshots.py options #53954

Clean up create_macos_gen_snapshots.py options #53954

Uh oh!

Conversation

cbracken commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jul 16, 2024

Uh oh!

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jul 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cbracken commented Jul 16, 2024 •

edited

Loading