Skip to content

Commit 5a1ac27

Browse files
tomerdtomerd
authored
make package.resolved more deterministic in edge cases (#3967)
motivation: in some cases like two URLs with casing differences, package.resolved could be non-deterministic changes: * base managed dependencies lookup on canonical location (case insensitive, ignore .git suffix) * refactor how package.resolved is saved to test for non material differences * adjust call sites that manipulate the pin store to be more efficient about when to save the resolved file * add warning when expected dependency that should be stored in the resolved file is not found * rename CanonicalPackageIdentity as CanonicalPackageLocation and use it to compare locations where required * rename LegacyPackageIdentity as PackageIdentityParser and simplify how identity parsing is wired (removed unusued complexity) * adjust call sites * add and adjust test
1 parent 2112045 commit 5a1ac27

14 files changed

+661
-238
lines changed

‎Sources/PackageCollections/PackageCollections.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
571571
let matches: [PackageCollectionsModel.Package]
572572
if let location = location {
573573
// A package identity can be associated with multiple repository URLs
574-
matches = packagesCollections.packages.filter { CanonicalPackageIdentity($0.location) == CanonicalPackageIdentity(location) }
574+
matches = packagesCollections.packages.filter { CanonicalPackageLocation($0.location) == CanonicalPackageLocation(location) }
575575
}
576576
else {
577577
matches = packagesCollections.packages

‎Sources/PackageGraph/PackageGraph+Loading.swift

Lines changed: 10 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -236,21 +236,7 @@ private func createResolvedPackages(
236236

237237
// Establish the manifest-declared package dependencies.
238238
package.manifest.dependenciesRequired(for: packageBuilder.productFilter).forEach { dependency in
239-
// FIXME: change this validation logic to use identity instead of location
240-
let dependencyLocation: String
241-
switch dependency {
242-
case .fileSystem(let settings):
243-
dependencyLocation = settings.path.pathString
244-
case .sourceControl(let settings):
245-
switch settings.location {
246-
case .local(let path):
247-
dependencyLocation = path.pathString
248-
case .remote(let url):
249-
dependencyLocation = url.absoluteString
250-
}
251-
case .registry(let settings):
252-
dependencyLocation = settings.identity.description
253-
}
239+
let dependencyPackageRef = dependency.createPackageRef()
254240

255241
// Otherwise, look it up by its identity.
256242
if let resolvedPackage = packagesByIdentity[dependency.identity] {
@@ -260,7 +246,7 @@ private func createResolvedPackages(
260246
guard dependencies[resolvedPackage.package.identity] == nil else {
261247
let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
262248
package: package.identity.description,
263-
dependencyLocation: dependencyLocation,
249+
dependencyLocation: dependencyPackageRef.locationString,
264250
otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
265251
identity: dependency.identity)
266252
return packageObservabilityScope.emit(error)
@@ -269,10 +255,10 @@ private func createResolvedPackages(
269255
// check if the resolved package location is the same as the dependency one
270256
// if not, this means that the dependencies share the same identity
271257
// which only allowed when overriding
272-
if !LocationComparator.areEqual(resolvedPackage.package.manifest.packageLocation, dependencyLocation) && !resolvedPackage.allowedToOverride {
258+
if resolvedPackage.package.manifest.canonicalPackageLocation != dependencyPackageRef.canonicalLocation && !resolvedPackage.allowedToOverride {
273259
let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
274260
package: package.identity.description,
275-
dependencyLocation: dependencyLocation,
261+
dependencyLocation: dependencyPackageRef.locationString,
276262
otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
277263
identity: dependency.identity)
278264
// 9/2021 this is currently emitting a warning only to support
@@ -284,14 +270,18 @@ private func createResolvedPackages(
284270
} else {
285271
return packageObservabilityScope.emit(error)
286272
}
273+
} else if resolvedPackage.package.manifest.canonicalPackageLocation == dependencyPackageRef.canonicalLocation &&
274+
resolvedPackage.package.manifest.packageLocation != dependencyPackageRef.locationString &&
275+
!resolvedPackage.allowedToOverride {
276+
packageObservabilityScope.emit(info: "dependency on '\(resolvedPackage.package.identity)' is represented by similar locations ('\(resolvedPackage.package.manifest.packageLocation)' and '\(dependencyPackageRef.locationString)') which are treated as the same canonical location '\(dependencyPackageRef.canonicalLocation)'.")
287277
}
288278

289279
// checks if two dependencies have the same explicit name which can cause target based dependency package lookup issue
290280
if let explicitDependencyName = dependency.explicitNameForTargetDependencyResolutionOnly {
291281
if let previouslyResolvedPackage = dependenciesByNameForTargetDependencyResolution[explicitDependencyName] {
292282
let error = PackageGraphError.dependencyAlreadySatisfiedByName(
293283
package: package.identity.description,
294-
dependencyLocation: dependencyLocation,
284+
dependencyLocation: dependencyPackageRef.locationString,
295285
otherDependencyURL: previouslyResolvedPackage.package.manifest.packageLocation,
296286
name: explicitDependencyName)
297287
return packageObservabilityScope.emit(error)
@@ -302,7 +292,7 @@ private func createResolvedPackages(
302292
if let previouslyResolvedPackage = dependenciesByNameForTargetDependencyResolution[dependency.identity.description] {
303293
let error = PackageGraphError.dependencyAlreadySatisfiedByName(
304294
package: package.identity.description,
305-
dependencyLocation: dependencyLocation,
295+
dependencyLocation: dependencyPackageRef.locationString,
306296
otherDependencyURL: previouslyResolvedPackage.package.manifest.packageLocation,
307297
name: dependency.identity.description)
308298
return packageObservabilityScope.emit(error)
@@ -674,22 +664,3 @@ fileprivate func findCycle(
674664
// Couldn't find any cycle in the graph.
675665
return nil
676666
}
677-
678-
679-
// TODO: model package location better / encapsulate into a new type (PackageLocation) so that such comparison is reusable
680-
// additionally move and rename CanonicalPackageIdentity to become a detail function of the PackageLocation abstraction, as it is not used otherwise
681-
struct LocationComparator {
682-
static func areEqual(_ lhs: String, _ rhs: String) -> Bool {
683-
if lhs == rhs {
684-
return true
685-
}
686-
687-
let canonicalLHS = CanonicalPackageIdentity(lhs)
688-
let canonicalRHS = CanonicalPackageIdentity(rhs)
689-
if canonicalLHS == canonicalRHS {
690-
return true
691-
}
692-
693-
return false
694-
}
695-
}

‎Sources/PackageGraph/PinsStore.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,13 @@ public final class PinsStore {
101101
self._pins[pin.packageRef.identity] = pin
102102
}
103103

104+
/// Remove a pin.
105+
///
106+
/// This will replace any previous pin with same package name.
107+
public func remove(_ pin: Pin) {
108+
self._pins[pin.packageRef.identity] = nil
109+
}
110+
104111
/// Unpin all of the currently pinned dependencies.
105112
///
106113
/// This method does not automatically write to state file.

‎Sources/PackageModel/Manifest.swift

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,11 @@ public final class Manifest {
4848
/// The repository URL the manifest was loaded from.
4949
public let packageLocation: String
5050

51+
/// The canonical repository URL the manifest was loaded from.
52+
public var canonicalPackageLocation: CanonicalPackageLocation {
53+
CanonicalPackageLocation(self.packageLocation)
54+
}
55+
5156
// FIXME: deprecated 2/2021, remove once clients migrate
5257
@available(*, deprecated, message: "use packageLocation instead")
5358
public var url: String {

‎Sources/PackageModel/Manifest/PackageDependencyDescription.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,13 @@ public enum PackageDependency: Equatable {
7272
public var nameForTargetDependencyResolutionOnly: String {
7373
switch self {
7474
case .fileSystem(let settings):
75-
return settings.nameForTargetDependencyResolutionOnly ?? LegacyPackageIdentity.computeDefaultName(fromPath: settings.path)
75+
return settings.nameForTargetDependencyResolutionOnly ?? PackageIdentityParser.computeDefaultName(fromPath: settings.path)
7676
case .sourceControl(let settings):
7777
switch settings.location {
7878
case .local(let path):
79-
return settings.nameForTargetDependencyResolutionOnly ?? LegacyPackageIdentity.computeDefaultName(fromPath: path)
79+
return settings.nameForTargetDependencyResolutionOnly ?? PackageIdentityParser.computeDefaultName(fromPath: path)
8080
case .remote(let url):
81-
return settings.nameForTargetDependencyResolutionOnly ?? LegacyPackageIdentity.computeDefaultName(fromURL: url)
81+
return settings.nameForTargetDependencyResolutionOnly ?? PackageIdentityParser.computeDefaultName(fromURL: url)
8282
}
8383
case .registry:
8484
return self.identity.description

‎Sources/PackageModel/PackageIdentity.swift

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -12,24 +12,8 @@ import Foundation
1212
import TSCBasic
1313
import TSCUtility
1414

15-
/// When set to `false`,
16-
/// `PackageIdentity` uses the canonical location of package dependencies as its identity.
17-
/// Otherwise, only the last path component is used to identify package dependencies.
18-
public var _useLegacyIdentities: Bool = true {
19-
willSet {
20-
PackageIdentity.provider = newValue ? LegacyPackageIdentity.self : CanonicalPackageIdentity.self
21-
}
22-
}
23-
24-
internal protocol PackageIdentityProvider: CustomStringConvertible {
25-
init(_ string: String)
26-
}
27-
2815
/// The canonical identifier for a package, based on its source location.
2916
public struct PackageIdentity: CustomStringConvertible {
30-
/// The underlying type used to create package identities.
31-
internal static var provider: PackageIdentityProvider.Type = LegacyPackageIdentity.self
32-
3317
/// A textual representation of this instance.
3418
public let description: String
3519

@@ -49,13 +33,13 @@ public struct PackageIdentity: CustomStringConvertible {
4933
/// - Parameter urlString: The package's URL.
5034
// FIXME: deprecate this
5135
public init(urlString: String) {
52-
self.description = Self.provider.init(urlString).description
36+
self.description = PackageIdentityParser(urlString).description
5337
}
5438

5539
/// Creates a package identity from a file path.
5640
/// - Parameter path: An absolute path to the package.
5741
public init(path: AbsolutePath) {
58-
self.description = Self.provider.init(path.pathString).description
42+
self.description = PackageIdentityParser(path.pathString).description
5943
}
6044

6145
/// Creates a plain package identity for a root package
@@ -282,7 +266,7 @@ extension PackageIdentity {
282266

283267
// MARK: -
284268

285-
struct LegacyPackageIdentity: PackageIdentityProvider, Equatable {
269+
struct PackageIdentityParser {
286270
/// A textual representation of this instance.
287271
public let description: String
288272

@@ -329,7 +313,7 @@ struct LegacyPackageIdentity: PackageIdentityProvider, Equatable {
329313
}
330314
}
331315

332-
/// A canonicalized package identity.
316+
/// A canonicalized package location.
333317
///
334318
/// A package may declare external packages as dependencies in its manifest.
335319
/// Each external package is uniquely identified by the location of its source code.
@@ -400,7 +384,7 @@ struct LegacyPackageIdentity: PackageIdentityProvider, Equatable {
400384
/// ```
401385
/// file:///Users/mona/LinkedList → /Users/mona/LinkedList
402386
/// ```
403-
public struct CanonicalPackageIdentity: PackageIdentityProvider, Equatable {
387+
public struct CanonicalPackageLocation: Equatable, CustomStringConvertible {
404388
/// A textual representation of this instance.
405389
public let description: String
406390

‎Sources/PackageModel/PackageReference.swift

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public struct PackageReference {
3434
/// A package from a registry.
3535
case registry(PackageIdentity)
3636

37-
// FIXME: we should not need this
37+
// FIXME: we should not need this once we migrate off URLs
3838
//@available(*, deprecated)
3939
public var locationString: String {
4040
switch self {
@@ -52,6 +52,12 @@ public struct PackageReference {
5252
}
5353
}
5454

55+
// FIXME: we should not need this once we migrate off URLs
56+
//@available(*, deprecated)
57+
public var canonicalLocation: CanonicalPackageLocation {
58+
return CanonicalPackageLocation(self.locationString)
59+
}
60+
5561
public var description: String {
5662
switch self {
5763
case .root(let path):
@@ -92,12 +98,18 @@ public struct PackageReference {
9298
/// The location of the package.
9399
///
94100
/// This could be a remote repository, local repository or local package.
95-
// FIXME: we should not need this
101+
// FIXME: we should not need this once we migrate off URLs
96102
//@available(*, deprecated)
97103
public var locationString: String {
98104
self.kind.locationString
99105
}
100106

107+
// FIXME: we should not need this once we migrate off URLs
108+
//@available(*, deprecated)
109+
public var canonicalLocation: CanonicalPackageLocation {
110+
self.kind.canonicalLocation
111+
}
112+
101113
/// The kind of package: root, local, or remote.
102114
public let kind: Kind
103115

@@ -107,13 +119,13 @@ public struct PackageReference {
107119
self.kind = kind
108120
switch kind {
109121
case .root(let path):
110-
self.deprecatedName = name ?? LegacyPackageIdentity.computeDefaultName(fromPath: path)
122+
self.deprecatedName = name ?? PackageIdentityParser.computeDefaultName(fromPath: path)
111123
case .fileSystem(let path):
112-
self.deprecatedName = name ?? LegacyPackageIdentity.computeDefaultName(fromPath: path)
124+
self.deprecatedName = name ?? PackageIdentityParser.computeDefaultName(fromPath: path)
113125
case .localSourceControl(let path):
114-
self.deprecatedName = name ?? LegacyPackageIdentity.computeDefaultName(fromPath: path)
126+
self.deprecatedName = name ?? PackageIdentityParser.computeDefaultName(fromPath: path)
115127
case .remoteSourceControl(let url):
116-
self.deprecatedName = name ?? LegacyPackageIdentity.computeDefaultName(fromURL: url)
128+
self.deprecatedName = name ?? PackageIdentityParser.computeDefaultName(fromURL: url)
117129
case .registry(let identity):
118130
// FIXME: this is a placeholder
119131
self.deprecatedName = name ?? identity.description
@@ -154,7 +166,7 @@ extension PackageReference: Equatable {
154166

155167
// TODO: consider rolling into Equatable
156168
public func equalsIncludingLocation(_ other: PackageReference) -> Bool {
157-
return self.identity == other.identity && self.kind.locationString == other.kind.locationString
169+
return self.identity == other.identity && self.canonicalLocation == other.canonicalLocation
158170
}
159171
}
160172

‎Sources/SPMTestSupport/Observability.swift

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,17 @@ public func testDiagnostics(
9090
line: UInt = #line,
9191
handler: (DiagnosticsTestResult) throws -> Void
9292
) {
93-
let diagnostics = problemsOnly ? diagnostics.filter({ $0.severity >= .warning }) : diagnostics
93+
testDiagnostics(diagnostics, minSeverity: problemsOnly ? .warning : .debug, file: file, line: line, handler: handler)
94+
}
95+
96+
public func testDiagnostics(
97+
_ diagnostics: [Basics.Diagnostic],
98+
minSeverity: Basics.Diagnostic.Severity,
99+
file: StaticString = #file,
100+
line: UInt = #line,
101+
handler: (DiagnosticsTestResult) throws -> Void
102+
) {
103+
let diagnostics = diagnostics.filter{ $0.severity >= minSeverity }
94104
let testResult = DiagnosticsTestResult(diagnostics)
95105

96106
do {

0 commit comments

Comments
 (0)