Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[macOS] Archive universal gen_snapshot binaries #53962

Merged

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Jul 17, 2024

In #53524, we started producing universal gen_snapshot_arm64 and gen_snapshot_x64 binaries. This migrates away from bundling gen_snapshot binaries with x64-only host architecture to universal binaries that bundle both x64 and arm64 host architectures.

Also adds a dependency on flutter/lib/snapshot:create_macos_gen_snapshots to the profile build, where it was previously missing. Presumably, gen_snapshot was being built as a side-effect of one of the other targets.

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 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 have looked into the abyss of our archive generation tooling, and have developed deep sense of dread.
  • 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.

In flutter#53524, we started producing universal `gen_snapshot_arm64` and
`gen_snapshot_x64` binaries. This migrates away from bundling
`gen_snapshot` binaries with x64-only host architecture to universal
binaries that bundle both x64 and arm64 host architectures.

Issue: flutter/flutter#101138
Issue: flutter/flutter#69157
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm

you might run into trouble with this:

https://github.com/flutter/engine/blob/main/ci/builders/mac_ios_engine.json#L518

but I believe it is safe to delete, if so.

@cbracken
Copy link
Member Author

cbracken commented Jul 17, 2024

I'm fairly convinced that gen_snapshot is the [suspected-]unused arm64-only gen_snapshot in the tool cache; I've got a followup that removes this to see if anything explodes, but my sense is that given this would have been unusable on Intel Macs for well over a year now, that we should have had bug reports from Intel Mac users by now if it were used.

Doing some testing since I could still imagine tool internals doing things like checking for an trying to re-cache it if missing.

@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 17, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 17, 2024
Copy link
Contributor

auto-submit bot commented Jul 17, 2024

auto label is removed for flutter/engine/53962, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@christopherfujino
Copy link
Member

auto label is removed for flutter/engine/53962, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

Whoops:

Cannot find gen_snapshot at /Volumes/Work/s/w/ir/cache/builder/src/out/ci/mac_profile_arm64/gen_snapshot_arm64

@cbracken
Copy link
Member Author

Fixed: turns out we never actually depended on the lib/snapshots:create_macos_gen_snapshots target (or the archive target we used to depend on)... but only for profile mode. Therefore it didn't show up in my find-and-replace patch yesterday to swap the dependency to the create_macos_gen_snapshots target. The old unsuffixed one is build as a side-effect of one of the other targets being depended on, the new ones aren't.

@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 17, 2024
@auto-submit auto-submit bot merged commit 9395461 into flutter:main Jul 17, 2024
31 checks passed
@cbracken cbracken deleted the create-macos-gen-snapshot-universal branch July 17, 2024 19:17
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 17, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 17, 2024
…151915)

flutter/engine@39ee1a5...e9dc620

2024-07-17 [email protected] Roll Skia from 485bf4ff886b to 5efad82d5387 (1 revision) (flutter/engine#53968)
2024-07-17 [email protected] [macOS] Archive universal gen_snapshot binaries (flutter/engine#53962)
2024-07-17 [email protected] Roll Skia from 4d2047aa3c8d to 485bf4ff886b (1 revision) (flutter/engine#53963)

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
TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this pull request Aug 7, 2024
…lutter#151915)

flutter/engine@39ee1a5...e9dc620

2024-07-17 [email protected] Roll Skia from 485bf4ff886b to 5efad82d5387 (1 revision) (flutter/engine#53968)
2024-07-17 [email protected] [macOS] Archive universal gen_snapshot binaries (flutter/engine#53962)
2024-07-17 [email protected] Roll Skia from 4d2047aa3c8d to 485bf4ff886b (1 revision) (flutter/engine#53963)

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
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
…lutter#151915)

flutter/engine@39ee1a5...e9dc620

2024-07-17 [email protected] Roll Skia from 485bf4ff886b to 5efad82d5387 (1 revision) (flutter/engine#53968)
2024-07-17 [email protected] [macOS] Archive universal gen_snapshot binaries (flutter/engine#53962)
2024-07-17 [email protected] Roll Skia from 4d2047aa3c8d to 485bf4ff886b (1 revision) (flutter/engine#53963)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants