Skip to content
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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

liamnichols
Copy link
Contributor

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:

[...]/usr/bin/libtool: file: /[...]/MyFramework1.framework/MyFramework1 is a dynamic library, not added to the static library

It appears that this happens when I have three targets:

  1. MyFramework1 - Dynamic Framework (framework)
  2. MyFramework2 - Static Framework (framework.static) with dependency on MyFramework1
  3. MyApp - iOS app target with dependency on MyFramework2 (and a transitive dependency on MyFramework1)

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 target MyApp instead. After looking at the docs, and digging into the current fixtures, it appeared that this was defaulting the link property to the correct value if MyFramework2 was a static library (library.static) but not for framework.static or framework with the MACH_O_TYPE build setting set to staticlib.

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 the defaultLinkage 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 in
8e19482.

@liamnichols
Copy link
Contributor Author

Before I mark this as ready for review, there are a couple of other places in PBXProjGenerator that also perform similar checks and I'm wondering if they also need similar patches? I've personally only ever used the target type of dependency so i'm not 100% sure how these other areas need to behave.

Requires ObjC Linking

let requiresObjCLinking = (buildSettings["OTHER_LDFLAGS"] as? String)?.contains("-ObjC") ?? (productType == .staticLibrary)

if !anyDependencyRequiresObjCLinking
&& dependencyTarget.requiresObjCLinking ?? (dependencyTarget.type == .staticLibrary) {
anyDependencyRequiresObjCLinking = true
}

I'm not completely sure if "ObjC linking" is the same and if it applies to static frameworks or not?

Other Dependency Types

.framework

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

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

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

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! 🙏

@yonaskolb
Copy link
Owner

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 know it's a been a while, but if you had the time to add some a unit test for this case it would be great too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants