Skip to content

Commit a441c21

Browse files
committed
fix dependency resolution edge cases issues (#3985)
motivation: dependency resolution could be incorrect in some edge cases when a pre-release requirement is specified, and not report the issue to the user changes: * depedency resolustion should not assume 0.0.0 is the correct minimal requirment, instead it should use the specified version * fix dependency term computation which used incorrect version requirement for the the parent node * add validation at the end of package udpate API to make sure no missing packages exist after resolution, and fail the operation if such exists. this mirrors the behavior of package resolve API * adjust tests
1 parent 68adc9d commit a441c21

File tree

3 files changed

+89
-58
lines changed

3 files changed

+89
-58
lines changed

Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,41 +1216,51 @@ internal final class PubGrubPackageContainer {
12161216

12171217
// Since the pinned version is most likely to succeed, we don't compute bounds for its
12181218
// incompatibilities.
1219-
return try Array(constraints.map { (constraint: PackageContainerConstraint) -> [Incompatibility] in
1220-
guard case .versionSet(let vs) = constraint.requirement else {
1219+
return try constraints.flatMap { constraint -> [Incompatibility] in
1220+
// We only have version-based requirements at this point.
1221+
guard case .versionSet(let constraintRequirement) = constraint.requirement else {
12211222
throw InternalError("Unexpected unversioned requirement: \(constraint)")
12221223
}
12231224
return try constraint.nodes().map { dependencyNode in
12241225
var terms: OrderedSet<Term> = []
1226+
// the package version requirement
12251227
terms.append(Term(node, .exact(version)))
1226-
terms.append(Term(not: dependencyNode, vs))
1228+
// the dependency's version requirement
1229+
terms.append(Term(not: dependencyNode, constraintRequirement))
12271230
return try Incompatibility(terms, root: root, cause: .dependency(node: node))
12281231
}
1229-
}.joined())
1232+
}
12301233
}
12311234

1232-
let (lowerBounds, upperBounds) = try self.computeBounds(for: node,
1233-
constraints: constraints,
1234-
startingWith: version)
1235-
1236-
return try constraints.flatMap { constraint in
1237-
return try constraint.nodes().map { constraintNode in
1238-
var terms: OrderedSet<Term> = []
1239-
let lowerBound = lowerBounds[constraintNode] ?? "0.0.0"
1240-
let upperBound = upperBounds[constraintNode] ?? Version(version.major + 1, 0, 0)
1241-
assert(lowerBound < upperBound)
1235+
let (lowerBounds, upperBounds) = try self.computeBounds(
1236+
for: node,
1237+
constraints: constraints,
1238+
startingWith: version
1239+
)
12421240

1243-
// We only have version-based requirements at this point.
1244-
guard case .versionSet(let vs) = constraint.requirement else {
1245-
throw InternalError("Unexpected unversioned requirement: \(constraint)")
1241+
return try constraints.flatMap { constraint -> [Incompatibility] in
1242+
// We only have version-based requirements at this point.
1243+
guard case .versionSet(let constraintRequirement) = constraint.requirement else {
1244+
throw InternalError("Unexpected unversioned requirement: \(constraint)")
1245+
}
1246+
return try constraint.nodes().compactMap { constraintNode in
1247+
// cycle
1248+
guard node != constraintNode else {
1249+
return nil
12461250
}
12471251

1248-
let requirement: VersionSetSpecifier = .range(lowerBound ..< upperBound)
1249-
terms.append(Term(node, requirement))
1250-
terms.append(Term(not: constraintNode, vs))
1251-
12521252
// Make a record for this dependency so we don't have to recompute the bounds when the selected version falls within the bounds.
1253-
self.emittedIncompatibilities[constraintNode] = requirement.union(emittedIncompatibilities[constraintNode] ?? .empty)
1253+
if let lowerBound = lowerBounds[constraintNode], let upperBound = upperBounds[constraintNode] {
1254+
assert(lowerBound < upperBound)
1255+
let requirement: VersionSetSpecifier = .range(lowerBound ..< upperBound)
1256+
self.emittedIncompatibilities[constraintNode] = requirement.union(emittedIncompatibilities[constraintNode] ?? .empty)
1257+
}
1258+
1259+
var terms: OrderedSet<Term> = []
1260+
// the package version requirement
1261+
terms.append(Term(node, .exact(version)))
1262+
// the dependency's version requirement
1263+
terms.append(Term(not: constraintNode, constraintRequirement))
12541264

12551265
return try Incompatibility(terms, root: root, cause: .dependency(node: node))
12561266
}

Sources/Workspace/Workspace.swift

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -784,7 +784,7 @@ extension Workspace {
784784
observabilityScope: ObservabilityScope
785785
) throws -> [(PackageReference, Workspace.PackageStateChange)]? {
786786
// Create cache directories.
787-
createCacheDirectories(observabilityScope: observabilityScope)
787+
self.createCacheDirectories(observabilityScope: observabilityScope)
788788

789789
// FIXME: this should not block
790790
// Load the root manifests and currently checked out manifests.
@@ -799,7 +799,9 @@ extension Workspace {
799799
guard let pinsStore = observabilityScope.trap({ try self.pinsStore.load() }) else { return nil }
800800

801801
// Ensure we don't have any error at this point.
802-
guard !observabilityScope.errorsReported else { return nil }
802+
guard !observabilityScope.errorsReported else {
803+
return nil
804+
}
803805

804806
// Add unversioned constraints for edited packages.
805807
var updateConstraints = currentManifests.editedPackagesConstraints()
@@ -830,17 +832,28 @@ extension Workspace {
830832
// Reset the active resolver.
831833
self.activeResolver = nil
832834

833-
guard !observabilityScope.errorsReported else { return nil }
835+
guard !observabilityScope.errorsReported else {
836+
return nil
837+
}
834838

835839
if dryRun {
836-
return observabilityScope.trap { return try computePackageStateChanges(root: graphRoot, resolvedDependencies: updateResults, updateBranches: true, observabilityScope: observabilityScope) }
840+
return observabilityScope.trap {
841+
return try self.computePackageStateChanges(root: graphRoot, resolvedDependencies: updateResults, updateBranches: true, observabilityScope: observabilityScope)
842+
}
837843
}
838844

839845
// Update the checkouts based on new dependency resolution.
840846
let packageStateChanges = self.updateDependenciesCheckouts(root: graphRoot, updateResults: updateResults, updateBranches: true, observabilityScope: observabilityScope)
841847

842848
// Load the updated manifests.
843849
let updatedDependencyManifests = try self.loadDependencyManifests(root: graphRoot, observabilityScope: observabilityScope)
850+
// If we have missing packages, something is fundamentally wrong with the resolution of the graph
851+
let stillMissingPackages = updatedDependencyManifests.computePackages().missing
852+
guard stillMissingPackages.isEmpty else {
853+
let missing = stillMissingPackages.map{ $0.description }
854+
observabilityScope.emit(error: "exhausted attempts to resolve the dependencies graph, with '\(missing.joined(separator: "', '"))' unresolved.")
855+
return nil
856+
}
844857

845858
// Update the resolved file.
846859
self.saveResolvedFile(
@@ -1721,7 +1734,7 @@ extension Workspace {
17211734
let dependenciesToLoad = dependenciesRequired.map{ $0.createPackageRef() }.filter { !loadedManifests.keys.contains($0.identity) }
17221735
let dependenciesManifests = try temp_await { self.loadManagedManifests(for: dependenciesToLoad, observabilityScope: observabilityScope, completion: $0) }
17231736
dependenciesManifests.forEach { loadedManifests[$0.key] = $0.value }
1724-
return pair.item.dependenciesRequired(for: pair.key.productFilter).compactMap { dependency in
1737+
return dependenciesRequired.compactMap { dependency in
17251738
loadedManifests[dependency.identity].flatMap {
17261739
// we also compare the location as this function may attempt to load
17271740
// dependencies that have the same identity but from a different location
@@ -2704,7 +2717,7 @@ extension Workspace {
27042717
) -> [(PackageReference, PackageStateChange)] {
27052718
// Get the update package states from resolved results.
27062719
guard let packageStateChanges = observabilityScope.trap({
2707-
try computePackageStateChanges(root: root, resolvedDependencies: updateResults, updateBranches: updateBranches, observabilityScope: observabilityScope)
2720+
try self.computePackageStateChanges(root: root, resolvedDependencies: updateResults, updateBranches: updateBranches, observabilityScope: observabilityScope)
27082721
}) else {
27092722
return []
27102723
}

0 commit comments

Comments
 (0)