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

[Infra] Only compile core extension headers once #13993

Merged
merged 3 commits into from
Oct 29, 2024
Merged

[Infra] Only compile core extension headers once #13993

merged 3 commits into from
Oct 29, 2024

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Oct 29, 2024

A custom module map was too difficult. I couldn't get CocoaPods to respect the custom module map and stopped when I started to spin on things. I switched to guarding the headers against being included twice. It seems promising so far. I also tried with using #pragma once but that didn't work because (my guess) is that the problem is not that the same header is being included twice, it's that two different headers with the same content are both being included (once in FirebaseCore and once in FirebaseCoreExtension).

Test cases and their results below:

  1. Below Podfile uses the current state and fails when building docs.
platform :ios, '13.0'

source 'https://github.com/firebase/SpecsDev.git'
source 'https://github.com/firebase/SpecsStaging.git'
source 'https://cdn.cocoapods.org/'

target 'tempDoc' do
  use_frameworks!

  # Pods for tempDoc
  pod 'FirebaseCrashlytics', '11.5.0'
end
  1. Below Podfile uses updated FirebaseCore spec but FirebaseCoreExtension @ main. The framework target builds but the docs target does not build because of the FirebaseCoreExtension @ main does not have guarded headers.
platform :ios, '13.0'

source 'https://github.com/firebase/SpecsDev.git'
source 'https://github.com/firebase/SpecsStaging.git'
source 'https://cdn.cocoapods.org/'

target 'tempDoc' do
  use_frameworks!

  # Pods for tempDoc
  pod 'FirebaseCore', :git => 'https://github.com/firebase/firebase-ios-sdk.git', :branch => 'nc/dup-decls' 
  pod 'FirebaseCrashlytics', '11.5.0'
end
  1. Below Podfile uses updated FirebaseCore spec AND updated FirebaseCoreExtension spec. The framework target builds AND the docs target builds.
platform :ios, '13.0'

source 'https://github.com/firebase/SpecsDev.git'
source 'https://github.com/firebase/SpecsStaging.git'
source 'https://cdn.cocoapods.org/'

target 'tempDoc' do
  use_frameworks!

  # Pods for tempDoc
  pod 'FirebaseCore', :git => 'https://github.com/firebase/firebase-ios-sdk.git', :branch => 'nc/dup-decls' 
  pod 'FirebaseCoreExtension', :git => 'https://github.com/firebase/firebase-ios-sdk.git', :branch => 'nc/dup-decls' 
  pod 'FirebaseCrashlytics', '11.5.0'
end
  1. Below Podfile uses old product pod and floats to pick up updated FirebaseCore (the floating case). The framework target builds but the docs target does not build because of using the FirebaseCoreExtension @ main.
platform :ios, '13.0'

source 'https://github.com/firebase/SpecsDev.git'
source 'https://github.com/firebase/SpecsStaging.git'
source 'https://cdn.cocoapods.org/'

target 'tempDoc' do
  use_frameworks!

  # Pods for tempDoc
  pod 'FirebaseCore', :git => 'https://github.com/firebase/firebase-ios-sdk.git', :branch => 'nc/dup-decls' 
  pod 'FirebaseCrashlytics', '11.4.0' # older pod!
end
  1. Below Podfile uses old product pod and floats to pick up updated FirebaseCore (the floating case). In this example, it floats to pick up FirebaseCoreExtension as well. The framework target builds and the docs target builds.
platform :ios, '13.0'

source 'https://github.com/firebase/SpecsDev.git'
source 'https://github.com/firebase/SpecsStaging.git'
source 'https://cdn.cocoapods.org/'

target 'tempDoc' do
  use_frameworks!

  # Pods for tempDoc
  pod 'FirebaseCore', :git => 'https://github.com/firebase/firebase-ios-sdk.git', :branch => 'nc/dup-decls' 
  pod 'FirebaseCoreExtension', :git => 'https://github.com/firebase/firebase-ios-sdk.git', :branch => 'nc/dup-decls' 
  pod 'FirebaseCrashlytics', '11.4.0' # older pod!
end

@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@paulb777
Copy link
Member

Are the podspecs published at /Users/nickcooke/Developer/firebase-3 so they show up in the Pods folder instead of the Development Pods folder?

@ncooke3
Copy link
Member Author

ncooke3 commented Oct 29, 2024

Are the podspecs published at /Users/nickcooke/Developer/firebase-3 so they show up in the Pods folder instead of the Development Pods folder?

No, is there a way I can do that locally? Or should I stage these to SpecsDev?

@paulb777
Copy link
Member

I'm not aware of how to do it locally. Staging to SpecsDev would be a good way to test

@ncooke3
Copy link
Member Author

ncooke3 commented Oct 29, 2024

Hmm, I actually don't think SpecsDev is that helpful because the spec points the same source that staging does.

@paulb777
Copy link
Member

Should be fine to move the tags to the tip of your branch temporarily and use SpecsStaging

@paulb777
Copy link
Member

Or CocoaPods-11.5.0 should be sufficient

@ncooke3
Copy link
Member Author

ncooke3 commented Oct 29, 2024

I actually went with this:

 pod 'FirebaseCore', :git => 'https://github.com/firebase/firebase-ios-sdk.git', :branch => 'nc/dup-decls'
 pod 'FirebaseCoreExtension', :git => 'https://github.com/firebase/firebase-ios-sdk.git', :branch => 'nc/dup-decls'
 pod 'FirebaseCrashlytics', '11.4.0'
Screenshot 2024-10-29 at 6 35 19 PM

I updated the PR description and the results were the same.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

LGTM

Gemfile Outdated Show resolved Hide resolved
@ncooke3 ncooke3 marked this pull request as ready for review October 29, 2024 22:45
@ncooke3 ncooke3 merged commit 00674c8 into main Oct 29, 2024
5 checks passed
@ncooke3 ncooke3 deleted the nc/dup-decls branch October 29, 2024 22:46
@ncooke3
Copy link
Member Author

ncooke3 commented Oct 29, 2024

Merged and moving tags to get coverage in the nightlies.

@paulb777
Copy link
Member

Confirmed the repro in #13756 now successfully builds docs when pointing at SpecsStaging.

@andrewheard
Copy link
Contributor

Nice solution, @ncooke3!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants