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

Declare framework dependencies in podspec #773

Merged
merged 1 commit into from
Aug 24, 2018

Conversation

jgavris
Copy link
Contributor

@jgavris jgavris commented Aug 21, 2018

Users may want to build the pod with Clang modules disabled using -fno-modules / CLANG_ENABLE_MODULES=NO because they are using ccache which doesn't support -fmodules. When modules are disabled, auto-linking of framework modules is also disabled, so these dependencies need to be expressed explicitly.

What does this PR do?

Declares all the frameworks the pod depends on explicitly in the podspec, so that users may disable 'Clang Modules' in order to build the Objective-C code using ccache for faster builds. When CLANG_ENABLE_MODULES is off, automatic framework linking is also implicitly turned off, so without this declaration the pod no longer links.

Where should the reviewer start?

Review that all frameworks listed are used in the pod.

How should this be manually tested?

Perform bundle exec pod install of this podspec and ensure the pod still builds in the target.

Questions:

  • Does the docs need an update?

No.

  • Are there any security concerns?

No, these are all Apple provided iOS frameworks.

  • Do we need to update engineering / success?

No.

@segmentio/gateway

Users may want to build the pod with Clang modules disabled -fno-modules because they are using ccache which doesn't support -fmodules. When modules are disabled, auto-linking of framework modules is also disabled, so these dependencies need to be expressed explicitly.
@codecov-io
Copy link

Codecov Report

Merging #773 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #773      +/-   ##
==========================================
+ Coverage   78.66%   78.73%   +0.06%     
==========================================
  Files          39       39              
  Lines        1467     1467              
==========================================
+ Hits         1154     1155       +1     
+ Misses        313      312       -1

@f2prateek
Copy link
Contributor

@jgavris thanks! I had a question - how did you figure out which frameworks to add? Was it manual, or can we use a tool to do this?

I'll let @fathyb review some of the details as well, but

@jgavris
Copy link
Contributor Author

jgavris commented Aug 21, 2018

I actually just iteratively compiled the project with CLANG_ENABLE_MODULES=NO in the pod target and looked at which frameworks provided the missing symbols.

You could also do something like

λ grep -hr "import <" . | sort -n | uniq
grep: ./.clang-format: No such file or directory
#import <AdSupport/ASIdentifierManager.h>
#import <Analytics/Analytics.h>
#import <Analytics/NSData+SEGGZIP.h>
#import <Analytics/SEGAES256Crypto.h>
#import <Analytics/SEGAnalytics.h>
#import <Analytics/SEGAnalyticsUtils.h>
#import <Analytics/SEGFileStorage.h>
#import <Analytics/SEGIntegrationsManager.h>
#import <Analytics/SEGSerializableValue.h>
#import <Analytics/SEGStoreKitTracker.h>
#import <Analytics/SEGUserDefaultsStorage.h>
#import <Analytics/UIViewController+SEGScreen.h>
#import <CommonCrypto/CommonCryptor.h>
#import <CommonCrypto/CommonKeyDerivation.h>
#import <CoreTelephony/CTCarrier.h>
#import <CoreTelephony/CTTelephonyNetworkInfo.h>
#import <Foundation/Foundation.h>
#import <StoreKit/StoreKit.h>
#import <SystemConfiguration/SystemConfiguration.h>
#import <UIKit/UIKit.h>
#import <arpa/inet.h>
#import <dlfcn.h>
#import <ifaddrs.h>
#import <netdb.h>
#import <netinet/in.h>
#import <netinet6/in6.h>
#import <objc/runtime.h>
#import <sys/socket.h>
#import <zlib.h>

@f2prateek
Copy link
Contributor

Thanks - this makes it much easier to verify if we missed something.

Copy link
Contributor

@fathyb fathyb left a comment

Choose a reason for hiding this comment

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

LGTM 👍
I added a branch on test-apps to test this https://github.com/segmentio/analytics-test-apps/tree/test/no-auto-link

@fathyb fathyb merged commit 2f2d35b into segmentio:master Aug 24, 2018
@jgavris jgavris deleted the podspec-dependencies branch August 29, 2018 20:31
@f2prateek f2prateek mentioned this pull request Oct 3, 2018
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.

4 participants