Skip to content

Restore Carthage#8084

Merged
paulb777 merged 5 commits intomasterfrom
pb-restore-carthage
May 17, 2021
Merged

Restore Carthage#8084
paulb777 merged 5 commits intomasterfrom
pb-restore-carthage

Conversation

@paulb777
Copy link
Copy Markdown
Member

@paulb777 paulb777 commented May 15, 2021

Restore Carthage distribution now that Carthage 0.38.0 supports binary xcframework distributions.

This PR removes the code that was repackaging build xcframeworks as frameworks for Carthage. Now we distribute exactly the same xcframeworks in the zip distribution as Carthage with the exception of the rebuilt FirebaseCoreDiagnostics.

Successful Carthage 8.0.0 generation in https://github.com/firebase/firebase-ios-sdk/actions/runs/845683211 after generating package on GHA after disabling --local-podspec-path from build_zip.sh. More context about this in #8082.

@paulb777 paulb777 marked this pull request as draft May 15, 2021 17:49
@google-cla google-cla bot added the cla: yes label May 15, 2021
@google-oss-bot
Copy link
Copy Markdown
Collaborator

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

Generated by 🚫 Danger

// Get the framework Headers directory. On macOS, it's a symbolic link.
let headersDir = archivePath.appendingPathComponent("Headers").resolvingSymlinksInPath()

// The macOS Headers directory can have a Headers file in it symbolically linked to nowhere.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixing a zip distribution bug caught by the Carthage hash generation

@paulb777 paulb777 marked this pull request as ready for review May 16, 2021 01:29
@paulb777 paulb777 requested a review from ryanwilson May 17, 2021 14:04
Comment on lines +1 to +2
{
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm we currently don't build this in the zip, do we? I thought we had excluded it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Doesn't look like it. It's part of 8.0.0. Perhaps we only excluded for Carthage before but now that Carthage matches zip, we shouldn't?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Discussed offline, SGTM on including it.

}
return xcframework
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yessssssss so happy all this code is going away 🥳

carthageToInstall[podName] = [carthageFramework]
}

if podName == "FirebaseCoreDiagnostics" {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to also gate this on includeCarthage? Otherwise we'll be compiling CD when we don't need to. Same in the below few blocks

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Any reason, we shouldn't always build Carthage when we're building a Firebase distro. How about removing the includeCarthage flag in a subsequent PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SGTM to move in a subsequent PR and merging as is.

}
frameworks.append(zipLocation)

CarthageUtils.generatePlistContents(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this not relevant anymore?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's done in the xcframework build. Ideally generatePlistContents should be moved out of CarthageUtils. I can do that in a subsequent PR.

@ryanwilson
Copy link
Copy Markdown
Member

ryanwilson commented May 17, 2021

Thanks for getting this updated so quickly!

@paulb777 paulb777 merged commit 4f00074 into master May 17, 2021
@paulb777 paulb777 deleted the pb-restore-carthage branch May 17, 2021 19:03
@firebase firebase locked and limited conversation to collaborators Jun 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants