fix dependency resolution edge cases issues #3985

tomerd · 2022-01-07T04:37:58Z

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

tomerd · 2022-01-07T04:38:09Z

@swift-ci please smoke test

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

tomerd · 2022-01-07T07:09:09Z

Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

+                // the package version requirement
+                terms.append(Term(node, .exact(version)))
+                // the dependency's version requirement
+                terms.append(Term(not: constraintNode, constraintRequirement))


this is the heart of the change, there a couple of things going on here that seem wrong:

requirement was used for two different purposes: to record the dependency version ( self.emittedIncompatibilities[constraintNode] = requirement.union(emittedIncompatibilities[constraintNode] ?? .empty)) but also as the resolved version for node (terms.append(Term(node, requirement))), which iiuc does not really make sense as it mixed the dependency (constraintNode) version with the package (node) version.

requirement is looked up on the bounds computed for the dependencies using computeBounds with constraintNode as the key which is correct, however if not found, defaults to 0.0.0 < version(major+1, 0, 0). from what I can see, the defaulting is wrong in two ways: it does not work for pre-release versions since it strips the pre-release extensions (e.g. .exact("1.0.0-alpha.10") becomes .range(0.0.0 ..< 2.0.0) which is causing downstream issue as we are using range checks later in the graph resolution and "1.0.0-alpha.10" is not included in 0.0.0 ..< 2.0.0 based on our version inclusion rules (which is the bug I was chasing and got me here). maybe slightly worse, the default is based on version which is the version of node and has nothing to do with constraintNode so we end up recording (in self.emittedIncompatibilities) version ranges that are potentially wrong

the change suggested here is to always set the node term to the exact version we are computing for which works for pre-release and non-pre-release. it does however break a bunch of tests that recorded the 0.0.0 < version(major+1, 0, 0) behavior, but maybe that is okay since they are testing for wrong behavior?

@SDGGiesbrecht very interested in your 👀 on this, as you are probably more familiar with this code / logic than I am. I actually hope I am wrong and this is not as problematic as I think it is :D

I don't have much knowledge here, but I would not be surprised if some of the tests are conceptually wrong and just verifying the implemented behavior. So I wouldn't be concerned about having to change the tests.

Taking a closer look now. I will get back to you.

I believe both your observations are correct, @tomerd, and same goes for your solutions.

Whatever situation used to hit the defaults will now result in nothing recorded into emittedIncompatibilities. At first that concerned me, but after tracking where that leads, it feels safe, even if a little odd. The only two times emittedIncompatibilities is read from are:

To skip processing a node that has already been handled. Such nodes will be re‐processed now, but I don’t think that would be a problem.

To not trust a pin if we have already encountered anything odd. This too only prevents a shortcut, as the whole graph will be traversed either way, just on the one hand not necessarily trying the most probable solution first when obvious, or on the other hand not preloading alternatives when overestimating the probability that a solution will pan out.

I would be curious to know how the defaults were reachable in the first place, and if it could be done from the wild or only by mocked tests that bypass earlier checks.

I actually hope I am wrong and this is not as problematic as I think it is :D

Yes, that code appears to have been embarrassingly invalid. I think it snuck by everyone for so long because without the proper incompatibility here, it would just fail to record a short‐circuit opportunity and merely be slower, and because the nonsensical extra incompatibility would usually end up so far afield that it would be completely disjoint from the actual problem space.

tomerd · 2022-01-07T07:10:04Z

Sources/Workspace/Workspace.swift

-        guard !observabilityScope.errorsReported else { return nil }
+        guard !observabilityScope.errorsReported else {
+            return nil
+        }


formatting only

tomerd · 2022-01-07T07:10:09Z

Sources/Workspace/Workspace.swift

-            return observabilityScope.trap { return try computePackageStateChanges(root: graphRoot, resolvedDependencies: updateResults, updateBranches: true, observabilityScope: observabilityScope) }
+            return observabilityScope.trap {
+                return try self.computePackageStateChanges(root: graphRoot, resolvedDependencies: updateResults, updateBranches: true, observabilityScope: observabilityScope)
+            }


formatting only

tomerd · 2022-01-07T07:10:30Z

Sources/Workspace/Workspace.swift

+            let missing = stillMissingPackages.map{ $0.description }
+            observabilityScope.emit(error: "exhausted attempts to resolve the dependencies graph, with '\(missing.joined(separator: "', '"))' unresolved.")
+            return nil
+        }


added validation as package update could fail silently without this check

This is a great improvement too — the silent failures cause so much confusion.

tomerd · 2022-01-07T07:11:05Z

Sources/Workspace/Workspace.swift

@@ -1715,7 +1728,7 @@ extension Workspace {
            let dependenciesToLoad = dependenciesRequired.map{ $0.createPackageRef() }.filter { !loadedManifests.keys.contains($0.identity) }
            let dependenciesManifests = try temp_await { self.loadManagedManifests(for: dependenciesToLoad, observabilityScope: observabilityScope, completion: $0) }
            dependenciesManifests.forEach { loadedManifests[$0.key] = $0.value }
-            return pair.item.dependenciesRequired(for: pair.key.productFilter).compactMap { dependency in
+            return dependenciesRequired.compactMap { dependency in


small cleanup as dependenciesRequired is already defined above as pair.item.dependenciesRequired(for: pair.key.productFilter)

tomerd · 2022-01-07T07:12:22Z

Tests/PackageGraphTests/PubgrubTests.swift

@@ -496,7 +496,7 @@ final class PubgrubTests: XCTestCase {
        ])
    }

-    func testCycle1() {
+    func _testCycle1() {


this test crashes now on a precondition and will be fixed together with the other failing tests if this change is good

tomerd · 2022-01-07T07:13:08Z

Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

                    return try Incompatibility(terms, root: root, cause: .dependency(node: node))
                }
-            }.joined())
+            }


bit of style cleanup, uses flatMap instead of creating an array and then joining it. no functional change.

tomerd · 2022-01-07T22:30:10Z

@swift-ci please smoke test

tomerd · 2022-01-07T23:10:20Z

@swift-ci please test package compatibility

tomerd · 2022-01-08T03:49:27Z

@swift-ci please smoke test macOS

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

motivation: improve dependency resolution performance changes: * add informational diagnostics on manifest cache hit/miss * disable expensive optimization in pubgrub resolver that has no functional value given changes from swiftlang#3985 * adjust resolver workspace delgate to be less noisy given the difference in resolution callbacks

motivation: improve dependency resolution performance changes: * add informational diagnostics on manifest cache hit/miss * disable expensive optimization in pubgrub resolver that has no functional value given changes from #3985 * adjust resolver workspace delgate to be less noisy given the difference in resolution callbacks

motivation: improve dependency resolution performance changes: * add informational diagnostics on manifest cache hit/miss * disable expensive optimization in pubgrub resolver that has no functional value given changes from swiftlang#3985 * adjust resolver workspace delgate to be less noisy given the difference in resolution callbacks

motivation: improve dependency resolution performance changes: * add informational diagnostics on manifest cache hit/miss * disable expensive optimization in pubgrub resolver that has no functional value given changes from #3985 * adjust resolver workspace delgate to be less noisy given the difference in resolution callbacks

tomerd requested review from abertelrud, elsh and neonichu as code owners January 7, 2022 04:37

tomerd changed the title ~~fix dependency resolution edge cases issues~~ [wip] fix dependency resolution edge cases issues Jan 7, 2022

tomerd force-pushed the fix/dependency-resolution-edge-cases branch from 04ccc12 to 6c445b5 Compare January 7, 2022 06:58

tomerd commented Jan 7, 2022

View reviewed changes

tests

9385373

tomerd changed the title ~~[wip] fix dependency resolution edge cases issues~~ fix dependency resolution edge cases issues Jan 7, 2022

tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Jan 8, 2022

abertelrud approved these changes Jan 8, 2022

View reviewed changes

tomerd merged commit 12b8e88 into swiftlang:main Jan 9, 2022

tomerd mentioned this pull request Jan 9, 2022

[5.6] fix dependency resolution edge cases issues (#3985) #3997

Merged

tomerd mentioned this pull request Jan 12, 2022

[wip] dependency resolution performance #4006

Closed

tomerd mentioned this pull request Jan 12, 2022

dependency resolution performance #4007

Merged

tomerd mentioned this pull request Jan 13, 2022

[5.6] dependency resolution performance (#4007) #4012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix dependency resolution edge cases issues #3985

fix dependency resolution edge cases issues #3985

Uh oh!

tomerd commented Jan 7, 2022

Uh oh!

tomerd commented Jan 7, 2022

Uh oh!

tomerd Jan 7, 2022 •

edited

Loading

Uh oh!

abertelrud Jan 7, 2022

Uh oh!

SDGGiesbrecht Jan 7, 2022

Uh oh!

SDGGiesbrecht Jan 7, 2022

Uh oh!

tomerd Jan 7, 2022

Uh oh!

tomerd Jan 7, 2022

Uh oh!

tomerd Jan 7, 2022

Uh oh!

abertelrud Jan 7, 2022

Uh oh!

tomerd Jan 7, 2022

Uh oh!

tomerd Jan 7, 2022

Uh oh!

tomerd Jan 7, 2022 •

edited

Loading

Uh oh!

tomerd commented Jan 7, 2022

Uh oh!

tomerd commented Jan 7, 2022

Uh oh!

tomerd commented Jan 8, 2022

Uh oh!

Uh oh!

fix dependency resolution edge cases issues #3985

fix dependency resolution edge cases issues #3985

Uh oh!

Conversation

tomerd commented Jan 7, 2022

Uh oh!

tomerd commented Jan 7, 2022

Uh oh!

tomerd Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Jan 7, 2022

Uh oh!

tomerd commented Jan 7, 2022

Uh oh!

tomerd commented Jan 8, 2022

Uh oh!

Uh oh!

tomerd Jan 7, 2022 •

edited

Loading

tomerd Jan 7, 2022 •

edited

Loading