From f26311be5d3429919beafe8795cce68b16fd6647 Mon Sep 17 00:00:00 2001 From: tom doron Date: Fri, 3 Sep 2021 11:12:24 -0700 Subject: [PATCH] relax strickter validation to warning to allow transition motivation: recent change to SwiftPM introduced strickter validation of dependencies uniquness to address undefined behavior. however, we want to allow users some times to adjust before making a breaking change changes: update the validation to emit a warning instead of error under the specific condition the previous behavior incorrectly ignored! --- .../PackageGraph/PackageGraph+Loading.swift | 18 +++- Sources/PackageModel/Manifest.swift | 2 +- Tests/WorkspaceTests/WorkspaceTests.swift | 97 ++++++++++++++++--- 3 files changed, 96 insertions(+), 21 deletions(-) diff --git a/Sources/PackageGraph/PackageGraph+Loading.swift b/Sources/PackageGraph/PackageGraph+Loading.swift index 5d83089274e..18f2a1eedce 100644 --- a/Sources/PackageGraph/PackageGraph+Loading.swift +++ b/Sources/PackageGraph/PackageGraph+Loading.swift @@ -216,7 +216,7 @@ private func createResolvedPackages( // Create a map of package builders keyed by the package identity. // This is guaranteed to be unique so we can use spm_createDictionary - let packageMapByIdentity: [PackageIdentity: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary{ + let packagesByIdentity: [PackageIdentity: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary{ return ($0.package.identity, $0) } @@ -242,7 +242,7 @@ private func createResolvedPackages( } // Otherwise, look it up by its identity. - if let resolvedPackage = packageMapByIdentity[dependency.identity] { + if let resolvedPackage = packagesByIdentity[dependency.identity] { // check if this resolved package already listed in the dependencies // this means that the dependencies share the same identity // FIXME: this works but the way we find out about this is based on a side effect, need to improve it @@ -255,16 +255,24 @@ private func createResolvedPackages( return diagnostics.emit(error, location: package.diagnosticLocation) } - // check if the resolved package url is the same as the dependency one + // check if the resolved package location is the same as the dependency one // if not, this means that the dependencies share the same identity - // FIXME: this works but the way we find out about this is based on a side effect, need to improve it + // which only allowed when overriding if resolvedPackage.package.manifest.packageLocation != dependencyLocation && !resolvedPackage.allowedToOverride { let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier( package: package.identity.description, dependencyLocation: dependencyLocation, otherDependencyURL: resolvedPackage.package.manifest.packageLocation, identity: dependency.identity) - return diagnostics.emit(error, location: package.diagnosticLocation) + // 9/2021 this is currently emitting a warning only to support + // backwards compatibility with older versions of SwiftPM that had too weak of a validation + // we will upgrade this to an error in a few versions to tighten up the validation + if dependency.explicitNameForTargetDependencyResolutionOnly == .none || + resolvedPackage.package.manifestName == dependency.explicitNameForTargetDependencyResolutionOnly { + diagnostics.emit(.warning(error.description + ". this will be upgraded to an error in future versions of SwiftPM."), location: package.diagnosticLocation) + } else { + return diagnostics.emit(error, location: package.diagnosticLocation) + } } // checks if two dependencies have the same explicit name which can cause target based dependency package lookup issue diff --git a/Sources/PackageModel/Manifest.swift b/Sources/PackageModel/Manifest.swift index fa97883972f..bdfec1f5f79 100644 --- a/Sources/PackageModel/Manifest.swift +++ b/Sources/PackageModel/Manifest.swift @@ -168,7 +168,7 @@ public final class Manifest: ObjectIdentifierProtocol { /// Returns the package dependencies required for a particular products filter. public func dependenciesRequired(for productFilter: ProductFilter) -> [PackageDependency] { #if ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION - // If we have already calcualted it, returned the cached value. + // If we have already calculated it, returned the cached value. if let dependencies = self._requiredDependencies[productFilter] { return self.dependencies } else { diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift index 38c178cc639..6bf06647723 100644 --- a/Tests/WorkspaceTests/WorkspaceTests.swift +++ b/Tests/WorkspaceTests/WorkspaceTests.swift @@ -5658,7 +5658,7 @@ final class WorkspaceTests: XCTestCase { ]) } - func testDuplicateDependencyIdentityAtRoot() throws { + func testDuplicateDependencyIdentityWithNameAtRoot() throws { let sandbox = AbsolutePath("/tmp/ws/") let fs = InMemoryFileSystem() @@ -5720,6 +5720,68 @@ final class WorkspaceTests: XCTestCase { } } + func testDuplicateDependencyIdentityWithoutNameAtRoot() 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: "FooUtilityPackage"), + .product(name: "BarProduct", package: "BarUtilityPackage") + ]), + ], + products: [], + dependencies: [ + .scm(path: "foo/utility", requirement: .upToNextMajor(from: "1.0.0")), + .scm(path: "bar/utility", requirement: .upToNextMajor(from: "1.0.0")), + ], + toolsVersion: .v5 + ), + ], + packages: [ + MockPackage( + name: "FooUtilityPackage", + path: "foo/utility", + targets: [ + MockTarget(name: "FooTarget"), + ], + products: [ + MockProduct(name: "FooProduct", targets: ["FooTarget"]), + ], + versions: ["1.0.0", "2.0.0"] + ), + // this package never gets loaded since the dependency declaration identity is the same as "FooPackage" + MockPackage( + name: "BarUtilityPackage", + path: "bar/utility", + 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: "'root' dependency on '/tmp/ws/pkgs/bar/utility' conflicts with dependency on '/tmp/ws/pkgs/foo/utility' which has the same identity 'utility'", + behavior: .error, + location: "'Root' /tmp/ws/roots/Root" + ) + } + } + } + func testDuplicateExplicitDependencyName_AtRoot() throws { let sandbox = AbsolutePath("/tmp/ws/") let fs = InMemoryFileSystem() @@ -6257,8 +6319,6 @@ final class WorkspaceTests: XCTestCase { ] ) - // FIXME: rdar://72940946 - // we need to improve this situation or diagnostics when working on identity workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in DiagnosticsEngineTester(diagnostics) { result in result.check( @@ -6282,8 +6342,8 @@ final class WorkspaceTests: XCTestCase { name: "Root", targets: [ MockTarget(name: "RootTarget", dependencies: [ - .product(name: "FooUtilityProduct", package: "FooUtilityPackage"), - .product(name: "BarProduct", package: "BarPackage") + .product(name: "FooUtilityProduct", package: "utility"), + .product(name: "BarProduct", package: "bar") ]), ], products: [], @@ -6311,7 +6371,7 @@ final class WorkspaceTests: XCTestCase { path: "bar", targets: [ MockTarget(name: "BarTarget", dependencies: [ - .product(name: "OtherUtilityProduct", package: "OtherUtilityPackage"), + .product(name: "OtherUtilityProduct", package: "utility"), ]), ], products: [ @@ -6337,12 +6397,19 @@ final class WorkspaceTests: XCTestCase { ] ) - // FIXME: rdar://72940946 - // we need to improve this situation or diagnostics when working on identity + // 9/2021 this is currently emitting a warning only to support backwards compatibility + // we will upgrade this to an error in a few versions to tighten up the validation workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in DiagnosticsEngineTester(diagnostics) { result in result.check( - diagnostic: "product 'OtherUtilityProduct' required by package 'bar' target 'BarTarget' not found in package 'OtherUtilityPackage'.", + diagnostic: "'bar' dependency on '/tmp/ws/pkgs/other-foo/utility' conflicts with dependency on '/tmp/ws/pkgs/foo/utility' which has the same identity 'utility'. this will be upgraded to an error in future versions of SwiftPM.", + behavior: .warning, + location: "'BarPackage' /tmp/ws/pkgs/bar" + ) + // FIXME: rdar://72940946 + // we need to improve this situation or diagnostics when working on identity + result.check( + diagnostic: "product 'OtherUtilityProduct' required by package 'bar' target 'BarTarget' not found in package 'utility'.", behavior: .error, location: "'BarPackage' /tmp/ws/pkgs/bar" ) @@ -6420,10 +6487,10 @@ final class WorkspaceTests: XCTestCase { ] ) - // FIXME: rdar://72940946 - // we need to improve this situation or diagnostics when working on identity workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in DiagnosticsEngineTester(diagnostics) { result in + // FIXME: rdar://72940946 + // we need to improve this situation or diagnostics when working on identity result.check( diagnostic: "cyclic dependency declaration found: Root -> FooUtilityPackage -> BarPackage -> FooUtilityPackage", behavior: .error, @@ -6505,10 +6572,10 @@ final class WorkspaceTests: XCTestCase { ] ) - // FIXME: rdar://72940946 - // we need to improve this situation or diagnostics when working on identity workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in DiagnosticsEngineTester(diagnostics) { result in + // FIXME: rdar://72940946 + // we need to improve this situation or diagnostics when working on identity result.check( diagnostic: "cyclic dependency declaration found: Root -> FooUtilityPackage -> BarPackage -> FooUtilityPackage", behavior: .error, @@ -6573,10 +6640,10 @@ final class WorkspaceTests: XCTestCase { ] ) - // FIXME: rdar://72940946 - // we need to improve this situation or diagnostics when working on identity workspace.checkPackageGraph(roots: ["foo"]) { graph, diagnostics in DiagnosticsEngineTester(diagnostics) { result in + // FIXME: rdar://72940946 + // we need to improve this situation or diagnostics when working on identity result.check( diagnostic: "cyclic dependency declaration found: Root -> BarPackage -> Root", behavior: .error,