Fix dependency framework/library linking #93
Conversation
| // targetFrameworkBuildFiles.append(dependencyBuildFile) | ||
| if dependencyTarget.type.isLibrary || dependencyTarget.type.isFramework { | ||
| let dependencyBuildFile = targetBuildFileReferences[dependencyTargetName]! | ||
| targetFrameworkBuildFiles.append(dependencyBuildFile) |
There was a problem hiding this comment.
Hmm, this seems to not be the type that Xcode is expecting.
There was a problem hiding this comment.
Oops, I was testing with a project without a bundle identifier, which really confuses Xcode. This does work
|
Updated the project since the changes are legit, the next test run should be green. |
|
Hmm, it looks like there is some issue with this, where right after generating the project, if I do a I'll try and look in to why this is happening |
|
Based on the project diff after opening Xcode, it seems like the issue might be re-using file reference IDs in the "frameworks" build phase. As soon as you open the project in Xcode, it gets re-written to have new IDs, but leaves the original there for 1 of the references. |
|
Ok I just pushed a commit with a fix for this. So it turns out that the problem is, if you want to include a framework like this into multiple copy/frameworks/whatever phases, you need separate file PBXBuildFiles for each case. But they all need to point back to the original PBXFileReference which represents the original target's product. This now allows the same framework to be linked to multiple dependent targets |
| @@ -46,6 +46,14 @@ extension PBXProductType { | |||
| } | |||
| } | |||
|
|
|||
| public var isFramework: Bool { | |||
|
Thanks for the review! Added some tests here |
| @@ -46,6 +46,14 @@ extension PBXProductType { | |||
| } | |||
| } | |||
|
|
|||
| public var isFramework: Bool { | |||
| return fileExtension == "framework" | |||
There was a problem hiding this comment.
I don't think this getter is need if it relates to a single case, we can just use target.type == .framework at call sites. At the very least, this getter could itself use self == .framework
There was a problem hiding this comment.
Let's leave the getter here to match the other accessors, but check the case within it 👍
| } | ||
|
|
||
| public var isLibrary: Bool { | ||
| return name == "library.static" || name == "library.dynamic" |
There was a problem hiding this comment.
can this use the actual enum cases instead of strings
|
|
||
| if embed { | ||
| if dependencyTarget.type.isLibrary { |
There was a problem hiding this comment.
Is a library never embeded, or are there special cases where they are?
If it can be sometimes, ca we move this logic up into where let embed is defined, which gives the embedding a default, and then users can override this.
If it's definitely never, we can also move this if into the if above if embed && !dependencyTarget.type.isLibrary
There was a problem hiding this comment.
They are never embedded. Since that would just mean that you're literally copying the .a file into your .app, which would never be used at runtime since static libraries are linked into your final binary instead. I've updated the condition!
Tests/LinuxMain.swift
Outdated
| @@ -5,4 +5,5 @@ XCTMain([ | |||
| testCase(GeneratorTests.allTests), | |||
| testCase(SpecLoadingTests.allTests), | |||
| testCase(FixtureTests.allTests), | |||
| testCase(TargetTests.allTests), | |||
There was a problem hiding this comment.
Can we make this ProjectSpecTests?
These should only be linked. If we add a library to the project here, Xcode will remove it next time it touches the project
|
All feedback addressed! |
This PR updates XcodeGen to link framework and library dependencies of targets, into the dependent targets. This is required for them to be usable at runtime. This also removes libraries from the copy framework build phase, since that just results in a
.afile ending up in your final package.This should fix #92