Skip to content

keep symbols for profile#39530

Merged
dnfield merged 2 commits intoflutter:masterfrom
dnfield:profile_symbols
Aug 30, 2019
Merged

keep symbols for profile#39530
dnfield merged 2 commits intoflutter:masterfrom
dnfield:profile_symbols

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Aug 29, 2019

Description

Adds back profile symbols for bitcode enabled builds - they do confuse things if you try to archive the app, but we don't need people to archive the app for profile anyway.

Related Issues

Unblocks #39503
Fixes #37065 for bitcode enabled builds

Tests

I added the following tests:

Added a test that verifies we strip the symbols on release - updated the test for profile so that it verifies we do not.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@dnfield dnfield requested review from jmagman and xster August 29, 2019 23:20
@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 29, 2019
@dnfield dnfield mentioned this pull request Aug 29, 2019
Merged
9 tasks
jmagman
jmagman approved these changes Aug 29, 2019
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

LGTM, let me test #39503 on top of this.

@dnfield
Copy link
Contributor Author

dnfield commented Aug 29, 2019

FWIW, they pass for me locally with this change.

@jmagman
Copy link
Member

jmagman commented Aug 29, 2019

FWIW, they pass for me locally with this change.

Task result:
{
  "success": true,
  "data": null,
  "benchmarkScoreKeys": []
}

Me too!

@codecov
Copy link

codecov bot commented Aug 30, 2019 •
edited
Loading

Codecov Report

Merging #39530 into master will increase coverage by 0.54%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #39530      +/-   ##
==========================================
+ Coverage   56.03%   56.57%   +0.54%     
==========================================
  Files         195      195              
  Lines       18507    18505       -2     
==========================================
+ Hits        10370    10470     +100     
+ Misses       8137     8035     -102
Flag Coverage Δ
#flutter_tool 56.57% <100%> (+0.54%) ⬆️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/base/build.dart 68.06% <100%> (ø) ⬆️
...s/flutter_tools/lib/src/test/flutter_platform.dart 0.53% <0%> (-27.88%) ⬇️
...ckages/flutter_tools/lib/src/commands/channel.dart 44.61% <0%> (-27.7%) ⬇️
packages/flutter_tools/lib/src/version.dart 88.99% <0%> (-3.83%) ⬇️
packages/flutter_tools/lib/src/cache.dart 43.5% <0%> (-0.73%) ⬇️
packages/flutter_tools/lib/src/base/logger.dart 82.06% <0%> (-0.39%) ⬇️
packages/flutter_tools/lib/src/vmservice.dart 37.46% <0%> (+0.3%) ⬆️
.../flutter_tools/lib/src/fuchsia/fuchsia_device.dart 55.39% <0%> (+0.37%) ⬆️
packages/flutter_tools/lib/src/build_info.dart 73.77% <0%> (+0.54%) ⬆️
packages/flutter_tools/lib/src/project.dart 83.7% <0%> (+0.95%) ⬆️
... and 12 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 9769754...2759c30. Read the comment docs.

@xster
Copy link
Member

xster commented Aug 30, 2019

I don't know what these stripped.S files are but conceptually SGTM

@dnfield
Copy link
Contributor Author

dnfield commented Aug 30, 2019

the .stripped.S is the assembly file with the DWARF debugging stripped out before compilation. For whatever reason just using strip on them wasn't working.

@dnfield dnfield merged commit eefe9d9 into flutter:master Aug 30, 2019
@dnfield dnfield deleted the profile_symbols branch August 30, 2019 03:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jmagman jmagman jmagman approved these changes

@xster xster Awaiting requested review from xster

Assignees

No one assigned

Labels

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.

We shouldn't strip dSYM for App.framework on profile mode

5 participants

@dnfield @jmagman @xster @googlebot @fluttergithubbot