Stabilize sorting of groups with duplicate names/paths (#671)

* Stabilize sorting of groups with duplicate names/paths

For example, previously a group with (name: nil, path: "Sources") would be considered equal to (name: "Sources", path: "../Sources"), even though they are distinct groups.

* Remove Comparable conformance from PBXFileElement

...as it isn't compatible with its Equatable conformance.

Renamed localizedStandardCompare to reflect the fact that PBXFileElement no longer has an inherent order.

* Update changelog

CHANGELOG.md

@@ -26,6 +26,7 @@
 - Fixed resolving relative path passed to `XcodeProj` [#751](https://github.com/yonaskolb/XcodeGen/pull/751) @PycKamil
 - Prefer configurations named "Debug" or "Release" for default scheme build configurations [#752](https://github.com/yonaskolb/XcodeGen/pull/752) @john-flanagan
 - Added an extra check for package versions. [#755](https://github.com/yonaskolb/XcodeGen/pull/755) @basvankuijck
+- Stabilized sorting of groups with duplicate names/paths. [#671](https://github.com/yonaskolb/XcodeGen/pull/671) @ChristopherRogers
 
 #### Internal
 - Update to SwiftCLI 6.0 and use the new property wrappers [#749](https://github.com/yonaskolb/XcodeGen/pull/749) @yonaskolb

Sources/XcodeGenKit/PBXProjGenerator.swift

@@ -227,7 +227,7 @@ public class PBXProjGenerator {
             for (platform, files) in carthageFrameworksByPlatform {
                 let platformGroup: PBXGroup = addObject(
                     PBXGroup(
-                        children: files.sorted { $0.nameOrPath < $1.nameOrPath },
+                        children: files.sorted(by: PBXFileElement.areNamePathInIncreasingOrder(_:_:)),
                         sourceTree: .group,
                         path: platform
                     )
@@ -272,7 +272,7 @@ public class PBXProjGenerator {
         // add derived groups at the end
         derivedGroups.forEach(sortGroups)
         mainGroup.children += derivedGroups
-            .sorted { $0.nameOrPath.localizedStandardCompare($1.nameOrPath) == .orderedAscending }
+            .sorted { $0.namePathLocalizedStandardCompare($1) == .orderedAscending }
             .map { $0 }
 
         let assetTags = Set(project.targets
@@ -551,8 +551,8 @@ public class PBXProjGenerator {
                 if sortOrder1 != sortOrder2 {
                     return sortOrder1 < sortOrder2
                 } else {
-                    if child1.nameOrPath != child2.nameOrPath {
-                        return child1.nameOrPath.localizedStandardCompare(child2.nameOrPath) == .orderedAscending
+                    if (child1.name, child1.path) != (child2.name, child2.path) {
+                        return child1.namePathLocalizedStandardCompare(child2) == .orderedAscending
                     } else {
                         return child1.context ?? "" < child2.context ?? ""
                     }

Sources/XcodeGenKit/XCProjExtensions.swift

@@ -3,9 +3,21 @@ import PathKit
 import XcodeProj
 
 extension PBXFileElement {
-
     public var nameOrPath: String {
-        name ?? path ?? ""
+        return name ?? path ?? ""
+    }
+
+    func namePathLocalizedStandardCompare(_ rhs: PBXFileElement) -> ComparisonResult {
+        return sortString.localizedStandardCompare(rhs.sortString)
+    }
+
+    static func areNamePathInIncreasingOrder(_ lhs: PBXFileElement, _ rhs: PBXFileElement) -> Bool {
+        return lhs.sortString < rhs.sortString
+    }
+
+    private var sortString: String {
+        // This string needs to be unique for all combinations of name & path or the order won't be stable.
+        return "\(name ?? path ?? "")\t\(name ?? "")\t\(path ?? "")"
     }
 }
 

Tests/ProjectSpecTests/ProjectSpecTests.swift

@@ -545,7 +545,7 @@ class ProjectSpecTests: XCTestCase {
                 let json = proj.toJSONDictionary()
                 let restoredProj = try Project(basePath: Path.current, jsonDictionary: json)
 
-                // Examin some properties to make debugging easier
+                // Examine some properties to make debugging easier
                 try expect(proj.aggregateTargets) == restoredProj.aggregateTargets
                 try expect(proj.configFiles) == restoredProj.configFiles
                 try expect(proj.settings) == restoredProj.settings

Tests/XcodeGenKitTests/SourceGeneratorTests.swift

@@ -676,6 +676,10 @@ class SourceGeneratorTests: XCTestCase {
 
             $0.it("sorts files") {
                 let directories = """
+                    A:
+                    - A.swift
+                    Source:
+                        - file.swift
                     Sources:
                     - file3.swift
                     - file.swift
@@ -686,17 +690,55 @@ class SourceGeneratorTests: XCTestCase {
                         - file.swift
                     - group:
                         - file.swift
+                    Z:
+                    - A:
+                        - file.swift
+                    B:
+                    - file.swift
                 """
                 try createDirectories(directories)
 
-                let target = Target(name: "Test", type: .application, platform: .iOS, sources: ["Sources"], dependencies: [Dependency(type: .carthage(findFrameworks: false, linkType: .dynamic), reference: "Alamofire")])
+                let target = Target(name: "Test", type: .application, platform: .iOS, sources: [
+                        "Sources",
+                        TargetSource(path: "Source", name: "S"),
+                        "A",
+                        TargetSource(path: "Z/A", name: "B"),
+                        "B",
+                ], dependencies: [Dependency(type: .carthage(findFrameworks: false, linkType: .dynamic), reference: "Alamofire")])
                 let project = Project(basePath: directoryPath, name: "Test", targets: [target])
 
                 let pbxProj = try project.generatePbxProj()
                 let mainGroup = try pbxProj.getMainGroup()
-                let group = mainGroup.children.compactMap { $0 as? PBXGroup }.first { $0.nameOrPath == "Sources" }!
-                let names = group.children.compactMap { $0.nameOrPath }
+                let mainGroupNames = mainGroup.children.prefix(5).map { $0.name }
+                try expect(mainGroupNames) == [
+                    nil,
+                    nil,
+                    "B",
+                    "S",
+                    nil,
+                ]
+                let mainGroupPaths = mainGroup.children.prefix(5).map { $0.path }
+                try expect(mainGroupPaths) == [
+                    "A",
+                    "B",
+                    "Z/A",
+                    "Source",
+                    "Sources",
+                ]
+
+                let group = mainGroup.children.compactMap { $0 as? PBXGroup }.first { $0.path == "Sources" }!
+                let names = group.children.map { $0.name }
                 try expect(names) == [
+                    nil,
+                    nil,
+                    nil,
+                    nil,
+                    nil,
+                    nil,
+                    nil,
+                ]
+                let paths = group.children.map { $0.path }
+                try expect(paths) == [
                     "1file.a",
                     "10file.a",
                     "file.swift",