Skip to content

Conversation

paulb777
Copy link
Member

@paulb777 paulb777 commented Feb 2, 2020

Continue xcframework started in #4737 and the rw-zip-xcframeworks branch

I've verified the xcframeworks comprised of static framework builds works for the Storage quickstart for iOS and Catalyst.

Future steps:

  • More substantial testing
  • Update path processing for pods with resources
  • Resolve remaining dynamic issues disucssed in Move to xcframework bundles #4737 - perhaps related to static_framework Pods?

I need to work on some other tasks after this PR lands - so others are welcome to pick up the work on the xcframework-master branch.

#no-changelog

@paulb777 paulb777 added the zip-builder Tools related to building the zip file. label Feb 2, 2020
@paulb777 paulb777 requested a review from ryanwilson February 3, 2020 15:57
Copy link
Member

@ryanwilson ryanwilson left a comment

Choose a reason for hiding this comment

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

Mostly LGTM just a couple things!

@@ -40,7 +40,7 @@ enum CocoaPodUtils {
}

/// Information associated with an installed pod.
struct PodInfo {
class PodInfo {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this moving to a class instead of struct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment.

Comment on lines 47 to 51
case .simulator:
return base
case .device:
// No extra arguments are required for simulator or device builds.
return base
Copy link
Member

Choose a reason for hiding this comment

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

This should be able to be combined I believe:

case .device, .simulator:
  // No extra arguments are required for device or simulator builds.
  return base

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

private func realFrameworkName(_ framework: String) -> String {
/// The dynamic framework name is different from the pod name when the module_name
/// specifier is used in the podspec.
// TODO: Automatically get the right name.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think the TODO here breaks the formatting in Xcode, do you mind moving it above the TODO above the docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

fatalError("Could not create framework directory while building framework \(framework). " +
"\(error)")
}
// Verify Firebase headers include an explicit umbrella header for Firebase.h.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: s/Firebase headers/Firebase frameworks/

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

/// Packages an XCFramework based on an almost complete framework folder (missing the binary but includes everything else needed)
/// and thin archives for each architecture slice.
/// - Parameter fromFolder: The almost complete framework folder. Includes everything but the binary.
/// - Parameter thinArchives: All the thin archives.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: missing parameters in docs here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Here and other places.

if !fileManager.directoryExists(at: xcframeworksDir) {
do {
try fileManager.createDirectory(at: xcframeworksDir,
withIntermediateDirectories: true)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this can likely fit on the line above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. 101 characters.

@@ -18,18 +18,16 @@ import Foundation

/// A struct to build a .modulemap in a given framework directory.
///
struct ModuleMapBuilder {
class ModuleMapBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

Same question - what's the reason for making this a class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually not needed. Only the other one needs to be a class.

Copy link
Member

@ryanwilson ryanwilson left a comment

Choose a reason for hiding this comment

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

Thanks!

@paulb777 paulb777 merged commit 30c1f44 into xcframework-master Feb 4, 2020
@paulb777 paulb777 deleted the pb-xcf-merge branch February 4, 2020 15:29
@firebase firebase locked and limited conversation to collaborators Mar 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes zip-builder Tools related to building the zip file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants