Skip to content

Switch iOS gen_snapshot from multi-arch binary to multiple binaries #37445

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

Merged
merged 3 commits into from
Aug 2, 2019
Merged

Switch iOS gen_snapshot from multi-arch binary to multiple binaries #37445

merged 3 commits into from
Aug 2, 2019

Conversation

liamappelbe
Copy link
Contributor

Description

As of flutter/engine#10430, we no longer combine the iOS armv7 and arm64 gen_snapshots into a single binary. Instead they're uploaded as two separate binaries in the same directory (https://chromium-review.googlesource.com/c/chromium/tools/build/+/1731443). So this change chooses the gen_snapshot to match the iosArch.

Related Issues

#22598

Tests

I built the flutter examples in this mode using Xcode 10 and verified that they work on an iPhone 4S.

Manual roll

git log b41c172..eac7ef0 --no-merges --oneline
flutter/engine@eac7ef065 Add copy_gen_snapshots.py tool (flutter/engine#10430)
flutter/engine@c3cc104e1 Fix mac gen_snapshot uploader (flutter/engine#10423)
flutter/engine@310bc030b Make kernel compiler use host toolchain (flutter/engine#10419)
flutter/engine@9fca3c744 Use simarm_x64 when targeting arm (flutter/engine#10010)
flutter/engine@bf9288597 [fuchsia] Add kernel_compiler target in build_fuchsia script (flutter/engine#10403)

@liamappelbe liamappelbe requested a review from cbracken August 2, 2019 00:29
@fluttergithubbot
Copy link
Contributor

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

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

/cc @dnfield

@fluttergithubbot fluttergithubbot added engine flutter/engine repository. See also e: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Aug 2, 2019
@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #37445 into master will increase coverage by 1.62%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #37445      +/-   ##
==========================================
+ Coverage   54.07%   55.69%   +1.62%     
==========================================
  Files         193      193              
  Lines       18018    18017       -1     
==========================================
+ Hits         9743    10035     +292     
+ Misses       8275     7982     -293
Flag Coverage Δ
#flutter_tool 55.69% <0%> (+1.62%) ⬆️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/base/build.dart 73.71% <0%> (+27.12%) ⬆️
...lutter_tools/lib/src/android/android_emulator.dart 32.65% <0%> (-30.62%) ⬇️
packages/flutter_tools/lib/src/cache.dart 41.99% <0%> (-0.73%) ⬇️
packages/flutter_tools/lib/src/vmservice.dart 37.15% <0%> (-0.31%) ⬇️
packages/flutter_tools/lib/src/base/logger.dart 82.44% <0%> (+0.38%) ⬆️
packages/flutter_tools/lib/src/base/process.dart 80.46% <0%> (+0.78%) ⬆️
packages/flutter_tools/lib/src/base/version.dart 83.33% <0%> (+2.38%) ⬆️
packages/flutter_tools/lib/src/device.dart 57.22% <0%> (+4.21%) ⬆️
...ackages/flutter_tools/lib/src/commands/config.dart 85.89% <0%> (+12.82%) ⬆️
packages/flutter_tools/lib/src/base/utils.dart 92.57% <0%> (+13.36%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af1bbc4...10b9458. Read the comment docs.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

We should add a GenSnapshot test that checks that we issue the run command with the appropriate binary name.

LGTM stamp from a Japanese personal seal

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
engine flutter/engine repository. See also e: labels. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants