Skip to content

Commit 12b8e88

Browse files
authored
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 a3946cc commit 12b8e88

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
@@ -778,7 +778,7 @@ extension Workspace {
778778
observabilityScope: ObservabilityScope
779779
) throws -> [(PackageReference, Workspace.PackageStateChange)]? {
780780
// Create cache directories.
781-
createCacheDirectories(observabilityScope: observabilityScope)
781+
self.createCacheDirectories(observabilityScope: observabilityScope)
782782

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

795795
// Ensure we don't have any error at this point.
796-
guard !observabilityScope.errorsReported else { return nil }
796+
guard !observabilityScope.errorsReported else {
797+
return nil
798+
}
797799

798800
// Add unversioned constraints for edited packages.
799801
var updateConstraints = currentManifests.editedPackagesConstraints()
@@ -824,17 +826,28 @@ extension Workspace {
824826
// Reset the active resolver.
825827
self.activeResolver = nil
826828

827-
guard !observabilityScope.errorsReported else { return nil }
829+
guard !observabilityScope.errorsReported else {
830+
return nil
831+
}
828832

829833
if dryRun {
830-
return observabilityScope.trap { return try computePackageStateChanges(root: graphRoot, resolvedDependencies: updateResults, updateBranches: true, observabilityScope: observabilityScope) }
834+
return observabilityScope.trap {
835+
return try self.computePackageStateChanges(root: graphRoot, resolvedDependencies: updateResults, updateBranches: true, observabilityScope: observabilityScope)
836+
}
831837
}
832838

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

836842
// Load the updated manifests.
837843
let updatedDependencyManifests = try self.loadDependencyManifests(root: graphRoot, observabilityScope: observabilityScope)
844+
// If we have missing packages, something is fundamentally wrong with the resolution of the graph
845+
let stillMissingPackages = updatedDependencyManifests.computePackages().missing
846+
guard stillMissingPackages.isEmpty else {
847+
let missing = stillMissingPackages.map{ $0.description }
848+
observabilityScope.emit(error: "exhausted attempts to resolve the dependencies graph, with '\(missing.joined(separator: "', '"))' unresolved.")
849+
return nil
850+
}
838851

839852
// Update the resolved file.
840853
self.saveResolvedFile(
@@ -1715,7 +1728,7 @@ extension Workspace {
17151728
let dependenciesToLoad = dependenciesRequired.map{ $0.createPackageRef() }.filter { !loadedManifests.keys.contains($0.identity) }
17161729
let dependenciesManifests = try temp_await { self.loadManagedManifests(for: dependenciesToLoad, observabilityScope: observabilityScope, completion: $0) }
17171730
dependenciesManifests.forEach { loadedManifests[$0.key] = $0.value }
1718-
return pair.item.dependenciesRequired(for: pair.key.productFilter).compactMap { dependency in
1731+
return dependenciesRequired.compactMap { dependency in
17191732
loadedManifests[dependency.identity].flatMap {
17201733
// we also compare the location as this function may attempt to load
17211734
// dependencies that have the same identity but from a different location
@@ -2698,7 +2711,7 @@ extension Workspace {
26982711
) -> [(PackageReference, PackageStateChange)] {
26992712
// Get the update package states from resolved results.
27002713
guard let packageStateChanges = observabilityScope.trap({
2701-
try computePackageStateChanges(root: root, resolvedDependencies: updateResults, updateBranches: updateBranches, observabilityScope: observabilityScope)
2714+
try self.computePackageStateChanges(root: root, resolvedDependencies: updateResults, updateBranches: updateBranches, observabilityScope: observabilityScope)
27022715
}) else {
27032716
return []
27042717
}

0 commit comments

Comments
 (0)