Skip to content

relax stricter validation to warning to allow transition #3713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 3, 2021
Merged

relax stricter validation to warning to allow transition #3713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions Sources/PackageGraph/PackageGraph+Loading.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Sources/PackageModel/Manifest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
97 changes: 82 additions & 15 deletions Tests/WorkspaceTests/WorkspaceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5658,7 +5658,7 @@ final class WorkspaceTests: XCTestCase {
])
}

func testDuplicateDependencyIdentityAtRoot() throws {
func testDuplicateDependencyIdentityWithNameAtRoot() throws {
let sandbox = AbsolutePath("/tmp/ws/")
let fs = InMemoryFileSystem()

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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(
Expand All @@ -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: [],
Expand Down Expand Up @@ -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: [
Expand All @@ -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"
)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down