Skip to content

Commit 98669ba

Browse files
tomerdtomerd
authored
improve package location validation (#3733)
motivation: new package location validation is a pure string comparison which is not subtle enough to address how git URLs can be expressed changes: * use the canonical location facilities to compare package locations * add more tests
1 parent 3590999 commit 98669ba

File tree

3 files changed

+245
-16
lines changed

3 files changed

+245
-16
lines changed

‎Sources/PackageGraph/PackageGraph+Loading.swift‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ private func createResolvedPackages(
258258
// check if the resolved package location is the same as the dependency one
259259
// if not, this means that the dependencies share the same identity
260260
// which only allowed when overriding
261-
if resolvedPackage.package.manifest.packageLocation != dependencyLocation && !resolvedPackage.allowedToOverride {
261+
if !LocationComparator.areEqual(resolvedPackage.package.manifest.packageLocation, dependencyLocation) && !resolvedPackage.allowedToOverride {
262262
let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
263263
package: package.identity.description,
264264
dependencyLocation: dependencyLocation,
@@ -269,7 +269,7 @@ private func createResolvedPackages(
269269
// we will upgrade this to an error in a few versions to tighten up the validation
270270
if dependency.explicitNameForTargetDependencyResolutionOnly == .none ||
271271
resolvedPackage.package.manifestName == dependency.explicitNameForTargetDependencyResolutionOnly {
272-
diagnostics.emit(.warning(error.description + ". this will be upgraded to an error in future versions of SwiftPM."), location: package.diagnosticLocation)
272+
diagnostics.emit(.warning(error.description + ". this will be escalated to an error in future versions of SwiftPM."), location: package.diagnosticLocation)
273273
} else {
274274
return diagnostics.emit(error, location: package.diagnosticLocation)
275275
}
@@ -652,3 +652,23 @@ fileprivate func findCycle(
652652
// Couldn't find any cycle in the graph.
653653
return nil
654654
}
655+
656+
657+
// TODO: model package location better / encapsulate into a new type (PackageLocation) so that such comparison is reusable
658+
// additionally move and rename CanonicalPackageIdentity to become a detail function of the PackageLocation abstraction, as it is not used otherwise
659+
struct LocationComparator {
660+
static func areEqual(_ lhs: String, _ rhs: String) -> Bool {
661+
if lhs == rhs {
662+
return true
663+
}
664+
665+
let canonicalLHS = CanonicalPackageIdentity(lhs)
666+
let canonicalRHS = CanonicalPackageIdentity(rhs)
667+
if canonicalLHS == canonicalRHS {
668+
return true
669+
}
670+
671+
return false
672+
}
673+
}
674+

‎Sources/PackageModel/PackageIdentity.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ struct LegacyPackageIdentity: PackageIdentityProvider, Equatable {
345345
/// ```
346346
/// file:///Users/mona/LinkedList → /Users/mona/LinkedList
347347
/// ```
348-
struct CanonicalPackageIdentity: PackageIdentityProvider, Equatable {
348+
public struct CanonicalPackageIdentity: PackageIdentityProvider, Equatable {
349349
/// A textual representation of this instance.
350350
public let description: String
351351

‎Tests/WorkspaceTests/WorkspaceTests.swift‎

Lines changed: 222 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6398,11 +6398,11 @@ final class WorkspaceTests: XCTestCase {
63986398
)
63996399

64006400
// 9/2021 this is currently emitting a warning only to support backwards compatibility
6401-
// we will upgrade this to an error in a few versions to tighten up the validation
6401+
// we will escalate this to an error in a few versions to tighten up the validation
64026402
workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
64036403
DiagnosticsEngineTester(diagnostics) { result in
64046404
result.check(
6405-
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.",
6405+
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 escalated to an error in future versions of SwiftPM.",
64066406
behavior: .warning,
64076407
location: "'BarPackage' /tmp/ws/pkgs/bar"
64086408
)
@@ -6417,7 +6417,151 @@ final class WorkspaceTests: XCTestCase {
64176417
}
64186418
}
64196419

6420-
func testDuplicateTransitiveIdentityGitHubURLs() throws {
6420+
func testDuplicateTransitiveIdentitySimilarURLs1() throws {
6421+
let sandbox = AbsolutePath("/tmp/ws/")
6422+
let fs = InMemoryFileSystem()
6423+
6424+
let workspace = try MockWorkspace(
6425+
sandbox: sandbox,
6426+
fs: fs,
6427+
roots: [
6428+
MockPackage(
6429+
name: "Root",
6430+
targets: [
6431+
MockTarget(name: "RootTarget", dependencies: [
6432+
.product(name: "FooProduct", package: "foo"),
6433+
.product(name: "BarProduct", package: "bar")
6434+
]),
6435+
],
6436+
products: [],
6437+
dependencies: [
6438+
.sourceControl(url: "https://github.com/foo/foo", requirement: .upToNextMajor(from: "1.0.0")),
6439+
.sourceControl(path: "bar", requirement: .upToNextMajor(from: "1.0.0")),
6440+
],
6441+
toolsVersion: .v5_6
6442+
),
6443+
],
6444+
packages: [
6445+
MockPackage(
6446+
name: "FooPackage",
6447+
url: "https://github.com/foo/foo",
6448+
targets: [
6449+
MockTarget(name: "FooTarget"),
6450+
],
6451+
products: [
6452+
MockProduct(name: "FooProduct", targets: ["FooTarget"]),
6453+
],
6454+
versions: ["1.0.0"]
6455+
),
6456+
MockPackage(
6457+
name: "BarPackage",
6458+
path: "bar",
6459+
targets: [
6460+
MockTarget(name: "BarTarget", dependencies: [
6461+
.product(name: "FooProduct", package: "foo"),
6462+
]),
6463+
],
6464+
products: [
6465+
MockProduct(name: "BarProduct", targets: ["BarTarget"]),
6466+
],
6467+
dependencies: [
6468+
.sourceControl(url: "https://github.com/foo/foo.git", requirement: .upToNextMajor(from: "1.0.0")),
6469+
],
6470+
versions: ["1.0.0"]
6471+
),
6472+
// this package never gets loaded since its identity is the same as "FooPackage"
6473+
MockPackage(
6474+
name: "FooPackage",
6475+
url: "https://github.com/foo/foo.git",
6476+
targets: [
6477+
MockTarget(name: "FooTarget"),
6478+
],
6479+
products: [
6480+
MockProduct(name: "FooProduct", targets: ["FooTarget"]),
6481+
],
6482+
versions: ["1.0.0"]
6483+
),
6484+
]
6485+
)
6486+
6487+
workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
6488+
XCTAssertNoDiagnostics(diagnostics)
6489+
}
6490+
}
6491+
6492+
func testDuplicateTransitiveIdentitySimilarURLs2() throws {
6493+
let sandbox = AbsolutePath("/tmp/ws/")
6494+
let fs = InMemoryFileSystem()
6495+
6496+
let workspace = try MockWorkspace(
6497+
sandbox: sandbox,
6498+
fs: fs,
6499+
roots: [
6500+
MockPackage(
6501+
name: "Root",
6502+
targets: [
6503+
MockTarget(name: "RootTarget", dependencies: [
6504+
.product(name: "FooProduct", package: "foo"),
6505+
.product(name: "BarProduct", package: "bar")
6506+
]),
6507+
],
6508+
products: [],
6509+
dependencies: [
6510+
.sourceControl(url: "https://github.com/foo/foo.git", requirement: .upToNextMajor(from: "1.0.0")),
6511+
.sourceControl(path: "bar", requirement: .upToNextMajor(from: "1.0.0")),
6512+
],
6513+
toolsVersion: .v5_6
6514+
),
6515+
],
6516+
packages: [
6517+
MockPackage(
6518+
name: "FooPackage",
6519+
url: "https://github.com/foo/foo.git",
6520+
targets: [
6521+
MockTarget(name: "FooTarget"),
6522+
],
6523+
products: [
6524+
MockProduct(name: "FooProduct", targets: ["FooTarget"]),
6525+
],
6526+
versions: ["1.0.0"]
6527+
),
6528+
MockPackage(
6529+
name: "BarPackage",
6530+
path: "bar",
6531+
targets: [
6532+
MockTarget(name: "BarTarget", dependencies: [
6533+
.product(name: "FooProduct", package: "foo"),
6534+
]),
6535+
],
6536+
products: [
6537+
MockProduct(name: "BarProduct", targets: ["BarTarget"]),
6538+
],
6539+
dependencies: [
6540+
.sourceControl(url: "http://github.com/foo/foo", requirement: .upToNextMajor(from: "1.0.0")),
6541+
],
6542+
versions: ["1.0.0"]
6543+
),
6544+
// this package never gets loaded since its identity is the same as "FooPackage"
6545+
MockPackage(
6546+
name: "FooPackage",
6547+
url: "http://github.com/foo/foo",
6548+
targets: [
6549+
MockTarget(name: "FooTarget"),
6550+
],
6551+
products: [
6552+
MockProduct(name: "FooProduct", targets: ["FooTarget"]),
6553+
],
6554+
versions: ["1.0.0"]
6555+
),
6556+
]
6557+
)
6558+
6559+
workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
6560+
XCTAssertNoDiagnostics(diagnostics)
6561+
}
6562+
}
6563+
6564+
func testDuplicateTransitiveIdentityGitHubURLs1() throws {
64216565
let sandbox = AbsolutePath("/tmp/ws/")
64226566
let fs = InMemoryFileSystem()
64236567

@@ -6484,15 +6628,80 @@ final class WorkspaceTests: XCTestCase {
64846628
]
64856629
)
64866630

6487-
// FIXME: this should not emit an error/warning - we should reconcile the URLS styles
64886631
workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
6489-
DiagnosticsEngineTester(diagnostics) { result in
6490-
result.check(
6491-
diagnostic: "'bar' dependency on 'git@github.com:foo/foo.git' conflicts with dependency on 'https://github.com/foo/foo.git' which has the same identity 'foo'. this will be upgraded to an error in future versions of SwiftPM.",
6492-
behavior: .warning,
6493-
location: "'BarPackage' /tmp/ws/pkgs/bar"
6494-
)
6495-
}
6632+
XCTAssertNoDiagnostics(diagnostics)
6633+
}
6634+
}
6635+
6636+
func testDuplicateTransitiveIdentityGitHubURLs2() throws {
6637+
let sandbox = AbsolutePath("/tmp/ws/")
6638+
let fs = InMemoryFileSystem()
6639+
6640+
let workspace = try MockWorkspace(
6641+
sandbox: sandbox,
6642+
fs: fs,
6643+
roots: [
6644+
MockPackage(
6645+
name: "Root",
6646+
targets: [
6647+
MockTarget(name: "RootTarget", dependencies: [
6648+
.product(name: "FooProduct", package: "foo"),
6649+
.product(name: "BarProduct", package: "bar")
6650+
]),
6651+
],
6652+
products: [],
6653+
dependencies: [
6654+
.sourceControl(url: "https://github.enterprise.com/foo/foo", requirement: .upToNextMajor(from: "1.0.0")),
6655+
.sourceControl(path: "bar", requirement: .upToNextMajor(from: "1.0.0")),
6656+
],
6657+
toolsVersion: .v5_6
6658+
),
6659+
],
6660+
packages: [
6661+
MockPackage(
6662+
name: "FooPackage",
6663+
url: "https://github.enterprise.com/foo/foo",
6664+
targets: [
6665+
MockTarget(name: "FooTarget"),
6666+
],
6667+
products: [
6668+
MockProduct(name: "FooProduct", targets: ["FooTarget"]),
6669+
],
6670+
versions: ["1.0.0"]
6671+
),
6672+
MockPackage(
6673+
name: "BarPackage",
6674+
path: "bar",
6675+
targets: [
6676+
MockTarget(name: "BarTarget", dependencies: [
6677+
.product(name: "FooProduct", package: "foo"),
6678+
]),
6679+
],
6680+
products: [
6681+
MockProduct(name: "BarProduct", targets: ["BarTarget"]),
6682+
],
6683+
dependencies: [
6684+
.sourceControl(url: "git@github.enterprise.com:foo/foo.git", requirement: .upToNextMajor(from: "1.0.0")),
6685+
],
6686+
versions: ["1.0.0"]
6687+
),
6688+
// this package never gets loaded since its identity is the same as "FooPackage"
6689+
MockPackage(
6690+
name: "FooPackage",
6691+
url: "git@github.enterprise.com:foo/foo.git",
6692+
targets: [
6693+
MockTarget(name: "FooTarget"),
6694+
],
6695+
products: [
6696+
MockProduct(name: "FooProduct", targets: ["FooTarget"]),
6697+
],
6698+
versions: ["1.0.0"]
6699+
),
6700+
]
6701+
)
6702+
6703+
workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
6704+
XCTAssertNoDiagnostics(diagnostics)
64966705
}
64976706
}
64986707

@@ -6564,11 +6773,11 @@ final class WorkspaceTests: XCTestCase {
65646773
)
65656774

65666775
// 9/2021 this is currently emitting a warning only to support backwards compatibility
6567-
// we will upgrade this to an error in a few versions to tighten up the validation
6776+
// we will escalate this to an error in a few versions to tighten up the validation
65686777
workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
65696778
DiagnosticsEngineTester(diagnostics) { result in
65706779
result.check(
6571-
diagnostic: "'bar' dependency on 'https://github.com/foo-moved/foo.git' conflicts with dependency on 'https://github.com/foo/foo.git' which has the same identity 'foo'. this will be upgraded to an error in future versions of SwiftPM.",
6780+
diagnostic: "'bar' dependency on 'https://github.com/foo-moved/foo.git' conflicts with dependency on 'https://github.com/foo/foo.git' which has the same identity 'foo'. this will be escalated to an error in future versions of SwiftPM.",
65726781
behavior: .warning,
65736782
location: "'BarPackage' /tmp/ws/pkgs/bar"
65746783
)

0 commit comments

Comments
 (0)