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

Update Podspec to the new project structure #465

Merged
merged 14 commits into from
May 4, 2024

Conversation

GLinnik21
Copy link
Collaborator

No description provided.

# Conflicts:
#	KSCrash.podspec
# Conflicts:
#	KSCrash.podspec
KSCrash.podspec Outdated Show resolved Hide resolved
@GLinnik21
Copy link
Collaborator Author

This is a subspecs approach that mimics the SPM structure. The main problem with this is that only individual podspecs can be modules, so we don't get the modular separation of SPM. With subspecs, everything ends up being a KSCrash.framework module with all the headers flattened together. A custom module map might be the solution, or podspecs for each module like Firebase does.

@GLinnik21
Copy link
Collaborator Author

The issue with the .modulemap file arises when trying to define modules for both .a libraries and .framework libraries. Here’s how it usually looks:

module KSCrashCore {
    header "KSSystemCapabilities.h"
    header "NSError+SimpleConstructor.h"
    export *
}

module KSCrashFilters {
    header "KSCrashReportFilterAlert.h"
    header "KSCrashReportFilterAppleFmt.h"
    header "KSCrashReportFilterBasic.h"
    header "KSCrashReportFilterGZip.h"
    header "KSCrashReportFilterJSON.h"
    header "KSCrashReportFilterSets.h"
    header "KSCrashReportFilterStringify.h"
    export *
}

This setup works fine for .a libraries with use_modular_headers!. However, issues arise with .framework libraries as they require a framework declaration, leading to warnings like:

    - WARN  | [KSCrash/RecordingCore] xcodebuild:  /<user_path>/DerivedData/App-borbxcvqwzuqknepndaqdvyfloem/Build/Intermediates.noindex/Pods.build/Release-iphonesimulator/KSCrash.build/module.modulemap:44:12: warning: skipping 'KSDebug.h' because module declaration of 'KSCrashRecordingCore' lacks the 'framework' qualifier [-Wincomplete-framework-module-declaration]

Even if you specify modules with a framework tag, .a libraries typically work fine since CocoaPods automatically removes the framework specifier. However, this gets complicated with .frameworks, where things like "submodules" are necessary but restrictive due to the required folder structure and limitations in having multiple parallel framework modules. According to the official Clang documentation on module and submodule declarations, you can't just set up multiple parallel framework modules. If you use submodules, they require a specific folder structure which adds another layer of complexity.

So, we're left with these options:

  1. Leave as is: Users will simply import KSCrash, and the module's contents will adjust based on the specified subspec. This approach leaves all "private" API modules exposed to the library users, which could be a downside if encapsulation is a concern.

  2. Create separate podspecs for each module: This is the only straightforward method fully supported by CocoaPods for handling Clang/Swift modules. By defining separate podspecs, each module can be independently managed and integrated, although this approach increases maintenance overhead and complexity in project management.

@GLinnik21 GLinnik21 marked this pull request as ready for review May 2, 2024 21:38
.github/workflows/cocoapods-lint.yml Show resolved Hide resolved
KSCrash.podspec Outdated Show resolved Hide resolved
KSCrash.podspec Show resolved Hide resolved
KSCrash.podspec Show resolved Hide resolved
@@ -16,123 +16,54 @@ Pod::Spec.new do |s|
s.xcconfig = { 'GCC_ENABLE_CPP_EXCEPTIONS' => 'YES' }
s.default_subspecs = 'Installations'

s.subspec 'Recording' do |recording|
recording.compiler_flags = '-fno-optimize-sibling-calls'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this -fno-optimize-sibling-calls flag removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess we can set it in CocoaPods, but in SPM it can only be set with .unsafeFlags, which is not allowed in "release" (specified by version), but is allowed in development (local package or specified by commit or branch).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's also unclear which functions this flag is for, so we can't know if it's needed in Recording or in RecordingCore.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, you've added it here #379 :)
Looks like it's actually needed. But maybe we can take a solution from rollbar/rollbar-apple#335 (if I understand that fix correctly).

Copy link
Collaborator

@bamx23 bamx23 left a comment

Choose a reason for hiding this comment

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

All looks great! Let's just make sure that TCO is addressed later.

@bamx23 bamx23 merged commit 7bc2fc9 into kstenerud:release-2.0 May 4, 2024
@GLinnik21 GLinnik21 linked an issue May 4, 2024 that may be closed by this pull request
@GLinnik21 GLinnik21 removed a link to an issue May 24, 2024
@GLinnik21 GLinnik21 added this to the 2.0.0 milestone Sep 20, 2024
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.

2 participants