From b26a7450a4086d3ffe03b4632b43254179f17e42 Mon Sep 17 00:00:00 2001 From: k-kohey Date: Mon, 25 Mar 2024 02:18:31 +0900 Subject: [PATCH 1/5] make allTargetNames Type Array --- .../PackageGraph/ModulesGraph+Loading.swift | 7 +-- .../PackageGraphTests/ModulesGraphTests.swift | 51 +++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index a056875684a..8a31a304328 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -429,7 +429,7 @@ private func createResolvedPackages( try dupProductsChecker.run(lookupByProductIDs: moduleAliasingUsed, observabilityScope: observabilityScope) // The set of all target names. - var allTargetNames = Set() + let allTargetNames = packageBuilders.reduce([]) { $0 + $1.targets }.map(\.target.name) // Track if multiple targets are found with the same name. var foundDuplicateTarget = false @@ -494,7 +494,8 @@ private func createResolvedPackages( // Establish dependencies in each target. for targetBuilder in packageBuilder.targets { // Record if we see a duplicate target. - foundDuplicateTarget = foundDuplicateTarget || !allTargetNames.insert(targetBuilder.target.name).inserted + let isDuplicateTarget = 1 < allTargetNames.filter { $0 == targetBuilder.target.name }.count + foundDuplicateTarget = foundDuplicateTarget || isDuplicateTarget // Directly add all the system module dependencies. targetBuilder.dependencies += implicitSystemTargetDeps.map { .target($0, conditions: []) } @@ -568,7 +569,7 @@ private func createResolvedPackages( // If a target with similar name was encountered before, we emit a diagnostic. if foundDuplicateTarget { var duplicateTargets = [String: [Package]]() - for targetName in allTargetNames.sorted() { + for targetName in Set(allTargetNames).sorted() { let packages = packageBuilders .filter({ $0.targets.contains(where: { $0.target.name == targetName }) }) .map{ $0.package } diff --git a/Tests/PackageGraphTests/ModulesGraphTests.swift b/Tests/PackageGraphTests/ModulesGraphTests.swift index efae57d71a4..420387d5d7d 100644 --- a/Tests/PackageGraphTests/ModulesGraphTests.swift +++ b/Tests/PackageGraphTests/ModulesGraphTests.swift @@ -2676,6 +2676,57 @@ final class ModulesGraphTests: XCTestCase { XCTAssertEqual(observability.diagnostics.count, 0, "unexpected diagnostics: \(observability.diagnostics.map { $0.description })") } + + func testDependencyResolutionWithErrorMessages() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/aaa/Sources/aaa/main.swift", + "/zzz/Sources/zzz/source.swift" + ) + + let observability = ObservabilitySystem.makeForTesting() + let _ = try loadModulesGraph( + fileSystem: fs, + manifests: [ + Manifest.createRootManifest( + displayName: "aaa", + path: "/aaa", + dependencies: [ + .localSourceControl(path: "/zzz", requirement: .upToNextMajor(from: "1.0.0")) + ], + products: [], + targets: [ + TargetDescription( + name: "aaa", + dependencies: ["zzy"], + type: .executable + ) + ]), + Manifest.createRootManifest( + displayName: "zzz", + path: "/zzz", + products: [ + ProductDescription( + name: "zzz", + type: .library(.automatic), + targets: ["zzz"] + ) + ], + targets: [ + TargetDescription( + name: "zzz" + ) + ]) + ], + observabilityScope: observability.topScope + ) + + testDiagnostics(observability.diagnostics) { result in + result.check( + diagnostic: "product 'zzy' required by package 'aaa' target 'aaa' not found. Did you mean 'zzz'?", + severity: .error + ) + } + } } From 3b12d72a140f1d0589c494b3bb9d52ef9c75e091 Mon Sep 17 00:00:00 2001 From: k-kohey Date: Mon, 25 Mar 2024 21:46:25 +0900 Subject: [PATCH 2/5] add "Did you mean '.product(...) to description" --- Sources/PackageGraph/ModulesGraph+Loading.swift | 11 ++++++++++- Sources/PackageGraph/ModulesGraph.swift | 8 +++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index 8a31a304328..9d00ed902b3 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -525,13 +525,22 @@ private func createResolvedPackages( } else { // Find a product name from the available product dependencies that is most similar to the required product name. let bestMatchedProductName = bestMatch(for: productRef.name, from: Array(allTargetNames)) + var packageContainingBestMatchedProduct: String? + if let bestMatchedProductName, productRef.name == bestMatchedProductName { + let dependentPackages = packageBuilder.dependencies.map(\.package) + for p in dependentPackages where p.targets.contains(where: { $0.name == bestMatchedProductName }) { + packageContainingBestMatchedProduct = p.identity.description + break + } + } let error = PackageGraphError.productDependencyNotFound( package: package.identity.description, targetName: targetBuilder.target.name, dependencyProductName: productRef.name, dependencyPackageName: productRef.package, dependencyProductInDecl: !declProductsAsDependency.isEmpty, - similarProductName: bestMatchedProductName + similarProductName: bestMatchedProductName, + packageContainingSimilarProduct: packageContainingBestMatchedProduct ) packageObservabilityScope.emit(error) } diff --git a/Sources/PackageGraph/ModulesGraph.swift b/Sources/PackageGraph/ModulesGraph.swift index 471d6192e5e..84f1bcd4907 100644 --- a/Sources/PackageGraph/ModulesGraph.swift +++ b/Sources/PackageGraph/ModulesGraph.swift @@ -23,7 +23,7 @@ enum PackageGraphError: Swift.Error { case cycleDetected((path: [Manifest], cycle: [Manifest])) /// The product dependency not found. - case productDependencyNotFound(package: String, targetName: String, dependencyProductName: String, dependencyPackageName: String?, dependencyProductInDecl: Bool, similarProductName: String?) + case productDependencyNotFound(package: String, targetName: String, dependencyProductName: String, dependencyPackageName: String?, dependencyProductInDecl: Bool, similarProductName: String?, packageContainingSimilarProduct: String?) /// The package dependency already satisfied by a different dependency package case dependencyAlreadySatisfiedByIdentifier(package: String, dependencyLocation: String, otherDependencyURL: String, identity: PackageIdentity) @@ -230,12 +230,14 @@ extension PackageGraphError: CustomStringConvertible { (cycle.path + cycle.cycle).map({ $0.displayName }).joined(separator: " -> ") + " -> " + cycle.cycle[0].displayName - case .productDependencyNotFound(let package, let targetName, let dependencyProductName, let dependencyPackageName, let dependencyProductInDecl, let similarProductName): + case .productDependencyNotFound(let package, let targetName, let dependencyProductName, let dependencyPackageName, let dependencyProductInDecl, let similarProductName, let packageContainingSimilarProduct): if dependencyProductInDecl { return "product '\(dependencyProductName)' is declared in the same package '\(package)' and can't be used as a dependency for target '\(targetName)'." } else { var description = "product '\(dependencyProductName)' required by package '\(package)' target '\(targetName)' \(dependencyPackageName.map{ "not found in package '\($0)'" } ?? "not found")." - if let similarProductName { + if let similarProductName, let packageContainingSimilarProduct { + description += " Did you mean '.product(name: \"\(similarProductName)\", package: \"\(packageContainingSimilarProduct)\")'?" + } else if let similarProductName { description += " Did you mean '\(similarProductName)'?" } return description From d9cb4affeaa17567e4a0692b13e4156c1185d64d Mon Sep 17 00:00:00 2001 From: k-kohey Date: Mon, 25 Mar 2024 21:48:50 +0900 Subject: [PATCH 3/5] fix comment --- Sources/PackageGraph/ModulesGraph+Loading.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index 9d00ed902b3..4e680fe2046 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -428,7 +428,7 @@ private func createResolvedPackages( ) try dupProductsChecker.run(lookupByProductIDs: moduleAliasingUsed, observabilityScope: observabilityScope) - // The set of all target names. + // The array of all target names. let allTargetNames = packageBuilders.reduce([]) { $0 + $1.targets }.map(\.target.name) // Track if multiple targets are found with the same name. From daa29acb75a9c11dbbe3e7a324865299119b4b16 Mon Sep 17 00:00:00 2001 From: k-kohey Date: Fri, 29 Mar 2024 21:12:10 +0900 Subject: [PATCH 4/5] Update Sources/PackageGraph/ModulesGraph+Loading.swift Co-authored-by: Yuta Saito --- Sources/PackageGraph/ModulesGraph+Loading.swift | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index 4e680fe2046..dd92e53fc19 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -428,12 +428,19 @@ private func createResolvedPackages( ) try dupProductsChecker.run(lookupByProductIDs: moduleAliasingUsed, observabilityScope: observabilityScope) - // The array of all target names. - let allTargetNames = packageBuilders.reduce([]) { $0 + $1.targets }.map(\.target.name) + // The set of all target names. + var allTargetNames = Set() // Track if multiple targets are found with the same name. var foundDuplicateTarget = false + for packageBuilder in packageBuilders { + for target in packageBuilder.targets { + // Record if we see a duplicate target. + foundDuplicateTarget = foundDuplicateTarget || !allTargetNames.insert(target.target.name).inserted + } + } + // Do another pass and establish product dependencies of each target. for packageBuilder in packageBuilders { let package = packageBuilder.package From 13f0a2a4641ee6ec065b401a60bf3a014e9403d7 Mon Sep 17 00:00:00 2001 From: k-kohey Date: Fri, 29 Mar 2024 21:16:24 +0900 Subject: [PATCH 5/5] these lines will be deleted as they have been moved elsewhere --- Sources/PackageGraph/ModulesGraph+Loading.swift | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index dd92e53fc19..5c0ab4e4ea1 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -500,10 +500,6 @@ private func createResolvedPackages( // Establish dependencies in each target. for targetBuilder in packageBuilder.targets { - // Record if we see a duplicate target. - let isDuplicateTarget = 1 < allTargetNames.filter { $0 == targetBuilder.target.name }.count - foundDuplicateTarget = foundDuplicateTarget || isDuplicateTarget - // Directly add all the system module dependencies. targetBuilder.dependencies += implicitSystemTargetDeps.map { .target($0, conditions: []) }