feat: Enable DEFINES_MODULE again
#3838
Conversation
If SDWebImage is imported in a Swift library with frameworks enabled (`use_frameworks!`), the pod install process fails because it does not define a module. I need modules in my [nitro](https://github.com/mrousavy/nitro) libraries, such as [react-native-nitro-image](https://github.com/mrousavy/react-native-nitro-image) - which uses SDWebImage. To fix this, you need to set `DEFINES_MODULE` to `YES` in your pod config. This was already done 7 years ago by @zhongwuzw in SDWebImage#2549, then reverted just two months later by @dreampiggy in SDWebImage#2604 because it broke the build in some projects: - SDWebImage#2601 - CocoaPods/CocoaPods#7584 Those issue reports are 6 years old and have been closed since many years, hinting that the issue has already been fixed. I have not had any issues with modules in Swift for at least 4 years too, so I think this is safe to add now. Let's test this enough before shipping though.
WalkthroughAdded DEFINES_MODULE => 'YES' to s.pod_target_xcconfig in SDWebImage.podspec, placed after DERIVE_MACCATALYST_PRODUCT_BUNDLE_IDENTIFIER => 'NO'. No other podspec fields or public APIs were modified. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
If this can be merged as-is, we should probably also add |
|
The CI fails to build because; Seems to be unrelated. I had this issue before and re-running failed tests made it work. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
SDWebImage.podspec (2)
30-34: Optionally also flip CLANG_ENABLE_MODULES to explicit YES.
Some setups rely on this being explicit when modules are defined via xcconfig.Apply this small addition:
s.pod_target_xcconfig = { 'SUPPORTS_MACCATALYST' => 'YES', 'DERIVE_MACCATALYST_PRODUCT_BUNDLE_IDENTIFIER' => 'NO', - 'DEFINES_MODULE' => 'YES' + 'DEFINES_MODULE' => 'YES', + 'CLANG_ENABLE_MODULES' => 'YES' }
30-34: Add a tiny Swift import test_spec to lock this in (nice-to-have).
A Swift test that justimport SDWebImageprevents future regressions.Example (outside this hunk):
- podspec:
s.test_spec 'SwiftImport' do |ts| ts.requires_app_host = false ts.source_files = 'Tests/SwiftImport/Sanity.swift' ts.frameworks = 'UIKit' end
- Tests/SwiftImport/Sanity.swift:
import SDWebImage import UIKit final class Sanity { func ping(_ v: UIImageView) { _ = v.sd_imageIndicator } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
SDWebImage.podspec(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Unit Test (iOS)
- GitHub Check: Build Library
- GitHub Check: Unit Test (macOS)
- GitHub Check: Cocoapods Lint
- GitHub Check: Cocoapods Demo
🔇 Additional comments (2)
SDWebImage.podspec (2)
33-33: LGTM: Re‑enabling module definition unblocks Swiftimport SDWebImagewithuse_frameworks!.
This aligns with the PR intent and should help Nitro-based Swift libraries.
30-34: Validate Podspec across CocoaPods integration modes: CocoaPods CLI isn’t available here; locally run the lint matrix (dynamic frameworks, static frameworks, static libraries with modular headers) across iOS, macOS, tvOS, and watchOS to ensure no regressions.
|
CocoaPods supports this by Podfile syntax You don't need each Pod (podspec) to do this. |
|
Adding it in a Podfile is not a solution for me. My Pod requires it only for it's pod (and it's dependencies), and other pods in the project (React Native) cannot build with that enabled. |
|
bump @dreampiggy - what do you think about it? I don't think it has any negative side-effect, but not having it blocks me from shipping it in my library (like https://github.com/mrousavy/react-native-nitro-image) |
|
Hey @dreampiggy - sorry for tagging again, but I really think this PR should be merged. As far as I am aware, it is non-breaking, and this is the only way to avoid the user having to manually edit his |
2 participants
New Pull Request Checklist
pod lib lint)Pull Request Description
If SDWebImage is imported in a Swift library with frameworks enabled (
use_frameworks!), the pod install process fails because it does not define a module. I need modules in my nitro libraries, such as react-native-nitro-image - which uses SDWebImage.To fix this, you need to set
DEFINES_MODULEtoYESin your pod config.This was already done 7 years ago by @zhongwuzw in #2549, then reverted just two months later by @dreampiggy in #2604 because it broke the build in some projects:
Those issue reports are 6 years old and have been closed since many years, hinting that the issue has already been fixed. I have not had any issues with modules in Swift for at least 4 years too, so I think this is safe to add now. Let's test this enough before shipping though.
Summary by CodeRabbit
importusage.