-
Notifications
You must be signed in to change notification settings - Fork 822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue that caused static framework target dependencies from having their default link
value inferred as true
#1110
base: master
Are you sure you want to change the base?
Conversation
…ies to dynamic framework targets to reproduce bug
…et dependencies where the dependency is a static framework (not library)
Before I mark this as ready for review, there are a couple of other places in Requires ObjC Linking
XcodeGen/Sources/XcodeGenKit/PBXProjGenerator.swift Lines 720 to 723 in a4d7a61
I'm not completely sure if "ObjC linking" is the same and if it applies to static frameworks or not? Other Dependency Types
|
if dependency.link ?? (target.type != .staticLibrary) { | |
let pbxBuildFile = PBXBuildFile(file: fileReference, settings: getDependencyFrameworkSettings(dependency: dependency)) | |
pbxBuildFile.platformFilter = platform | |
let buildFile = addObject(pbxBuildFile) | |
targetFrameworkBuildFiles.append(buildFile) | |
} |
I think this is intentionally specific to libraries so can remain 👍
.carthage
XcodeGen/Sources/XcodeGenKit/PBXProjGenerator.swift
Lines 897 to 904 in a4d7a61
let isStaticLibrary = target.type == .staticLibrary | |
let isCarthageStaticLink = dependency.carthageLinkType == .static | |
if dependency.link ?? (!isStaticLibrary && !isCarthageStaticLink) { | |
let pbxBuildFile = PBXBuildFile(file: fileReference, settings: getDependencyFrameworkSettings(dependency: dependency)) | |
pbxBuildFile.platformFilter = platform | |
let buildFile = addObject(pbxBuildFile) | |
targetFrameworkBuildFiles.append(buildFile) | |
} |
I think maybe this needs patching, but i've not used Carthage in a longgg time so I can't be sure
.package
XcodeGen/Sources/XcodeGenKit/PBXProjGenerator.swift
Lines 926 to 937 in a4d7a61
let link = dependency.link ?? (target.type != .staticLibrary) | |
if link { | |
let buildFile = addObject( | |
PBXBuildFile(product: packageDependency, settings: getDependencyFrameworkSettings(dependency: dependency)) | |
) | |
targetFrameworkBuildFiles.append(buildFile) | |
} else { | |
let targetDependency = addObject( | |
PBXTargetDependency(platformFilter: platform, product: packageDependency) | |
) | |
dependencies.append(targetDependency) | |
} |
Again, I don't have enough context to know the right answer
.bundle
XcodeGen/Sources/XcodeGenKit/PBXProjGenerator.swift
Lines 951 to 953 in a4d7a61
case .bundle: | |
// Static and dynamic libraries can't copy resources | |
guard target.type != .staticLibrary && target.type != .dynamicLibrary else { break } |
Looks good, its specifically checking for libraries (not link types).
@yonaskolb: Any input on the bits above would be great, thanks! 🙏
Hi @liamnichols. Thanks for this, your change makes sense. As for the other places, I'm honestly not sure. Maybe just go with your change, and someone else might bring up if something else is not working. For this could you add a changelog entry, might need to rebase off master to get the latest changelog. |
I'm updating our project to statically link some framework targets instead of using dynamic linking for everything and I spotted some warnings appearing in CI when doing do:
It appears that this happens when I have three targets:
MyFramework1
- Dynamic Framework (framework
)MyFramework2
- Static Framework (framework.static
) with dependency onMyFramework1
MyApp
- iOS app target with dependency onMyFramework2
(and a transitive dependency onMyFramework1
)In this setup, the
MyFramework2
target should not have a "Link Binary With Libraries" build phase because the libraries should be linked into the executable targetMyApp
instead. After looking at the docs, and digging into the current fixtures, it appeared that this was defaulting thelink
property to the correct value ifMyFramework2
was a static library (library.static
) but not forframework.static
orframework
with theMACH_O_TYPE
build setting set tostaticlib
.After setting up some extra targets in the fixtures to reproduce the setup (1f391f1), i tracked the issue down to
PBXProjectGenerator
and updated it to use thedefaultLinkage
property that is also used for figuring out if the dependency should be embedded or not (159f719). You can find the fixtures regenerated after making the changes in8e19482.