Fix Resource generation for Carthage xcframeworks #8182

paulb777 · 2021-06-03T00:12:51Z

Carthage resources should remain in each .framework instead of at the root of .xcframeworks.

After all xcframework resources are processed and stored at the root of the .xcframework at https://github.com/firebase/firebase-ios-sdk/blob/master/ReleaseTooling/Sources/ZipBuilder/FrameworkBuilder.swift#L697, the new code consolidates both Zip Distro and Carthage resource processing in ZipBuilder.swift.

It still remains for the future to handle platform-specific Resources.

Successful test at https://github.com/firebase/firebase-ios-sdk/actions/runs/903575515.
I verified the Zip in the artifact matches the 8.1.0 release. The only exception is GoogleSignIn Resources are now in the framework. See comments for more discussion about GoogleSignIn. I also verified that the Firestore, GoogleSignIn, and FIAM Carthage zips successfully install on carthage update including the xcframeworks that were not installing because of the Resources location.

google-oss-bot · 2021-06-03T00:13:43Z

	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 · 2021-06-03T00:13:58Z

ReleaseTooling/Sources/ZipBuilder/ZipBuilder.swift

        // Update the README.
        readmeDeps += dependencyString(for: pod.key, in: productDir, frameworks: podFrameworks)
-
-        // Special case for Crashlytics:


The Crashlytics code is only reindented and shows up in green below.

paulb777 · 2021-06-03T00:14:44Z

ReleaseTooling/Sources/ZipBuilder/FirebaseBuilder.swift

@@ -46,24 +46,6 @@ struct FirebaseBuilder {
        options: carthageBuildOptions
      )

-      // Prepare the release directory for zip packaging.


This chunk is migrated to the if packageKind == "Firebase" { code below.

paulb777 · 2021-06-03T00:15:43Z

ReleaseTooling/Sources/ZipBuilder/ZipBuilder.swift

+            // Move all the bundles in the frameworks out to a common "Resources" directory to
+            // match the existing Zip structure.
+            let resourcesDir = productPath.appendingPathComponent("Resources")
+            try fileManager.moveItem(at: xcResourceDir, to: resourcesDir)


moveItem instead of ResourcesManager.moveAllBundles since the resources were already processed when the xcframework was created.

ryanwilson

Thanks for the comments, they helped a bunch!

ryanwilson · 2021-06-03T17:49:21Z

ReleaseTooling/Sources/ZipBuilder/ZipBuilder.swift

+        // TODO: Investigate changing the zip distro to also have Resources in the .frameworks to
+        // enable different platform Resources.


I thought there was an issue in having them embedded in the frameworks but I can't remember, maybe it was an older Xcode version and latest versions would be fine.

We might have had another issue in the past, like an invalid Info.plist. My testing shows it to be fine now.

ryanwilson · 2021-06-03T17:49:43Z

ReleaseTooling/Sources/ZipBuilder/ZipBuilder.swift

+            // match the existing Zip structure.
+            let resourcesDir = productPath.appendingPathComponent("Resources")
+            try fileManager.moveItem(at: xcResourceDir, to: resourcesDir)
+


Nit: extra blank line.

ryanwilson · 2021-06-03T17:52:32Z

ReleaseTooling/Sources/ZipBuilder/ZipBuilder.swift

+              guard fileManager.isDirectory(at: platformPath)
+              else { continue }


This could probably be on a single line

Addressing in next commit

paulb777

Tests look good. Will fix spaces review commit and summarize testing in PR intro, and then merge

paulb777 · 2021-06-03T21:02:10Z

ReleaseTooling/Sources/ZipBuilder/ZipBuilder.swift

+        // TODO: Investigate changing the zip distro to also have Resources in the .frameworks to
+        // enable different platform Resources.


We might have had another issue in the past, like an invalid Info.plist. My testing shows it to be fine now.

paulb777 · 2021-06-03T21:02:30Z

ReleaseTooling/Sources/ZipBuilder/ZipBuilder.swift

+              guard fileManager.isDirectory(at: platformPath)
+              else { continue }


Addressing in next commit

paulb777 · 2021-06-03T21:19:07Z

There is an issue with blaze created frameworks 😞

~/test-8082/Firebase-actions-dir/carthage/8.1.0/GoogleSignIn  $ swift
Welcome to Apple Swift version 5.4 (swiftlang-1205.0.26.9 clang-1205.0.19.55).
Type :help for assistance.
  1> import Foundation
  2> Bundle(path: "GoogleSignIn.framework")!.infoDictionary
$R0: [String : Any]? = 0 key/value pairs

The one diff in the generated zip is the GoogleSignIn resources aren't being moved out of the framework any longer. I'll restore so that the zip is unchanged.

paulb777 · 2021-06-03T22:49:01Z

On additional investigation, the Carthage GoogleSignIn.framework is the same structure it has always been. It turns out that Carthage needs to be able to process Info.plist's for .xcframeworks , but not for .frameworks. Given that GoogleSignIn will soon move to .xcframeworks and issues like #8154, I'll merge as is.

google-cla bot added the cla: yes label Jun 3, 2021

google-oss-bot added the release-tooling label Jun 3, 2021

paulb777 commented Jun 3, 2021

View reviewed changes

paulb777 mentioned this pull request Jun 3, 2021

Nightly Testing Report #8175

Closed

paulb777 added 5 commits June 3, 2021 09:40

Fix Resource generation for Carthage xcframeworks

6fc928d

head doesn't build because of an AppCheck Catalyst build issue

d93a4f8

Save Carthage artifacts

7a1dd82

Restore zip.yml after fix from #8184

f712fd6

Restore build_zip.sh

54bab2b

paulb777 force-pushed the pb-fix-carthage-resources branch from 43f5cd2 to 54bab2b Compare June 3, 2021 16:43

ryanwilson approved these changes Jun 3, 2021

View reviewed changes

paulb777 commented Jun 3, 2021

View reviewed changes

review

21fde3e

paulb777 merged commit 2fe8a25 into master Jun 3, 2021

paulb777 deleted the pb-fix-carthage-resources branch June 3, 2021 22:51

paulb777 mentioned this pull request Jun 11, 2021

Only five of six xcframeworks get installed from the specified zip Carthage/Carthage#3185

Closed

firebase locked and limited conversation to collaborators Jul 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix Resource generation for Carthage xcframeworks #8182

Fix Resource generation for Carthage xcframeworks #8182

Uh oh!

paulb777 commented Jun 3, 2021 •

edited

Loading

Uh oh!

google-oss-bot commented Jun 3, 2021

Uh oh!

paulb777 Jun 3, 2021

Uh oh!

paulb777 Jun 3, 2021

Uh oh!

paulb777 Jun 3, 2021

Uh oh!

ryanwilson left a comment

Uh oh!

ryanwilson Jun 3, 2021

Uh oh!

paulb777 Jun 3, 2021

Uh oh!

ryanwilson Jun 3, 2021

Uh oh!

ryanwilson Jun 3, 2021

Uh oh!

paulb777 Jun 3, 2021

Uh oh!

paulb777 left a comment

Uh oh!

paulb777 Jun 3, 2021

Uh oh!

paulb777 Jun 3, 2021

Uh oh!

paulb777 commented Jun 3, 2021

Uh oh!

paulb777 commented Jun 3, 2021

Uh oh!

Uh oh!

		// TODO: Investigate changing the zip distro to also have Resources in the .frameworks to
		// enable different platform Resources.

		guard fileManager.isDirectory(at: platformPath)
		else { continue }

Fix Resource generation for Carthage xcframeworks #8182

Fix Resource generation for Carthage xcframeworks #8182

Uh oh!

Conversation

paulb777 commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Jun 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulb777 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulb777 commented Jun 3, 2021

Uh oh!

paulb777 commented Jun 3, 2021

Uh oh!

Uh oh!

paulb777 commented Jun 3, 2021 •

edited

Loading