Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Fix bad dSYM for symbol-stripped dynamic framework builds #6531

Merged
merged 6 commits into from
Sep 30, 2016
Merged

Fix bad dSYM for symbol-stripped dynamic framework builds #6531

merged 6 commits into from
Sep 30, 2016

Conversation

friedbunny
Copy link
Contributor

@friedbunny friedbunny commented Sep 30, 2016 •
edited
Loading

Fixes #6502. Stripped dynamic framework packages should now come with the correct dSYM.

Disabling GCC_GENERATE_DEBUGGING_SYMBOLS for SYMBOLS=NO builds meant that those builds had no debug symbols to strip or add to a dSYM. Running make iframework SYMBOLS=NO would fail to create a dSYM, but our deploy script would evidently reuse the symbol-ful dSYM.

With the added UUID validation, the above scenario would now fail:

* Stripping symbols from binaries…
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/strip: changes being made to the file will invalidate the code signature in: /Users/jason/Documents/Mapbox/mapbox-gl-native/build/ios/pkg/dynamic/Mapbox.framework/Mapbox (for architecture i386)
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/strip: changes being made to the file will invalidate the code signature in: /Users/jason/Documents/Mapbox/mapbox-gl-native/build/ios/pkg/dynamic/Mapbox.framework/Mapbox (for architecture x86_64)
* Validating dSYM and framework UUIDs
error: unable to open 'build/ios/pkg/dynamic/Mapbox.framework.dSYM/Contents/Resources/DWARF/Mapbox': No such file or directory
Error: dSYM and framework UUIDs do not match.
build/ios/pkg/dynamic/Mapbox.framework.dSYM/Contents/Resources/DWARF/Mapbox

build/ios/pkg/dynamic/Mapbox.framework/Mapbox
  23F302DF-17AB-384C-879B-746C8A73511A (arm64) 2877864D-E317-3250-8756-11D94C868177 (i386) 59352B56-89D5-3E8E-B83E-9B15657B5C83 (armv7) F91C2CAC-5283-3ADF-ACFB-DD86B4BF5BFF (x86_64)
make: *** [iframework] Error 1

Note: GCC_GENERATE_DEBUGGING_SYMBOLS=YES is the default setting.

Useful resources

/cc @boundsj @1ec5

@friedbunny friedbunny added bug iOS Mapbox Maps SDK for iOS build macOS Mapbox Maps SDK for macOS labels Sep 30, 2016
@friedbunny friedbunny added this to the ios-v3.4.0 milestone Sep 30, 2016
@friedbunny friedbunny self-assigned this Sep 30, 2016
@mention-bot
Copy link

@friedbunny, thanks for your PR! By analyzing this pull request, we identified @1ec5, @incanus and @boundsj to be potential reviewers.

@friedbunny
[ios][build] Generate debug symbols for all builds
30d07f9 
Fixes the issue where our stripped dynamic build did not have a valid dSYM.

Disabling GCC_GENERATE_DEBUGGING_SYMBOLS for SYMBOLS=NO builds meant
that those builds had no debug symbols to strip or add to a dSYM.
@friedbunny friedbunny force-pushed the fb-dsyms-for-everyone branch from 9a898db to 5ce9ab1 Compare September 30, 2016 09:22
@friedbunny
Copy link
Contributor Author

friedbunny commented Sep 30, 2016 •
edited
Loading

One other thing: the strip command was never being invoked because of bash boolean comparison problems. Our symbol “stripping” was coming entirely from the fact that we weren’t generating debug symbols.

Now that we are invoking strip -Sx, we’re also stripping non-global symbols. So far, this looks like it reduces the raw binary size of Mapbox.framework/Mapbox by 20% — v3.4.0-alpha.4 was 45.3MB, this PR is 35.1MB.

The dSYM size is also much smaller: 234MB vs 32MB. This worries me and I’m not sure why it’s happened, but I was able to successfully symbolicate crashes with this smaller dSYM...

1ec5
1ec5 approved these changes Sep 30, 2016
View reviewed changes
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Great catches all around, and validating the dSYM as part of the build process is a great idea. Would be nice if we can extend validation to macOS, but if that’s nontrivial, it wouldn’t block this PR.

platform/macos/scripts/package.sh
@@ -43,7 +42,7 @@ if [[ -e ${PRODUCTS}/${BUILDTYPE}/${NAME}.framework.dSYM ]]; then
cp -r ${PRODUCTS}/${BUILDTYPE}/${NAME}.framework.dSYM "${OUTPUT}"
fi

if [[ "${GCC_GENERATE_DEBUGGING_SYMBOLS}" == false ]]; then
if [[ ${SYMBOLS} = NO ]]; then
step "Stripping binaries…"
strip -Sx "${OUTPUT}/${NAME}.framework/${NAME}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to do dSYM validation for macOS as well? I think it might’ve caught #5818 indirectly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be as easy as copying it over, I think. Will give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did this in 6723a61.

@1ec5 1ec5 mentioned this pull request Sep 30, 2016
Closed
@friedbunny friedbunny force-pushed the fb-dsyms-for-everyone branch from 5ce9ab1 to a8e15e8 Compare September 30, 2016 21:02
@friedbunny
Copy link
Contributor Author

friedbunny commented Sep 30, 2016 •
edited
Loading

Should be ready to merge.

1ec5
1ec5 approved these changes Sep 30, 2016
View reviewed changes
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

👌

@1ec5
Copy link
Contributor

1ec5 commented Sep 30, 2016

Oh, this is probably worth mentioning in the iOS changelog too.

@friedbunny friedbunny force-pushed the fb-dsyms-for-everyone branch from 672c1ce to 8971788 Compare September 30, 2016 22:48
@friedbunny friedbunny merged commit 5b28ebb into master Sep 30, 2016
@friedbunny friedbunny deleted the fb-dsyms-for-everyone branch September 30, 2016 22:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@1ec5 1ec5 1ec5 approved these changes

Assignees

@friedbunny friedbunny

Labels
bug build iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Milestone
ios-v3.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@friedbunny @mention-bot @1ec5