Introduce a xcframework dependency type #769

lakpa · 2019-12-08T19:17:56Z

Short description 📝

This PR introduces a new dependency type to support xcframework.

Solution 📦

A graph node to represent xcframework dependency called XCFrameworkNode is introduced which inherits from PrecompiledNode, the XCframeworkNode can leverage existing lookup however the only exception we need to consider is during embedding phase when we need to specifically look for xcframework file type to skip getting the xcframework being picked up as precompiled to skip embed script build phase and embed the respective file reference with required CodeSignOnCopy attribute instead.

kwridan

Looks good @lakpa 👍

some final bits:

The dependencies section in the docs will need a new entry for xcframework https://github.com/tuist/tuist/blob/master/docs/usage/dependencies.mdx
Worth adding a static library xcframework fixture too

kwridan · 2019-12-09T21:23:55Z

fixtures/ios_app_with_xcframeworks/Tests/xcframeworksTests.swift

+import Foundation
+import XCTest
+
+@testable import xcframeworks


typo: @testable import App

kwridan · 2019-12-09T21:35:18Z

Tests/TuistGeneratorTests/Generator/LinkGeneratorTests.swift

+        let pbxproj = PBXProj()
+        let pbxTarget = PBXNativeTarget(name: "Test")
+        let sourceRootPath = AbsolutePath("/")
+
+        let group = PBXGroup()
+        pbxproj.add(object: group)
+
+        let from = AbsolutePath("/Frameworks/")
+        let fileAbsolutePath = AbsolutePath("/Frameworks/Test.xcframework")
+        let fileRelativePath = RelativePath("./Test.xcframework")
+        let fileElements = ProjectFileElements()
+        fileElements.addFileElement(from: from,
+                                          fileAbsolutePath: fileAbsolutePath,
+                                          fileRelativePath: fileRelativePath,
+                                          name: nil,
+                                          toGroup: group,
+                                          pbxproj: pbxproj)
+
+        let wakaFile = fileElements.file(path: fileAbsolutePath)


I wonder if we can benefit from a test helper here to make the test more concise.

// Given let xcframeworkPath = AbsolutePath("/Frameworks/Test.xcframework") let dependencies: [DependencyReference] = [ .absolute(xcframeworkPath) ] let fileElements = createFileElements(for: xcframeworkPath) // ...

Note: "waka" was the name of a framework used in another test, there is no need to use it here as the framework we have here is called "test", the name doesn't really impact the test.

Agreed, we could make code concise by using a helper.

kwridan · 2019-12-09T21:39:16Z

Tests/TuistCoreTests/Graph/Nodes/TargetNodeTests.swift

        // Given
        let library = LibraryNode.test()
        let framework = FrameworkNode.test()
        let cocoapods = CocoaPodsNode.test()
+        let xcframework = try XCFrameworkNode.test()


Does this helper exist? is there a reason it's throwing?

Thanks for spotting, i was running some trials, it got left over after. Will remove it.

kwridan · 2019-12-09T21:46:21Z

Tests/TuistGeneratorTests/Generator/LinkGeneratorTests.swift

+        let copyBuildPhase: PBXCopyFilesBuildPhase? = pbxTarget.buildPhases.last as? PBXCopyFilesBuildPhase
+        XCTAssertEqual(copyBuildPhase?.name, "Embed Frameworks")
+        let wakaBuildFile: PBXBuildFile? = copyBuildPhase?.files?.last
+        XCTAssertEqual(wakaBuildFile?.file, wakaFile)
+        let settings: [String: [String]]? = wakaBuildFile?.settings as? [String: [String]]
+        XCTAssertEqual(settings, ["ATTRIBUTES": ["CodeSignOnCopy"]])


Similarly here we could benefit from some array comparisons to make the asserts more readable

(this is somewhat subjective)

// Then let copyBuildPhase = try XCTUnwrap(pbxTarget.embedFrameworksBuildPhases().first) let buildFiles = try XCTUnwrap(copyBuildPhase.files) XCTAssertEqual(buildFiles.map { $0.file?.path }, [ "Test.xcframework" ]) XCTAssertEqual(buildFiles.map { $0.settings as? [String: [String]] }, [ ["ATTRIBUTES": ["CodeSignOnCopy"]] ])

pepicrft · 2019-12-10T09:46:54Z

Sources/TuistGenerator/Generator/LinkGenerator.swift

@@ -169,7 +169,19 @@ final class LinkGenerator: LinkGenerating {

        try dependencies.forEach { dependency in
            if case let GraphDependencyReference.absolute(path) = dependency {
-                precompiledFrameworkPaths.append(path)
+                if path.extension == "xcframework" {


pepicrft · 2019-12-10T09:47:27Z

Tests/TuistGeneratorTests/Generator/LinkGeneratorTests.swift

@@ -88,6 +88,39 @@ final class LinkGeneratorErrorTests: XCTestCase {
            XCTAssertEqual($0 as? LinkGeneratorError, LinkGeneratorError.missingProduct(name: "Test"))
        }
    }
+
+    func test_generateEmbedPhase_setupEmbedFrameworksBuildPhase_whenXCFrameworkIsPresent() throws {


Beautifully tested 👏

pepicrft · 2019-12-10T09:47:59Z

features/generate.feature

@@ -230,3 +230,11 @@ Scenario: The project is an iOS application with watch app (ios_app_with_watchap
    Then I should be able to build for watchOS the scheme App
    Then the product 'App.app' with destination 'Debug-iphoneos' contains resource 'Watch/WatchApp.app'
    Then the product 'WatchApp.app' with destination 'Debug-watchos' contains extension 'WatchAppExtension' 
+
+Scenario: The project is an iOS application with Xcframeworks (ios_app_with_xcframeworks)


Nitpick: Xcframeworks -> xcframeworks

pepicrft

Hey @lakpa! Just wanted to say you did an amazing job here 👏. I'm very excited about seeing xcframeworks' support landing in the project.

lakpa · 2019-12-10T10:33:20Z

Hey @lakpa! Just wanted to say you did an amazing job here 👏. I'm very excited about seeing xcframeworks' support landing in the project.

Could manage to do with guidance from you and @kwridan , thanks for your guidance!!!

lakpa · 2019-12-10T10:42:41Z

Worth adding a static library xcframework fixture too

@kwridan , wondering if I could include the static lib fixture in next PR as follow up work is needed to support static libs, which hasn't accounted in this PR in regards to binary path.

kwridan

awesome, thanks for all the updates @lakpa

kwridan · 2019-12-11T19:46:42Z

Tests/TuistGeneratorTests/Generator/LinkGeneratorTests.swift

@@ -482,4 +515,19 @@ final class LinkGeneratorErrorTests: XCTestCase {

        return projectFileElements
    }
+
+    private func createFileElements(fileAbsolutePath: AbsolutePath,


awesome! thanks for making this helper 👍

One last suggestion to simplify it even further, we can prime the ProjectFileElements directly with the fileElement.

private func createFileElements(fileAbsolutePath: AbsolutePath) -> ProjectFileElements { let fileElements = ProjectFileElements() fileElements.elements[fileAbsolutePath] = PBXFileReference(path: fileAbsolutePath.basename) return fileElements }

Something like this makes it an even more re-usable helper here as it not specific to the Test.xcframework

kwridan · 2019-12-11T19:46:55Z

Tests/TuistGeneratorTests/Generator/LinkGeneratorTests.swift

+        XCTAssertEqual(buildFiles.map { $0.settings as? [String: [String]] }, [
+            ["ATTRIBUTES": ["CodeSignOnCopy"]]
+        ])
+    }


…y-type

lakpa requested review from pepicrft and kwridan December 8, 2019 19:18

lakpa added 2 commits December 9, 2019 21:29

Introduce a xcFramework dependency type

cef36f0

Add fixture for iOS app with xcframework

d8aa4da

lakpa force-pushed the feature/introduction-xcframework-dependency-type branch from 5a2069f to 0cdf13f Compare December 9, 2019 21:30

Add fixture tests for ios app with xcframework

9c21445

lakpa force-pushed the feature/introduction-xcframework-dependency-type branch from 0cdf13f to 9c21445 Compare December 9, 2019 21:33

kwridan reviewed Dec 9, 2019

View reviewed changes

Address merge conflicts

8b2e5ed

lakpa force-pushed the feature/introduction-xcframework-dependency-type branch from 0cc6b9d to 5f6241a Compare December 9, 2019 22:58

pepicrft reviewed Dec 10, 2019

View reviewed changes

pepicrft approved these changes Dec 10, 2019

View reviewed changes

lakpa requested a review from kwridan December 10, 2019 10:37

lakpa force-pushed the feature/introduction-xcframework-dependency-type branch from 5f6241a to 8d5344a Compare December 10, 2019 11:24

kwridan approved these changes Dec 11, 2019

View reviewed changes

lakpa added 2 commits December 11, 2019 20:31

Address comments

4842721

Update changelog

44dca72

lakpa force-pushed the feature/introduction-xcframework-dependency-type branch from 5778923 to 44dca72 Compare December 11, 2019 20:38

Merge branch 'master' into feature/introduction-xcframework-dependenc…

881ddfd

…y-type

lakpa merged commit 0ac8004 into tuist:master Dec 11, 2019

lakpa deleted the feature/introduction-xcframework-dependency-type branch December 11, 2019 20:47

kwridan mentioned this pull request Dec 17, 2019

Support .xcframework dependency type #401

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a xcframework dependency type #769

Introduce a xcframework dependency type #769

lakpa commented Dec 8, 2019 •

edited

kwridan left a comment

kwridan Dec 9, 2019

kwridan Dec 9, 2019

lakpa Dec 9, 2019

kwridan Dec 9, 2019

lakpa Dec 9, 2019

kwridan Dec 9, 2019

pepicrft Dec 10, 2019

pepicrft Dec 10, 2019

pepicrft Dec 10, 2019

pepicrft left a comment

lakpa commented Dec 10, 2019

lakpa commented Dec 10, 2019 •

edited

kwridan left a comment

kwridan Dec 11, 2019

kwridan Dec 11, 2019

Introduce a xcframework dependency type #769

Introduce a xcframework dependency type #769

Conversation

lakpa commented Dec 8, 2019 • edited

Short description 📝

Solution 📦

kwridan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepicrft left a comment

Choose a reason for hiding this comment

lakpa commented Dec 10, 2019

lakpa commented Dec 10, 2019 • edited

kwridan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakpa commented Dec 8, 2019 •

edited

lakpa commented Dec 10, 2019 •

edited