diff --git a/lib/kintsugi/apply_change_to_project.rb b/lib/kintsugi/apply_change_to_project.rb index 0e925b2..c474e3f 100644 --- a/lib/kintsugi/apply_change_to_project.rb +++ b/lib/kintsugi/apply_change_to_project.rb @@ -728,25 +728,54 @@ def add_child_to_component(component, change, change_path) end def add_remote_swift_package_reference(containing_component, change, change_path) - remote_swift_package_reference = - containing_component.project.new(Xcodeproj::Project::XCRemoteSwiftPackageReference) - add_attributes_to_component(remote_swift_package_reference, change, change_path) + project = containing_component.project + # A remote package reference is shared project-wide: `rootObject.packageReferences` and every + # product dependency's `package` point at the same object. The diff adds it from each place, + # so reuse an equivalent reference already in the project rather than adding a duplicate. + remote_swift_package_reference = existing_remote_swift_package_reference(project, change) + + if remote_swift_package_reference.nil? + remote_swift_package_reference = + project.new(Xcodeproj::Project::XCRemoteSwiftPackageReference) + add_attributes_to_component(remote_swift_package_reference, change, change_path) + end case containing_component when Xcodeproj::Project::XCSwiftPackageProductDependency containing_component.package = remote_swift_package_reference when Xcodeproj::Project::PBXProject - containing_component.package_references << remote_swift_package_reference + references = containing_component.package_references + unless references.any? { |reference| reference.equal?(remote_swift_package_reference) } + references << remote_swift_package_reference + end else raise MergeError, "Trying to add remote swift package reference to an unsupported " \ "component type #{containing_component.isa}. Change is: #{change}" end end + # An existing remote swift package reference in `project` whose tree hash equals `change`, else + # nil. Package references are shared project-wide, so the lookup is not scoped to a target. + def existing_remote_swift_package_reference(project, change) + project.objects.find do |object| + object.isa == "XCRemoteSwiftPackageReference" && object.to_tree_hash == change + end + end + def add_swift_package_product_dependency(containing_component, change, change_path) - swift_package_product_dependency = - containing_component.project.new(Xcodeproj::Project::XCSwiftPackageProductDependency) - add_attributes_to_component(swift_package_product_dependency, change, change_path) + # Within a single target, the target's `packageProductDependencies` entry and the `productRef` + # of the build file that links the product are the same object. The diff adds it from both + # places, so reuse an equivalent dependency already present in the SAME target rather than + # adding a duplicate. The reuse is scoped to the target because Xcode keeps a separate + # dependency object per target. Looking up before creating avoids adding a throwaway object. + target = owning_native_target(containing_component) + swift_package_product_dependency = existing_package_product_dependency(target, change) + + if swift_package_product_dependency.nil? + swift_package_product_dependency = + containing_component.project.new(Xcodeproj::Project::XCSwiftPackageProductDependency) + add_attributes_to_component(swift_package_product_dependency, change, change_path) + end case containing_component when Xcodeproj::Project::PBXBuildFile @@ -759,6 +788,33 @@ def add_swift_package_product_dependency(containing_component, change, change_pa end end + # The native target that owns `component`: the target itself, or -- for a build file -- the + # target whose build phases contain it. Returns nil if none (e.g. the build phase is not yet + # linked to a target), in which case the caller adds a fresh dependency. + def owning_native_target(component) + return component if component.is_a?(Xcodeproj::Project::PBXNativeTarget) + return nil unless component.is_a?(Xcodeproj::Project::PBXBuildFile) + + component.project.native_targets.find do |target| + # Match by object identity, not `include?` (which compares by value): a freshly-created + # build file is still attribute-less here, and `PBXBuildFile#==` would treat it as equal to + # any other attribute-less build file, wrongly attributing it to an earlier target. + target.build_phases.any? do |build_phase| + build_phase.files.any? { |file| file.equal?(component) } + end + end + end + + # An existing package product dependency of `target` (in its `packageProductDependencies` or + # referenced by one of its build files) whose tree hash equals `change`, else nil. + def existing_package_product_dependency(target, change) + return nil if target.nil? + + candidates = target.package_product_dependencies.to_a + + target.build_phases.flat_map(&:files).map(&:product_ref).compact + candidates.uniq.find { |candidate| candidate.to_tree_hash == change } + end + def add_reference_proxy(containing_component, change, change_path) case containing_component when Xcodeproj::Project::PBXBuildFile @@ -1084,14 +1140,24 @@ def add_build_configuration(configuration_list, change, change_path) end def add_build_file(build_phase, change, change_path) - if change["fileRef"].nil? - puts "Warning: Trying to add a build file without any file reference to build phase " \ - "'#{build_phase}'" + # A build file references either a file (`fileRef`) or a Swift package product (`productRef`). + if change["fileRef"].nil? && change["productRef"].nil? + puts "Warning: Trying to add a build file without any file or product reference to build " \ + "phase '#{build_phase}'" return end existing_build_file = build_phase.files.find do |build_file| - build_file.file_ref && build_file.file_ref.path == change["fileRef"]["path"] + if change["fileRef"] + build_file.file_ref && build_file.file_ref.path == change["fileRef"]["path"] + else + # Compare the whole product reference, not just its name: two different Swift package + # products (from different packages) may share a product name, so a name-only match would + # wrongly drop a distinct build file. Comparing the full tree hash also distinguishes + # nameless products by their package. + build_file.product_ref && + build_file.product_ref.to_tree_hash == change["productRef"] + end end return if !Settings.allow_duplicates && !existing_build_file.nil? diff --git a/spec/kintsugi_apply_change_to_project_spec.rb b/spec/kintsugi_apply_change_to_project_spec.rb index a387acf..e12ee49 100644 --- a/spec/kintsugi_apply_change_to_project_spec.rb +++ b/spec/kintsugi_apply_change_to_project_spec.rb @@ -644,6 +644,174 @@ expect(base_project).to be_equivalent_to_project(theirs_project) end + it "adds a build file that references a swift package product" do + theirs_project = create_copy_of_project(base_project, "theirs") + build_file = theirs_project.new(Xcodeproj::Project::PBXBuildFile) + build_file.product_ref = create_swift_package_product_dependency(theirs_project) + theirs_project.targets[0].frameworks_build_phase.files << build_file + + changes_to_apply = get_diff(theirs_project, base_project) + + described_class.apply_change_to_project(base_project, changes_to_apply, theirs_project) + + expect(base_project).to be_equivalent_to_project(theirs_project) + end + + it "links a target and its build file to the same package product dependency" do + theirs_project = create_copy_of_project(base_project, "theirs") + dependency = create_swift_package_product_dependency(theirs_project) + theirs_project.targets[0].package_product_dependencies << dependency + build_file = theirs_project.new(Xcodeproj::Project::PBXBuildFile) + build_file.product_ref = dependency + theirs_project.targets[0].frameworks_build_phase.files << build_file + + changes_to_apply = get_diff(theirs_project, base_project) + + described_class.apply_change_to_project(base_project, changes_to_apply, theirs_project) + + expect(base_project).to be_equivalent_to_project(theirs_project) + + # The dependency is shared, as Xcode writes it: the build file's product_ref is the very same + # object as the target's package product dependency, not a duplicate. + dependencies = base_project.objects.select { |o| o.isa == "XCSwiftPackageProductDependency" } + expect(dependencies.count).to eq(1) + product_build_file = + base_project.targets[0].frameworks_build_phase.files.find(&:product_ref) + expect(product_build_file.product_ref) + .to equal(base_project.targets[0].package_product_dependencies.first) + end + + it "shares one package reference between packageReferences and the product dependency" do + # The shape Xcode writes when adding a package: the package reference sits on + # rootObject.packageReferences, the product dependency's `package` points at that same object, + # and the build file's product_ref points at the dependency. + theirs_project = create_copy_of_project(base_project, "theirs") + package_reference = theirs_project.new(Xcodeproj::Project::XCRemoteSwiftPackageReference) + package_reference.repositoryURL = "https://github.com/example/pkg" + package_reference.requirement = {"kind" => "upToNextMajorVersion", "minimumVersion" => "1.0.0"} + theirs_project.root_object.package_references << package_reference + dependency = theirs_project.new(Xcodeproj::Project::XCSwiftPackageProductDependency) + dependency.package = package_reference + dependency.product_name = "Example" + theirs_project.targets[0].package_product_dependencies << dependency + build_file = theirs_project.new(Xcodeproj::Project::PBXBuildFile) + build_file.product_ref = dependency + theirs_project.targets[0].frameworks_build_phase.files << build_file + + changes_to_apply = get_diff(theirs_project, base_project) + + described_class.apply_change_to_project(base_project, changes_to_apply, theirs_project) + + expect(base_project).to be_equivalent_to_project(theirs_project) + + # A single package reference object, shared by rootObject.packageReferences and the + # dependency's `package`; a single dependency, linked by the build file. + package_references = base_project.objects.select { |o| o.isa == "XCRemoteSwiftPackageReference" } + dependencies = base_project.objects.select { |o| o.isa == "XCSwiftPackageProductDependency" } + expect(package_references.count).to eq(1) + expect(dependencies.count).to eq(1) + expect(base_project.root_object.package_references.first).to equal(package_references.first) + expect(dependencies.first.package).to equal(package_references.first) + product_build_file = + base_project.targets[0].frameworks_build_phase.files.find(&:product_ref) + expect(product_build_file.product_ref).to equal(dependencies.first) + end + + it "keeps a separate package product dependency per target for the same product" do + base_project.new_target("com.apple.product-type.library.static", "bar", :ios) + + theirs_project = create_copy_of_project(base_project, "theirs") + theirs_project.targets.each do |target| + dependency = create_swift_package_product_dependency(theirs_project) + target.package_product_dependencies << dependency + build_file = theirs_project.new(Xcodeproj::Project::PBXBuildFile) + build_file.product_ref = dependency + target.frameworks_build_phase.files << build_file + end + + changes_to_apply = get_diff(theirs_project, base_project) + + described_class.apply_change_to_project(base_project, changes_to_apply, theirs_project) + + expect(base_project).to be_equivalent_to_project(theirs_project) + + # Each target keeps its own dependency object -- Xcode does not share one across targets -- but + # within a target the build file's product_ref is that same object. + dependencies = base_project.objects.select { |o| o.isa == "XCSwiftPackageProductDependency" } + expect(dependencies.count).to eq(2) + base_project.targets.each do |target| + product_build_file = target.frameworks_build_phase.files.find(&:product_ref) + expect(product_build_file.product_ref).to equal(target.package_product_dependencies.first) + end + end + + it "keeps two products that share a name but come from different packages" do + base_dependency = create_swift_package_product_dependency(base_project) + base_project.targets[0].package_product_dependencies << base_dependency + base_build_file = base_project.new(Xcodeproj::Project::PBXBuildFile) + base_build_file.product_ref = base_dependency + base_project.targets[0].frameworks_build_phase.files << base_build_file + + # theirs links a second product with the SAME product name but from a different package. + theirs_project = create_copy_of_project(base_project, "theirs") + other_package = theirs_project.new(Xcodeproj::Project::XCRemoteSwiftPackageReference) + other_package.repositoryURL = "http://other" + other_dependency = theirs_project.new(Xcodeproj::Project::XCSwiftPackageProductDependency) + other_dependency.product_name = "foo" + other_dependency.package = other_package + theirs_project.targets[0].package_product_dependencies << other_dependency + other_build_file = theirs_project.new(Xcodeproj::Project::PBXBuildFile) + other_build_file.product_ref = other_dependency + theirs_project.targets[0].frameworks_build_phase.files << other_build_file + + changes_to_apply = get_diff(theirs_project, base_project) + + described_class.apply_change_to_project(base_project, changes_to_apply, theirs_project) + + expect(base_project).to be_equivalent_to_project(theirs_project) + + # Both same-named products are distinct (different packages), so both survive, each linked by + # its own build file -- a name-only dedup would have dropped the second build file. + dependencies = base_project.objects.select { |o| o.isa == "XCSwiftPackageProductDependency" } + expect(dependencies.count).to eq(2) + product_build_files = + base_project.targets[0].frameworks_build_phase.files.select(&:product_ref) + expect(product_build_files.count).to eq(2) + end + + it "attributes an added product build file to its own target despite a bare build file elsewhere" do + base_project.new_target("com.apple.product-type.library.static", "bar", :ios) + foo = base_project.targets.find { |target| target.display_name == "foo" } + # foo already links the product, and also carries a bare (reference-less) build file, which is + # value-equal to any freshly-created build file. + foo.package_product_dependencies << create_swift_package_product_dependency(base_project) + foo_build_file = base_project.new(Xcodeproj::Project::PBXBuildFile) + foo_build_file.product_ref = foo.package_product_dependencies.first + foo.frameworks_build_phase.files << foo_build_file + foo.frameworks_build_phase.files << base_project.new(Xcodeproj::Project::PBXBuildFile) + + # theirs links the same product in bar. + theirs_project = create_copy_of_project(base_project, "theirs") + theirs_bar = theirs_project.targets.find { |target| target.display_name == "bar" } + theirs_bar.package_product_dependencies << create_swift_package_product_dependency(theirs_project) + bar_build_file = theirs_project.new(Xcodeproj::Project::PBXBuildFile) + bar_build_file.product_ref = theirs_bar.package_product_dependencies.first + theirs_bar.frameworks_build_phase.files << bar_build_file + + changes_to_apply = get_diff(theirs_project, base_project) + + described_class.apply_change_to_project(base_project, changes_to_apply, theirs_project) + + expect(base_project).to be_equivalent_to_project(theirs_project) + + # bar's build file must link bar's own dependency, not foo's (a value-based target lookup would + # attribute it to foo via foo's bare build file, collapsing the two targets' dependencies). + merged_bar = base_project.targets.find { |target| target.display_name == "bar" } + bar_reference = merged_bar.frameworks_build_phase.files.map(&:product_ref).compact.first + expect(merged_bar.package_product_dependencies).to include(bar_reference) + expect(base_project.objects.count { |o| o.isa == "XCSwiftPackageProductDependency" }).to eq(2) + end + it "changes framework from reference proxy to file reference" do framework_filename = "baz"