yonaskolb / XcodeGen

A Swift command line tool for generating your Xcode project
MIT License
7.01k stars 818 forks source link

Reuse ReferenceProxy references when possible #1354

Closed OdNairy closed 1 year ago

OdNairy commented 1 year ago

XcodeGen should reuse PBXReferenceProxy for external targets when possible. In the current state, Xcode will remove all duplicated dependencies as soon as any change in pbxproj happens.

https://user-images.githubusercontent.com/735178/235425213-eba0bdda-8caf-4b14-9f4a-7244ddb7c030.mov

OdNairy commented 1 year ago

@yonaskolb Is there anything I can do to help you to review this PR?

antonyalkmim commented 1 year ago

Thanks for this fix @OdNairy 👏.

I'm facing a problem using targetTemplates that I think this PR solves. Unfortunately I cant run some targets because the dependencies are not being imported on some targets that uses the same template. Clone and run xcodegen locally solves my problem.

@yonaskolb are you waiting for more PRs to add on next release or there is a release date? Can you release this fix for us? 😅

liamnichols commented 1 year ago

I was currently investigating something similar to #1339 in my project file and I wanted to test with the latest master to try and dig into the issue, but I think something in this commit might have added to the problem.

Prior to this change, I had one unused object reference for a PBXGroup that I was investigating, but now there are a lot of unused PBXContainerItemProxy references in the generated project file:

Screenshot 2023-07-25 at 17 42 53

(R.swift has a linting tool that is picking this up for me)

The references seem to be related to targets from a different Xcode project of mine:

Screenshot 2023-07-25 at 17 43 45

TofuClient, XCTestKit, APIKit, ImageKit etc are all targets in a referenced project called Frameworks.xcodeproj

Perhaps the missing piece here is that we're still creating new PBXContainerItemProxy objects when we reuse the PBXReferenceProxy which is leading to the orphaned object?

liamnichols commented 1 year ago

Ah yes, further up in the file, we're always adding the container item when we create it:

https://github.com/yonaskolb/XcodeGen/blob/af1adfe1cddaac0fc6be0ad50b79f6227900fa18/Sources/XcodeGenKit/PBXProjGenerator.swift#L410-L417

But really I think that we need to do something like this:

diff --git a/Sources/XcodeGenKit/PBXProjGenerator.swift b/Sources/XcodeGenKit/PBXProjGenerator.swift
index 6e77c3f..9ad0c91 100644
--- a/Sources/XcodeGenKit/PBXProjGenerator.swift
+++ b/Sources/XcodeGenKit/PBXProjGenerator.swift
@@ -407,13 +407,11 @@ public class PBXProjGenerator {
             )
         )

-        let productProxy = addObject(
-            PBXContainerItemProxy(
-                containerPortal: .fileReference(projectFileReference),
-                remoteGlobalID: targetObject.product.flatMap(PBXContainerItemProxy.RemoteGlobalID.object),
-                proxyType: .reference,
-                remoteInfo: target
-            )
+        let productProxy = PBXContainerItemProxy(
+            containerPortal: .fileReference(projectFileReference),
+            remoteGlobalID: targetObject.product.flatMap(PBXContainerItemProxy.RemoteGlobalID.object),
+            proxyType: .reference,
+            remoteInfo: target
         )

         var path = targetObject.productNameWithExtension()
@@ -437,6 +435,7 @@ public class PBXProjGenerator {
         if let existingValue = existingValue {
             productReferenceProxy = existingValue
         } else {
+            addObject(productProxy)
             productReferenceProxy = addObject(
                 PBXReferenceProxy(
                     fileType: productReferenceProxyFileType,

It fixed the issue in my project file, but I need to run through and see if there is more to it than that or not.

yonaskolb commented 1 year ago

@liamnichols would you mind opening a PR for this?