Skip to content

Commit 04ccc12

Browse files
committed
fix dependency resolution edge cases issues
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 requirment 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
1 parent 5a1ac27 commit 04ccc12

File tree

2 files changed

+33
-15
lines changed

2 files changed

+33
-15
lines changed

Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1229,25 +1229,30 @@ internal final class PubGrubPackageContainer {
12291229
}.joined())
12301230
}
12311231

1232-
let (lowerBounds, upperBounds) = try self.computeBounds(for: node,
1233-
constraints: constraints,
1234-
startingWith: version)
1232+
let (lowerBounds, upperBounds) = try self.computeBounds(
1233+
for: node,
1234+
constraints: constraints,
1235+
startingWith: version
1236+
)
12351237

12361238
return try constraints.flatMap { constraint in
12371239
return try constraint.nodes().map { constraintNode in
12381240
var terms: OrderedSet<Term> = []
1239-
let lowerBound = lowerBounds[constraintNode] ?? "0.0.0"
1240-
let upperBound = upperBounds[constraintNode] ?? Version(version.major + 1, 0, 0)
1241+
1242+
let lowerBound = lowerBounds[node] ?? version
1243+
let upperBound = upperBounds[node] ?? Version(version.major + 1, 0, 0)
12411244
assert(lowerBound < upperBound)
1245+
let requirement: VersionSetSpecifier = .range(lowerBound ..< upperBound)
12421246

12431247
// We only have version-based requirements at this point.
1244-
guard case .versionSet(let vs) = constraint.requirement else {
1248+
guard case .versionSet(let constraintVersionSet) = constraint.requirement else {
12451249
throw InternalError("Unexpected unversioned requirement: \(constraint)")
12461250
}
12471251

1248-
let requirement: VersionSetSpecifier = .range(lowerBound ..< upperBound)
1249-
terms.append(Term(node, requirement))
1250-
terms.append(Term(not: constraintNode, vs))
1252+
// the package version requirement
1253+
terms.append(Term(node, .exact(version)))
1254+
// the dependency's version requirement
1255+
terms.append(Term(not: constraintNode, constraintVersionSet))
12511256

12521257
// Make a record for this dependency so we don't have to recompute the bounds when the selected version falls within the bounds.
12531258
self.emittedIncompatibilities[constraintNode] = requirement.union(emittedIncompatibilities[constraintNode] ?? .empty)

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)