Support Static Frameworks for Carthage dependencies #688

giginet · 2019-10-20T06:08:18Z

Motivation and Context

Carthage officially supports Static Frameworks.
https://github.com/Carthage/Carthage#build-static-frameworks-to-speed-up-your-apps-launch-times

These frameworks are built to different directory with dynamic ones. So we can't integrate static frameworks automatically by carthage dependency.

Description

This PR makes it enable to integrate Carthage static framework easily.

targets:
  MyTarget:
    dependencies:
      - carthage: AStaticFramework
        linkType: static

With enabling this option, XcodeGen behaves like following:

Finding search path should beCarthage/Build/${Platform}/Static/${FrameworkName}.framework
Static frameworks aren't added to the copy-framework build phase

Sources/XcodeGenKit/PBXProjGenerator.swift

giginet · 2019-10-20T06:12:03Z

Sources/XcodeGenKit/PBXProjGenerator.swift

-                configFrameworkBuildPaths = [carthagePlatformBuildPath] + frameworkBuildPaths.sorted()
+                var carthagePlatformBuildPaths: [String] = []
+                if carthageDependencies.contains(where: { isStaticDependency(for: $0) }) {
+                    let carthagePlatformBuildPath = "$(PROJECT_DIR)/" + carthageResolver.buildPath(for: target.platform, isStatic: true)


If targets have any static frameworks, the static framework search path will be added before dynamic one.

giginet · 2019-10-20T06:13:21Z

Tests/XcodeGenKitTests/ProjectGeneratorTests.swift

+                        }
+                        try expect(file.file?.nameOrPath) == "MyStaticFramework.framework"
+
+                        try expect(target.carthageCopyFrameworkBuildPhase()).beNil()


If a target contains only static dependencies, then Carthage build phase should not be added.

giginet · 2019-10-20T06:13:45Z

Tests/XcodeGenKitTests/ProjectGeneratorTests.swift

+                        guard let copyCarthagePhase = target.carthageCopyFrameworkBuildPhase() else {
+                            return XCTFail("Carthage Build Phase should be exist")
+                        }
+                        try expect(copyCarthagePhase.inputPaths) == [dynamicFramework.file?.fullPath(sourceRoot: Path("$(SRCROOT)"))?.string]


Static Framework should not be copied.

yonaskolb · 2019-10-27T07:02:22Z

@brentleyjones were you looking at this work earlier?

brentleyjones · 2019-10-28T14:46:18Z

were you looking at this work earlier?

@yonaskolb I was, but have since long abandoned that branch, and currently don't have strong opinions about static linking. I'll give this a once over though.

Docs/ProjectSpec.md

brentleyjones · 2019-10-28T14:48:30Z

Sources/XcodeGenKit/PBXProjGenerator.swift

@@ -459,6 +459,15 @@ public class PBXProjGenerator {
            }
            return !linkingAttributes.isEmpty ? ["ATTRIBUTES": linkingAttributes] : nil
        }
+
+        func isStaticDependency(for carthageDependency: Dependency) -> Bool {


Could this instead be an extension on Dependency?

This is a local function inside generateTarget function. So it has a narrower scope than fileprivate extension. This is why I use function instead of extension.

Sources/XcodeGenKit/PBXProjGenerator.swift

Sources/ProjectSpec/Dependency.swift

giginet · 2019-10-29T17:55:03Z

@brentleyjones Thanks for your review!

I addressed your points. Could you review this again?

brentleyjones

This looks good. Thanks @giginet!

giginet · 2019-10-29T18:11:19Z

@yonaskolb We need your eyes before merging. Could you check this in your spare time?

yonaskolb · 2019-11-07T11:58:46Z

Sources/ProjectSpec/Dependency.swift

@@ -36,7 +36,7 @@ public struct Dependency: Equatable {
    public enum DependencyType: Equatable {
        case target
        case framework
-        case carthage(findFrameworks: Bool?)
+        case carthage(findFrameworks: Bool?, static: Bool)


How do you feel about making this a linkType: LinkType enum with static and dynamic cases? I don't feel too strongly either way. I think we @brentleyjones was building this he mentioned a similar approach.

Are there other dependency types that we might want use this on? If so we could move this property into the Dependency itself, as opposed to the enum

1

I feel good linkType than the current implementation 👍

2
Are there other dependency types that we might want use this on?

No. I think it's easier to read Dependency has linkType property than using associated value.
But using associated value is more right to express structures.
That's difficult choices...

yonaskolb

You can rebase off master to fix CI

yonaskolb · 2019-11-07T12:08:23Z

Sources/XcodeGenKit/PBXProjGenerator.swift

@@ -463,6 +463,15 @@ public class PBXProjGenerator {
            }
            return !linkingAttributes.isEmpty ? ["ATTRIBUTES": linkingAttributes] : nil
        }
+
+        func isStaticDependency(for carthageDependency: Dependency) -> Bool {


I agree about moving this to an extension. If we move static/linkType to Dependency itself like in the other comment we don't even need this function.

Otherwise if this crashes when used with other types it should be called carthageDependencyIsStatic, otherwise it should probably just return false for other cases

giginet · 2019-11-07T14:42:11Z

@yonaskolb @brentleyjones I modified spec interface from your comments. Could you review again?
705e351

brentleyjones · 2019-11-07T14:51:55Z

Sources/ProjectSpec/Dependency.swift


    public enum DependencyType: Equatable {
        case target
        case framework
-        case carthage(findFrameworks: Bool?)
+        case carthage(findFrameworks: Bool?, linkType: CarthageLinkType = .default)


I'm not sure if we are dropping lower Swift compiler versions here yet, so we shouldn't use a default associated value.

reverted. 14a249d

This reverts commit 7590523.

giginet added 13 commits October 20, 2019 14:48

Add isStatic option

05e566a

Support Carthage static framework

495ee2e

Fix static supports

da1f00b

Fix tests

6861f4e

Drop isStatic property

0315692

Add tests for static dependencies

e69d119

Add tests for mixed dependencies

2eab87a

Add test for parsing static carthage dependency

a5c3e4e

Add documentation for Carthage static frameworks

c5134ca

CHANGELOG

ece256a

Revert unnecessary changes

9434b58

isStatic should be non-optional

adf16ef

Tweak

2a0b4b8

giginet requested a review from yonaskolb October 20, 2019 06:08

Update CHANGELOG.md

1d75920

giginet commented Oct 20, 2019

View reviewed changes

Sources/XcodeGenKit/PBXProjGenerator.swift Outdated Show resolved Hide resolved

giginet commented Oct 20, 2019

View reviewed changes

Update ProjectSpec.md

45bf301

yonaskolb requested a review from brentleyjones October 27, 2019 07:02

brentleyjones reviewed Oct 28, 2019

View reviewed changes

giginet added 3 commits October 30, 2019 02:46

Merge remote-tracking branch 'origin/master' into static

b6cb516

Flip documentation

6929285

Simplify conditions

e54155e

brentleyjones approved these changes Oct 29, 2019

View reviewed changes

giginet added 2 commits October 30, 2019 11:35

Merge branch 'master' into static

1f8285c

Merge branch 'master' into static

f71cc95

yonaskolb reviewed Nov 7, 2019

View reviewed changes

Define CarthageLinkType instead of isStatic

705e351

Use default assoc value

7590523

brentleyjones requested changes Nov 7, 2019

View reviewed changes

giginet added 2 commits November 7, 2019 23:56

Revert "Use default assoc value"

14a249d

This reverts commit 7590523.

Use computed property instead

60ad019

brentleyjones approved these changes Nov 7, 2019

View reviewed changes

brentleyjones merged commit 03076f4 into yonaskolb:master Nov 7, 2019

giginet deleted the static branch November 7, 2019 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Static Frameworks for Carthage dependencies #688

Support Static Frameworks for Carthage dependencies #688

giginet commented Oct 20, 2019 •

edited

Loading

giginet Oct 20, 2019 •

edited

Loading

giginet Oct 20, 2019 •

edited

Loading

giginet Oct 20, 2019

yonaskolb commented Oct 27, 2019

brentleyjones commented Oct 28, 2019

brentleyjones Oct 28, 2019

giginet Oct 28, 2019 •

edited

Loading

giginet commented Oct 29, 2019

brentleyjones left a comment

giginet commented Oct 29, 2019

yonaskolb Nov 7, 2019

giginet Nov 7, 2019 •

edited

Loading

yonaskolb left a comment

yonaskolb Nov 7, 2019

giginet commented Nov 7, 2019

brentleyjones Nov 7, 2019

giginet Nov 7, 2019

Support Static Frameworks for Carthage dependencies #688

Support Static Frameworks for Carthage dependencies #688

Conversation

giginet commented Oct 20, 2019 • edited Loading

Motivation and Context

Description

giginet Oct 20, 2019 • edited Loading

Choose a reason for hiding this comment

giginet Oct 20, 2019 • edited Loading

Choose a reason for hiding this comment

giginet Oct 20, 2019

Choose a reason for hiding this comment

yonaskolb commented Oct 27, 2019

brentleyjones commented Oct 28, 2019

brentleyjones Oct 28, 2019

Choose a reason for hiding this comment

giginet Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

giginet commented Oct 29, 2019

brentleyjones left a comment

Choose a reason for hiding this comment

giginet commented Oct 29, 2019

yonaskolb Nov 7, 2019

Choose a reason for hiding this comment

giginet Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

yonaskolb left a comment

Choose a reason for hiding this comment

yonaskolb Nov 7, 2019

Choose a reason for hiding this comment

giginet commented Nov 7, 2019

brentleyjones Nov 7, 2019

Choose a reason for hiding this comment

giginet Nov 7, 2019

Choose a reason for hiding this comment

giginet commented Oct 20, 2019 •

edited

Loading

giginet Oct 20, 2019 •

edited

Loading

giginet Oct 20, 2019 •

edited

Loading

giginet Oct 28, 2019 •

edited

Loading

giginet Nov 7, 2019 •

edited

Loading