From e2799dafdf6329d1d48489e34a7ed8cb76eaef9d Mon Sep 17 00:00:00 2001 From: tomer doron Date: Thu, 25 Feb 2021 17:35:25 -0800 Subject: [PATCH] improve and simplify validation of target based dependencies (#3280) motivation: SwiftPM 5.2 introduced target based dependencies which added complexity at code level and to users, this PR tries to both simplify the "rules" and the code changes: * allow target dependency in the form of ".product(name:, package:)" where package parameter is the last segment of the dependecy URL, which is the most intuitive choice * change validation code to encourage the above from instead of encouraging adding "name" attibute to the dependency decleration, as we want to get away from adding this attribute in the long run * add several tests that capture the numerous permutations we are coding for --- .../PackageGraph/PackageGraph+Loading.swift | 33 +- Sources/PackageGraph/PackageGraph.swift | 33 +- Sources/PackageModel/Manifest.swift | 2 +- .../PackageGraphTests/PackageGraphTests.swift | 658 +++++++++++++++++- Tests/WorkspaceTests/WorkspaceTests.swift | 136 +++- 5 files changed, 787 insertions(+), 75 deletions(-) diff --git a/Sources/PackageGraph/PackageGraph+Loading.swift b/Sources/PackageGraph/PackageGraph+Loading.swift index 12038425d7c..d85ba5f6228 100644 --- a/Sources/PackageGraph/PackageGraph+Loading.swift +++ b/Sources/PackageGraph/PackageGraph+Loading.swift @@ -141,7 +141,6 @@ extension PackageGraph { ) let rootPackages = resolvedPackages.filter{ rootManifestSet.contains($0.manifest) } - checkAllDependenciesAreUsed(rootPackages, diagnostics) return try PackageGraph( @@ -414,37 +413,23 @@ private func createResolvedPackages( continue } - // If package name is mentioned, ensure it is valid. - if let packageName = productRef.package { - // Find the declared package and check that it contains - // the product we found above. - guard let dependencyPackage = packageMapByNameForTargetDependencyResolutionOnly[packageName], dependencyPackage.products.contains(product) else { - let error = PackageGraphError.productDependencyIncorrectPackage( - name: productRef.name, - package: packageName - ) - diagnostics.emit(error, location: diagnosticLocation()) - continue - } - } else if packageBuilder.package.manifest.toolsVersion >= .v5_2 { - // Starting in 5.2, and target-based dependency, we require target product dependencies to - // explicitly reference the package containing the product, or for the product, package and - // dependency to share the same name. We don't check this in manifest loading for root-packages so - // we can provide a more detailed diagnostic here. + // Starting in 5.2, and target-based dependency, we require target product dependencies to + // explicitly reference the package containing the product, or for the product, package and + // dependency to share the same name. We don't check this in manifest loading for root-packages so + // we can provide a more detailed diagnostic here. + if packageBuilder.package.manifest.toolsVersion >= .v5_2 && productRef.package == nil{ let referencedPackageIdentity = identityResolver.resolveIdentity(for: product.packageBuilder.package.manifest.packageLocation) - guard let packageDependency = (packageBuilder.package.manifest.dependencies.first { package in + guard let referencedPackageDependency = (packageBuilder.package.manifest.dependencies.first { package in return package.identity == referencedPackageIdentity }) else { throw InternalError("dependency reference for \(product.packageBuilder.package.manifest.packageLocation) not found") } - - let packageName = product.packageBuilder.package.name - if productRef.name != packageDependency.nameForTargetDependencyResolutionOnly || packageDependency.nameForTargetDependencyResolutionOnly != packageName { + let referencedPackageName = referencedPackageDependency.nameForTargetDependencyResolutionOnly + if productRef.name != referencedPackageName { let error = PackageGraphError.productDependencyMissingPackage( productName: productRef.name, targetName: targetBuilder.target.name, - packageName: packageName, - packageDependency: packageDependency + packageDependency: referencedPackageDependency ) diagnostics.emit(error, location: diagnosticLocation()) } diff --git a/Sources/PackageGraph/PackageGraph.swift b/Sources/PackageGraph/PackageGraph.swift index 7c72b53159a..bff225fb227 100644 --- a/Sources/PackageGraph/PackageGraph.swift +++ b/Sources/PackageGraph/PackageGraph.swift @@ -21,9 +21,6 @@ enum PackageGraphError: Swift.Error { /// The product dependency not found. case productDependencyNotFound(dependencyProductName: String, dependencyPackageName: String?, packageName: String, targetName: String) - /// The product dependency was found but the package name did not match. - case productDependencyIncorrectPackage(name: String, package: String) - /// The package dependency name does not match the package name. case incorrectPackageDependencyName(dependencyPackageName: String, dependencyName: String, dependencyLocation: String, resolvedPackageName: String, resolvedPackageURL: String) @@ -37,7 +34,6 @@ enum PackageGraphError: Swift.Error { case productDependencyMissingPackage( productName: String, targetName: String, - packageName: String, packageDependency: PackageDependencyDescription ) @@ -173,9 +169,6 @@ extension PackageGraphError: CustomStringConvertible { case .productDependencyNotFound(let dependencyProductName, let dependencyPackageName, let packageName, let targetName): return "product '\(dependencyProductName)' \(dependencyPackageName.map{ "not found in package '\($0)'" } ?? "not found"). it is required by package '\(packageName)' target '\(targetName)'." - case .productDependencyIncorrectPackage(let name, let package): - return "product dependency '\(name)' in package '\(package)' not found" - case .incorrectPackageDependencyName(let dependencyPackageName, let dependencyName, let dependencyURL, let resolvedPackageName, let resolvedPackageURL): return """ '\(dependencyPackageName)' dependency on '\(dependencyURL)' has an explicit name '\(dependencyName)' which does not match the \ @@ -191,32 +184,14 @@ extension PackageGraphError: CustomStringConvertible { case .productDependencyMissingPackage( let productName, let targetName, - let packageName, let packageDependency ): - var solutionSteps: [String] = [] - - // If the package dependency name is the same as the package name, or if the product name and package name - // don't correspond, we need to rewrite the target dependency to explicit specify the package name. - if packageDependency.nameForTargetDependencyResolutionOnly == packageName || productName != packageName { - solutionSteps.append(""" - reference the package in the target dependency with '.product(name: "\(productName)", package: \ - "\(packageName)")' - """) - } - - // If the name of the product and the package are the same, or if the package dependency implicit name - // deduced from the URL is not correct, we need to rewrite the package dependency declaration to specify the - // package name. - if productName == packageName || packageDependency.nameForTargetDependencyResolutionOnly != packageName { - let dependencySwiftRepresentation = packageDependency.swiftRepresentation(overridingName: packageName) - solutionSteps.append(""" - provide the name of the package dependency with '\(dependencySwiftRepresentation)' - """) - } + let solution = """ + reference the package in the target dependency with '.product(name: "\(productName)", package: \ + "\(packageDependency.nameForTargetDependencyResolutionOnly)")' + """ - let solution = solutionSteps.joined(separator: " and ") return "dependency '\(productName)' in target '\(targetName)' requires explicit declaration; \(solution)" case .duplicateProduct(let product, let packages): diff --git a/Sources/PackageModel/Manifest.swift b/Sources/PackageModel/Manifest.swift index aac662ba0ff..4aa4ba6a0e4 100644 --- a/Sources/PackageModel/Manifest.swift +++ b/Sources/PackageModel/Manifest.swift @@ -272,7 +272,7 @@ public final class Manifest: ObjectIdentifierProtocol { return nil } - return dependencies.first(where: { $0.nameForTargetDependencyResolutionOnly == packageName }) + return self.dependencies.first(where: { $0.nameForTargetDependencyResolutionOnly == packageName }) } /// Registers a required product with a particular dependency if possible, or registers it as unknown. diff --git a/Tests/PackageGraphTests/PackageGraphTests.swift b/Tests/PackageGraphTests/PackageGraphTests.swift index 7fe5491d855..718706d76d9 100644 --- a/Tests/PackageGraphTests/PackageGraphTests.swift +++ b/Tests/PackageGraphTests/PackageGraphTests.swift @@ -674,19 +674,27 @@ class PackageGraphTests: XCTestCase { ) DiagnosticsEngineTester(diagnostics) { result in - result.checkUnordered(diagnostic: """ - dependency 'BarLib' in target 'Foo' requires explicit declaration; reference the package in the target \ - dependency with '.product(name: "BarLib", package: "Bar")' - """, behavior: .error, location: "'Foo' /Foo") - result.checkUnordered(diagnostic: """ - dependency 'Biz' in target 'Foo' requires explicit declaration; provide the name of the package \ - dependency with '.package(name: "Biz", url: "/BizPath", .exact("1.2.3"))' - """, behavior: .error, location: "'Foo' /Foo") - result.checkUnordered(diagnostic: """ - dependency 'FizLib' in target 'Foo' requires explicit declaration; reference the package in the target \ - dependency with '.product(name: "FizLib", package: "Fiz")' and provide the name of the package \ - dependency with '.package(name: "Fiz", url: "/FizPath", from: "1.1.2")' - """, behavior: .error, location: "'Foo' /Foo") + result.checkUnordered( + diagnostic: """ + dependency 'BarLib' in target 'Foo' requires explicit declaration; reference the package in the target dependency with '.product(name: "BarLib", package: "Bar")' + """, + behavior: .error, + location: "'Foo' /Foo" + ) + result.checkUnordered( + diagnostic: """ + dependency 'Biz' in target 'Foo' requires explicit declaration; reference the package in the target dependency with '.product(name: "Biz", package: "BizPath")' + """, + behavior: .error, + location: "'Foo' /Foo" + ) + result.checkUnordered( + diagnostic: """ + dependency 'FizLib' in target 'Foo' requires explicit declaration; reference the package in the target dependency with '.product(name: "FizLib", package: "FizPath")' + """, + behavior: .error, + location: "'Foo' /Foo" + ) } } @@ -1255,4 +1263,628 @@ class PackageGraphTests: XCTestCase { let store = try PinsStore(pinsFile: AbsolutePath("/pins"), fileSystem: fs) XCTAssertThrows(StringError("duplicated entry for package \"Yams\""), { try store.restore(from: json) }) } + + func testTargetDependencies_Pre52() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Bar/Sources/Bar/bar.swift" + ) + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, + manifests: [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5, + dependencies: [ + .scm(location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["Bar"]), + ]), + Manifest.createManifest( + name: "Bar", + path: "/Bar", + packageKind: .local, + packageLocation: "/Bar", + v: .v5, + products: [ + ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + ) + + XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)") + } + + func testTargetDependencies_Pre52_UnknownProduct() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Bar/Sources/Bar/bar.swift" + ) + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, + manifests: [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5, + dependencies: [ + .scm(location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["Unknown"]), + ]), + Manifest.createManifest( + name: "Bar", + path: "/Bar", + packageKind: .local, + packageLocation: "/Bar", + v: .v5, + products: [ + ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + ) + + DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in + result.check( + diagnostic: """ + product 'Unknown' not found. it is required by package 'Foo' target 'Foo'. + """, + behavior: .error, + location: "'Foo' /Foo" + ) + } + } + + func testTargetDependencies_Post52_NamesAligned() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Bar/Sources/Bar/bar.swift" + ) + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, + manifests: [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5_2, + dependencies: [ + .scm(location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["Bar"]), + ]), + Manifest.createManifest( + name: "Bar", + path: "/Bar", + packageKind: .local, + packageLocation: "/Bar", + v: .v5_2, + products: [ + ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + ) + + XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)") + } + + func testTargetDependencies_Post52_UnknownProduct() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Bar/Sources/Bar/bar.swift" + ) + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, + manifests: [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5_2, + dependencies: [ + .scm(location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["Unknown"]), + ]), + Manifest.createManifest( + name: "Bar", + path: "/Bar", + packageKind: .local, + packageLocation: "/Bar", + v: .v5_2, + products: [ + ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + ) + + DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in + result.check( + diagnostic: """ + product 'Unknown' not found. it is required by package 'Foo' target 'Foo'. + """, + behavior: .error, + location: "'Foo' /Foo" + ) + } + } + + func testTargetDependencies_Post52_ProductPackageNoMatch() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Bar/Sources/Bar/bar.swift" + ) + + let manifests = try [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5_2, + dependencies: [ + .scm(location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["ProductBar"]), + ]), + Manifest.createManifest( + name: "Bar", + path: "/Bar", + packageKind: .local, + packageLocation: "/Bar", + v: .v5_2, + products: [ + ProductDescription(name: "ProductBar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + + do { + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: manifests) + DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in + result.check( + diagnostic: """ + dependency 'ProductBar' in target 'Foo' requires explicit declaration; reference the package in the target dependency with '.product(name: "ProductBar", package: "Bar")' + """, + behavior: .error, + location: "'Foo' /Foo" + ) + } + } + + // fixit + + do { + let fixedManifests = try [ + manifests[0].withTargets([ + TargetDescription(name: "Foo", dependencies: [.product(name: "ProductBar", package: "Bar")]), + ]), + manifests[1] // same + ] + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: fixedManifests) + XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)") + } + } + + + // TODO: remove this when we remove explicit dependency name + func testTargetDependencies_Post52_ProductPackageNoMatch_DependencyExplicitName() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Bar/Sources/Bar/bar.swift" + ) + + let manifests = try [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5_2, + dependencies: [ + .scm(name: "Bar", location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["ProductBar"]), + ]), + Manifest.createManifest( + name: "Bar", + path: "/Bar", + packageKind: .local, + packageLocation: "/Bar", + v: .v5_2, + products: [ + ProductDescription(name: "ProductBar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + + do { + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: manifests) + DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in + result.check( + diagnostic: """ + dependency 'ProductBar' in target 'Foo' requires explicit declaration; reference the package in the target dependency with '.product(name: "ProductBar", package: "Bar")' + """, + behavior: .error, + location: "'Foo' /Foo" + ) + } + } + + + // fixit + + do { + let fixedManifests = try [ + manifests[0].withTargets([ + TargetDescription(name: "Foo", dependencies: [.product(name: "ProductBar", package: "Bar")]), + ]), + manifests[1] // same + ] + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: fixedManifests) + XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)") + } + } + + func testTargetDependencies_Post52_LocationAndManifestNameDontMatch() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Some-Bar/Sources/Bar/bar.swift" + ) + + let manifests = try [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5_2, + dependencies: [ + .scm(location: "/Some-Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["Bar"]), + ]), + Manifest.createManifest( + name: "Bar", + path: "/Some-Bar", + packageKind: .local, + packageLocation: "/Some-Bar", + v: .v5_2, + products: [ + ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + + do { + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: manifests) + DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in + result.check( + diagnostic: """ + dependency 'Bar' in target 'Foo' requires explicit declaration; reference the package in the target dependency with '.product(name: "Bar", package: "Some-Bar")' + """, + behavior: .error, + location: "'Foo' /Foo" + ) + } + } + + // fixit + + do { + let fixedManifests = try [ + manifests[0].withTargets([ + TargetDescription(name: "Foo", dependencies: [.product(name: "Bar", package: "Some-Bar")]), + ]), + manifests[1] // same + ] + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: fixedManifests) + XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)") + } + } + + func testTargetDependencies_Post52_LocationAndManifestNameDontMatch_ProductPackageDontMatch() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Some-Bar/Sources/Bar/bar.swift" + ) + + let manifests = try [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5_2, + dependencies: [ + .scm(location: "/Some-Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["ProductBar"]), + ]), + Manifest.createManifest( + name: "Bar", + path: "/Some-Bar", + packageKind: .local, + packageLocation: "/Some-Bar", + v: .v5_2, + products: [ + ProductDescription(name: "ProductBar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + + do { + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: manifests) + DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in + result.check( + diagnostic: """ + dependency 'ProductBar' in target 'Foo' requires explicit declaration; reference the package in the target dependency with '.product(name: "ProductBar", package: "Some-Bar")' + """, + behavior: .error, + location: "'Foo' /Foo" + ) + } + } + + // fix it + + do { + let fixedManifests = try [ + manifests[0].withTargets([ + TargetDescription(name: "Foo", dependencies: [.product(name: "ProductBar", package: "Foo-Bar")]), + ]), + manifests[1] // same + ] + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: fixedManifests) + XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)") + } + } + + // test backwards compatibility 5.2 < 5.4 + // TODO: remove this when we remove explicit dependency name + func testTargetDependencies_Post52_LocationAndManifestNameDontMatch_WithDependencyName() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Some-Bar/Sources/Bar/bar.swift" + ) + + let manifests = try [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5_2, + dependencies: [ + .scm(name: "Bar", location: "/Some-Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["Bar"]), + ]), + Manifest.createManifest( + name: "Bar", + path: "/Some-Bar", + packageKind: .local, + packageLocation: "/Some-Bar", + v: .v5_2, + products: [ + ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: manifests) + XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)") + } + + // test backwards compatibility 5.2 < 5.4 + // TODO: remove this when we remove explicit dependency name + func testTargetDependencies_Post52_LocationAndManifestNameDontMatch_ProductPackageDontMatch_WithDependencyName() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Some-Bar/Sources/Bar/bar.swift" + ) + + let manifests = try [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5_2, + dependencies: [ + .scm(name: "Bar", location: "/Some-Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["ProductBar"]), + ]), + Manifest.createManifest( + name: "Bar", + path: "/Some-Bar", + packageKind: .local, + packageLocation: "/Some-Bar", + v: .v5_2, + products: [ + ProductDescription(name: "ProductBar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + + do { + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: manifests) + DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in + result.check( + diagnostic: """ + dependency 'ProductBar' in target 'Foo' requires explicit declaration; reference the package in the target dependency with '.product(name: "ProductBar", package: "Bar")' + """, + behavior: .error, + location: "'Foo' /Foo" + ) + } + } + + // fix it + + do { + let fixedManifests = try [ + manifests[0].withTargets([ + TargetDescription(name: "Foo", dependencies: [.product(name: "ProductBar", package: "Some-Bar")]), + ]), + manifests[1] // same + ] + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: fixedManifests) + XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)") + } + } + + func testTargetDependencies_Post52_ManifestNameNotMatchedWithURL() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Foo/Sources/Foo/foo.swift", + "/Bar/Sources/Bar/bar.swift" + ) + + let manifests = try [ + Manifest.createManifest( + name: "Foo", + path: "/Foo", + packageKind: .root, + packageLocation: "/Foo", + v: .v5_2, + dependencies: [ + .scm(name: "Bar", location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + targets: [ + TargetDescription(name: "Foo", dependencies: ["ProductBar"]), + ]), + Manifest.createManifest( + name: "Some-Bar", + path: "/Bar", + packageKind: .local, + packageLocation: "/Bar", + v: .v5_2, + products: [ + ProductDescription(name: "ProductBar", type: .library(.automatic), targets: ["Bar"]) + ], + targets: [ + TargetDescription(name: "Bar"), + ]), + ] + + do { + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: manifests) + DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in + result.check( + diagnostic: """ + 'Foo' dependency on '/Bar' has an explicit name 'Bar' which does not match the name 'Some-Bar' set for '/Bar' + """, + behavior: .error, + location: "'Foo' /Foo" + ) + } + } + + // fix it + + do { + let fixedManifests = [ + try manifests[0].withDependencies([ + .scm(name: "Some-Bar", location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + ]).withTargets([ + TargetDescription(name: "Foo", dependencies: [.product(name: "ProductBar", package: "Some-Bar")]), + ]), + manifests[1] // same + ] + + let diagnostics = DiagnosticsEngine() + _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: fixedManifests) + XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)") + } + } +} + + +extension Manifest { + func withTargets(_ targets: [TargetDescription]) -> Manifest { + Manifest.createManifest( + name: self.name, + path: self.path.parentDirectory.pathString, + packageKind: self.packageKind, + packageLocation: self.packageLocation, + v: self.toolsVersion, + dependencies: self.dependencies, + targets: targets + ) + } + + func withDependencies(_ dependencies: [PackageDependencyDescription]) -> Manifest { + Manifest.createManifest( + name: self.name, + path: self.path.parentDirectory.pathString, + packageKind: self.packageKind, + packageLocation: self.packageLocation, + v: self.toolsVersion, + dependencies: dependencies, + targets: self.targets + ) + } } diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift index 3ca91b60704..732903934a8 100644 --- a/Tests/WorkspaceTests/WorkspaceTests.swift +++ b/Tests/WorkspaceTests/WorkspaceTests.swift @@ -5746,8 +5746,7 @@ final class WorkspaceTests: XCTestCase { } } - // this test is currently demonstrating bad behavior that would be fixed with https://github.com/apple/swift-package-manager/pull/3280 - /*func testDuplicateManifestNameAtRoot_ExplicitTargetDependecy() throws { + func testDuplicateManifestNameAtRoot_ExplicitTargetDependecy() throws { let sandbox = AbsolutePath("/tmp/ws/") let fs = InMemoryFileSystem() @@ -5802,7 +5801,7 @@ final class WorkspaceTests: XCTestCase { XCTAssert(diagnostics.diagnostics.isEmpty, diagnostics.description) } } - }*/ + } func testExplicitDependencyNameAndManifestNameMismatchAtRoot() throws { let sandbox = AbsolutePath("/tmp/ws/") @@ -5870,8 +5869,7 @@ final class WorkspaceTests: XCTestCase { } } - // this test is currently demonstrating bad behavior that would be fixed with https://github.com/apple/swift-package-manager/pull/3280 - /*func testManifestNameAndIdentityConflictAtRoot() throws { + func testManifestNameAndIdentityConflictAtRoot_Pre52() throws { let sandbox = AbsolutePath("/tmp/ws/") let fs = InMemoryFileSystem() @@ -5883,8 +5881,8 @@ final class WorkspaceTests: XCTestCase { name: "Root", targets: [ MockTarget(name: "RootTarget", dependencies: [ - "FooPackage", - "BarPackage" + "FooProduct", + "BarProduct" ]), ], products: [], @@ -5927,7 +5925,129 @@ final class WorkspaceTests: XCTestCase { } } } - */ + + func testManifestNameAndIdentityConflictAtRoot_Post52_Incorrect() throws { + let sandbox = AbsolutePath("/tmp/ws/") + let fs = InMemoryFileSystem() + + let workspace = try MockWorkspace( + sandbox: sandbox, + fs: fs, + roots: [ + MockPackage( + name: "Root", + targets: [ + MockTarget(name: "RootTarget", dependencies: [ + "FooProduct", + "BarProduct" + ]), + ], + products: [], + dependencies: [ + .git(path: "foo", requirement: .upToNextMajor(from: "1.0.0")), + .git(path: "bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + toolsVersion: .v5_3 + ), + ], + packages: [ + MockPackage( + name: "foo", + path: "foo", + targets: [ + MockTarget(name: "FooTarget"), + ], + products: [ + MockProduct(name: "FooProduct", targets: ["FooTarget"]), + ], + versions: ["1.0.0", "2.0.0"] + ), + MockPackage( + name: "foo", + path: "bar", + targets: [ + MockTarget(name: "BarTarget"), + ], + products: [ + MockProduct(name: "BarProduct", targets: ["BarTarget"]), + ], + versions: ["1.0.0", "2.0.0"] + ), + ] + ) + + workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in + DiagnosticsEngineTester(diagnostics) { result in + result.check( + diagnostic: "dependency 'FooProduct' in target 'RootTarget' requires explicit declaration; reference the package in the target dependency with '.product(name: \"FooProduct\", package: \"foo\")'", + behavior: .error, + location: "'Root' /tmp/ws/roots/Root" + ) + result.check( + diagnostic: "dependency 'BarProduct' in target 'RootTarget' requires explicit declaration; reference the package in the target dependency with '.product(name: \"BarProduct\", package: \"bar\")'", + behavior: .error, + location: "'Root' /tmp/ws/roots/Root" + ) + } + } + } + + func testManifestNameAndIdentityConflictAtRoot_Post52_Correct() throws { + let sandbox = AbsolutePath("/tmp/ws/") + let fs = InMemoryFileSystem() + + let workspace = try MockWorkspace( + sandbox: sandbox, + fs: fs, + roots: [ + MockPackage( + name: "Root", + targets: [ + MockTarget(name: "RootTarget", dependencies: [ + .product(name: "FooProduct", package: "foo"), + .product(name: "BarProduct", package: "bar"), + ]), + ], + products: [], + dependencies: [ + .git(path: "foo", requirement: .upToNextMajor(from: "1.0.0")), + .git(path: "bar", requirement: .upToNextMajor(from: "1.0.0")), + ], + toolsVersion: .v5_3 + ), + ], + packages: [ + MockPackage( + name: "foo", + path: "foo", + targets: [ + MockTarget(name: "FooTarget"), + ], + products: [ + MockProduct(name: "FooProduct", targets: ["FooTarget"]), + ], + versions: ["1.0.0", "2.0.0"] + ), + MockPackage( + name: "foo", + path: "bar", + targets: [ + MockTarget(name: "BarTarget"), + ], + products: [ + MockProduct(name: "BarProduct", targets: ["BarTarget"]), + ], + versions: ["1.0.0", "2.0.0"] + ), + ] + ) + + workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in + DiagnosticsEngineTester(diagnostics) { result in + XCTAssert(diagnostics.diagnostics.isEmpty, diagnostics.description) + } + } + } func testManifestNameAndIdentityConflictWithExplicitDependencyNamesAtRoot() throws { let sandbox = AbsolutePath("/tmp/ws/")