Skip to content

Include debug symbol in xcframework when building iOS-framework - #80065

Merged
jmagman merged 5 commits into
flutter:masterfrom
HelloCore:feature/xcframework-dsym
Apr 14, 2021
Merged

Include debug symbol in xcframework when building iOS-framework#80065
jmagman merged 5 commits into
flutter:masterfrom
HelloCore:feature/xcframework-dsym

Conversation

@HelloCore

@HelloCore HelloCore commented Apr 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fix #80063 #64730

Adding dSYM and bcsymbolmap to xcframework using -debug-symbols flag.

after_dsym

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, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboard

flutter-dashboard Bot commented Apr 8, 2021

Copy link
Copy Markdown

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 Hixie on the #hackers channel in Chat.

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.

@flutter-dashboard flutter-dashboard Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 8, 2021
@google-cla google-cla Bot added the cla: yes label Apr 8, 2021
@jonahwilliams
jonahwilliams requested a review from jmagman April 8, 2021 20:47
jmagman
jmagman requested changes Apr 8, 2021
View reviewed changes

@jmagman jmagman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This needs tests. Unfortunately this file is essentially un-unit tested, but pretty easy to add to the integration test https://github.com/flutter/flutter/blob/master/dev/devicelab/bin/tasks/build_ios_framework_module_test.dart

Comment thread packages/flutter_tools/lib/src/commands/build_ios_framework.dart Outdated
defaultsTo: true,
hide: !verboseHelp,
)
..addFlag('include-dsym',

@jmagman jmagman Apr 8, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any reason to not include it? I'd rather just include it instead of an undiscoverable flag and let Xcode filter it as needed on the embedding side.

@HelloCore HelloCore Apr 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm not sure if Flutter team intended to exclude dSYM or not, so I added flag just in case, I'm going to remove it.

@linhzz999

linhzz999 commented Apr 10, 2021

Copy link
Copy Markdown

Ok

@HelloCore

HelloCore commented Apr 10, 2021

Copy link
Copy Markdown
Contributor Author

@jmagman I have added more check to the integration test.

For BCSymbolMaps, I'm not sure how to test, because Xcode generated UUID as a file name, for example, BCSymbolMaps/B2F12025-2869-3357-886A-209E722F2B02.bcsymbolmap.

So, I tested by checking if directory BCSymbolMaps exists instead.

@HelloCore HelloCore changed the title Add option to include debug symbol in xcframework Include debug symbol in xcframework when building iOS-framework Apr 10, 2021
@flutter-dashboard flutter-dashboard Bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Apr 10, 2021
jmagman
jmagman reviewed Apr 13, 2021
View reviewed changes

@jmagman jmagman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I commented about a few minor nits, and the analyzer warning.

Once those are fixed, this looks good! Thank you for working on it!

Comment thread packages/flutter_tools/lib/src/commands/build_ios_framework.dart Outdated
Comment thread dev/devicelab/bin/tasks/build_ios_framework_module_test.dart Outdated
Comment thread dev/devicelab/bin/tasks/build_ios_framework_module_test.dart Outdated
jmagman
jmagman approved these changes Apr 14, 2021
View reviewed changes

@jmagman jmagman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@fluttergithubbot

fluttergithubbot commented Apr 14, 2021

Copy link
Copy Markdown
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Windows gradle_plugin_light_apk_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

@jmagman
jmagman merged commit d6fbfdb into flutter:master Apr 14, 2021
@itlwy

itlwy commented Jun 17, 2021

Copy link
Copy Markdown

Hello,
I used these commits of this issue to modify my local flutter_tools (version of 2.0.4) ,but it has no effect.
I found that there are no dSYM and bcsymbolmap in the xcframework directory during the compilation process.

Looking forward to reply，thank you.
@HelloCore

flutter doctor

Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel stable, 2.0.4, on Mac OS X 10.15.5 19F101 darwin-x64, locale zh-Hans-CN)
[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.3)
[✓] Xcode - develop for iOS and macOS
[✓] Chrome - develop for the web
[✓] Android Studio (version 4.1)
[✓] IntelliJ IDEA Community Edition (version 2020.3)
[✓] VS Code (version 1.57.0)
[✓] Connected device (3 available)

• No issues found!

@HelloCore

HelloCore commented Jun 17, 2021

Copy link
Copy Markdown
Contributor Author

@itlwy
If you modify the file locally, you might use dart to call that file directly.

I'm using this command.

$ dart .fvm/flutter_sdk/packages/flutter_tools/bin/flutter_tools.dart build ios-framework --no-debug --no-profile --cocoapods

@itlwy

itlwy commented Jun 17, 2021

Copy link
Copy Markdown

I think it’s not for this reason.
I have deleted /path/to/flutter_sdk/bin/cache/flutter_tools.stamp and rebuilt.
Please see the picture below:
QQ20210617-141642

@HelloCore

@jmagman jmagman mentioned this pull request Oct 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmagman jmagman jmagman approved these changes

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Missing dSYM and bcsymbolmap in xcframework from flutter build ios-framework

5 participants

@HelloCore @linhzz999 @fluttergithubbot @itlwy @jmagman