Page MenuHomePhabricator

Enable Common Cpp sources compilation and usage in NotificationService
ClosedPublic

Authored by marcin on Mar 8 2022, 6:33 AM.
Tags
None
Referenced Files
F3521584: D3370.diff
Mon, Dec 23, 4:03 AM
Unknown Object (File)
Thu, Dec 19, 6:01 PM
Unknown Object (File)
Wed, Dec 18, 10:55 AM
Unknown Object (File)
Tue, Dec 17, 6:22 PM
Unknown Object (File)
Tue, Dec 17, 6:22 PM
Unknown Object (File)
Tue, Dec 17, 6:22 PM
Unknown Object (File)
Tue, Dec 17, 6:22 PM
Unknown Object (File)
Tue, Dec 17, 6:22 PM

Details

Summary

NotificationService is an Application Extension and hence a separate XCode target so it needs its own Compile Sources definition. Moreover using UI - related API is not allow in application extension. Therefore in order to use some expo libraries (parts of which use UI API, but not the ones we use in NotificationService) I needed to programatically disable APPLICATION_EXTENSION_API_ONLY flag in Podfile.

Test Plan

Firstly succesfully compile iOS app for both release and debug version. Then place calls to methods from CommonCpp (SQLiteQueryExecutor in particular) inside NotificationService callback and send push notification to running app instance on physical device. Check whether CommonCpp is working inside NotificationService callbacks as expected.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change NotificationService target so that it uses as little react as possible to compile and run EXSecureStore.

ashoat requested changes to this revision.Mar 14 2022, 9:00 PM

Podfile changes look clean!

native/ios/Comm.xcodeproj/project.pbxproj
1113 ↗(On Diff #10140)

Why is that? Does it get set to NO for some other target? Does it matter that it is being set to YES for Pods-NotificationService.debug.xcconfig here?

native/ios/Podfile
61 ↗(On Diff #10140)

I don't think this was addressed

This revision now requires changes to proceed.Mar 14 2022, 9:00 PM
native/ios/Comm.xcodeproj/project.pbxproj
1113 ↗(On Diff #10140)

Answering 1): I did not set this flag to NO for any target in XCode. When I attempted to do so for NotificationService I could not compile this target. Only after setting it in Podfile inside post_install call I was able to compile Notification Service with expo dependencies.
Answering 2): My local Pods-NotificationService.debug.xcconfig doesn't contain this flag.

native/ios/Podfile
61 ↗(On Diff #10140)

Sorry, my mistake. I should remove this unnecessary space shortly.

native/ios/Comm.xcodeproj/project.pbxproj
1113 ↗(On Diff #10140)

I don't think either of my questions has been answered in detail.

My first question: does this flag get set to NO for some other target? To answer this, you will have to go through every single related target (the pods for the NotificationService across debug/release, and the actual NotificationService itself) and figure out what the flag is set to, and then share that information here.

My second question: you need to determine IF IT MATTERS that we are setting this to NO. What does it mean to set this to YES vs. NO? How does it work if one target with YES includes one with NO, or the other way around?

Your answer will probably need to be multiple paragraphs long, with links to documentation.

APPLICATION_EXTENSION_API_ONLY is set to:

  • YES every time it appears in native/ios/Comm.xcodeproj/project.pbxproj, which is for Pods-NotificationService.debug.xcconfig and Pods-NotificationService.release.xcconfig
  • NO every time it appears in native/ios/Pods/Pods.xcodeproj/project.pbxproj, which is for all pods both .debug.xcconfig and .release.xcconfig

I found the following article that explains why this flag has to be explicitly set to YES for NotificationService: https://swift-cast.com/2021/04/38/.
I have also found the following github issue: https://github.com/CocoaPods/CocoaPods/issues/9233.
After reading it I came to a conclusion that if I explicitly set APPLICATION_EXTENSION_API_ONLY flag to YES for Notification service then it is automatically applied to Pods included in this target. Then compilation fails since UI-related API is used in Pods we want to compile in NotificationService. Setting this flag to NO for all Pods in post_install hook in Podfile is a way to overwrite this compiler flag inheritance.
Moreover, I have found an iOS library which official installation guide directly advises users to set this flag to NO inside a Podfile to overcome the same exact issue we are trying to solve in this diff: https://cocoapods.org/pods/CrowdinSDK#installation.
Finally, if Apple reviewers for some reason disapprove of using this flag to compile expo and react native staff for Notification Service (App Extension) I have created an alternative solution that is more complex but does not utilise this flag and allows us to compile CommonCpp sources for Notification service without need to compile libraries that use UI-related API. It can be seen on the following branch: https://github.com/CommE2E/comm/tree/marcin/eng-490-expo-patching
@ashoat please let me know if this explanation is sufficient.

Thanks for the detailed answer, @marcinwasowicz!

Unify architecture settings for Comm and NotificationService targets in XCode.

Latest changes fixed the problem with building on M1

This revision is now accepted and ready to land.Mar 22 2022, 2:57 AM
ashoat requested changes to this revision.Mar 22 2022, 8:25 AM
ashoat added inline comments.
native/ios/Podfile
56 ↗(On Diff #10589)

Does this apply to all targets now? Are we sure we want to apply it to all targets? Is it possibly that we're inadvertedly applying it to some targets we haven't considered? (Targets that aren't the Comm app, or the notification service extension)

63 ↗(On Diff #10589)

It's a bit confusing to me that we exclude both of these here, but in the project.pbxproj only arm64 is mentioned. Do you know why that is?

This revision now requires changes to proceed.Mar 22 2022, 8:25 AM
native/ios/Podfile
56 ↗(On Diff #10589)

There are two changes here compared to master branch:

  1. post_install hook is moved from target 'Comm' scope to global Podfile scope
  2. some lines are added that disable APPLICATION_EXTENSION_API_ONLY flag to enable CommonCpp sources compilation in 'NotificationService' target.

Let me explain why those changes should not raise concern.
Ad. 1) According to this doc: https://www.rubydoc.info/github/CocoaPods/Core/Pod%2FPodfile%2FDSL:post_install post_install is a hook applied just before XCode project is written to disk. According to this doc: https://www.rubydoc.info/gems/cocoapods-core/0.34.1/Pod/Podfile/DSL Podfile hooks always exist in global scope. Even if this post_install hook was written inside target 'Comm' scope its behaviour would be the same as if it was in global scope. This is even more confirmed by the fact that an attempt to write multiple post_install hooks (e.g. each per target) leads to errors: https://github.com/CocoaPods/CocoaPods/issues/6172.
Ad. 2) Now that we established that we cannot have multiple post_install hooks and the one that we have should be in global scope we can admit it is impossible to use this flag anywhere else.

63 ↗(On Diff #10589)

Build settings in the XCode project refer to targets: Comm and NotificationService. This line changes build settings for pods. I did not modify this line and according to the explanation above it has alway been that we exclude both architectures for all pods. I do not know why it is that, but it is not related to any change introduced in this diff. Target 'Comm' excluded only amr64 architecture before I started to work on this issue and the very recent changes I introduced were to unify excluded architectures for both Comm and NotificationService. Would you like me to try whether we can exclude i386 arch for XCode targets as well?

ashoat requested changes to this revision.Mar 23 2022, 7:12 AM

Leaving this on your queue until we can confirm that Apple will approve APPLICATION_EXTENSION_API_ONLY

native/ios/Podfile
56 ↗(On Diff #10589)

Thanks for the explanation!

63 ↗(On Diff #10589)

No, that's alright – thanks for this explanation

74 ↗(On Diff #10589)

There's an extraneous space at the end of this line

This revision now requires changes to proceed.Mar 23 2022, 7:12 AM

Rebase to keep up to date

Hit plan changes to keep it out of reviewers queue.

Hit plan changes to remove it out of reviewers queue.

This revision is now accepted and ready to land.May 16 2022, 1:01 PM