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

RUMM-1200 Remove Carthage workaround for missing arm64 slice in PLCR's xcframework #618

Merged

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Sep 30, 2021

What and why?

📦 This PR removes workaround added in early development of DatadogCrashReporting when PLCR's xcframework (1.8.0) was missing the arm64-simulator slice.

The recent version of PLCR's xcframework (1.10.0) includes the missing slice, so this workaround is not needed anymore. It also means that now all targets in Datadog.xcworkspace can be compiled on Apple Silicon.

How?

Made necessary updates in xcconfigs.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@ncreated ncreated self-assigned this Sep 30, 2021
Comment on lines +23 to 25
// To build only active architecture for all configurations. This gives us ~10% build time gain\n
// in targets which do not use 'Debug' configuration.\n
ONLY_ACTIVE_ARCH = YES\n
Copy link
Member Author

Choose a reason for hiding this comment

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

I did this measurement in DatadogIntegrationTests (it doesn't use Debug configuration):

  • with ONLY_ACTIVE_ARCH = YES: 65s, 65s, 75s
  • without ONLY_ACTIVE_ARCH = YES: 72s, 81s, 77s

It is justified to have higher times when building all architectures, as produced artifacts contain more slices, e.g.:

❯ lipo -info /Users/.../Release-iphonesimulator/Datadog.framework/Datadog
Architectures in the fat file: /.../Release-iphonesimulator/Datadog.framework/Datadog are: x86_64 arm64 

vs:

❯ lipo -info /Users/.../Release-iphonesimulator/Datadog.framework/Datadog
Architectures in the fat file: /.../Release-iphonesimulator/Datadog.framework/Datadog are: x86_64

Comment on lines -26 to -29
// Note: this is only problematic when `carthage update --use-xcframeworks` builds `CrashReporter.xcframework` from source
// and should not be an issue once pre-build xcframeworks support is added to Carthage: https://github.com/Carthage/Carthage/pull/3123
// However, carthage always falls back to build from source if it can't download pre-build binaries, so this adjustment
// will be still required.
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure if this is correct and/or helps with anything. Even if falling back to build from source, as we were only allowing ARCHS[sdk=iphonesimulator*] = x86_64 in this patch, that would fail to compile for simulator on Apple Silicon. I think the strategy of not overriding ARCHS in such case is safer. As we don't need this patch anymore for our own development, I remove it entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I only reffer to the note on posibility to remove this patch. The patch was valid, but we don't need it anymore.

@ncreated ncreated force-pushed the ncreated/RUMM-1200-remove-workaround-for-carthage-build branch from 09581c4 to 0b21ab4 Compare September 30, 2021 09:47
@ncreated ncreated marked this pull request as ready for review September 30, 2021 09:48
@ncreated ncreated requested a review from a team as a code owner September 30, 2021 09:48
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

🧹

@ncreated ncreated merged commit 2892a3e into master Sep 30, 2021
@ncreated ncreated deleted the ncreated/RUMM-1200-remove-workaround-for-carthage-build branch September 30, 2021 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants