add delegate to track events from the dependency resolver #3420
Conversation
tomerd
requested review from
abertelrud,
friedbunny and
neonichu
as code owners
|
@neonichu @abertelrud looking for early feedback on the approach and open questions, so we can finalize the feature |
|
|
||
| public protocol DependencyResolverDelegate { | ||
| func willResolve(term: Term) | ||
| func didResolve(term: Term, version: Version) |
| func satisfied(term: Term, by: Assignment, incompatibility: Incompatibility) | ||
| func partiallySatisfied(term: Term, by: Assignment, incompatibility: Incompatibility, difference: Term) | ||
| func failedToResolve(incompatibility: Incompatibility) | ||
| func computed(bindings: [DependencyResolver.Binding]) |
There was a problem hiding this comment.
^^ are more questionable and mirror the current logging behavior of pubgrub resolver
|
I think having a delegate that's 1:1 with the only current resolver makes sense to me, we can choose to not emit logs for every callback if we want to. I don't think we will know enough to have a differently shaped delegate until we would actually have more than one resolver implementation. |
100%. As Boris says, the callbacks should reflect what's actually happening. Individual delegates can then easily choose to ignore some of them, as long as they are all optional. |
I think I want to modulate this statement ("all optional"): since the delegate itself is optional, it might make sense to define the "major" events that the delegate just has to implement, and then maybe some of the minor ones could be optional (have default implementations that don't do anything). Or should they all be optional for consistency, especially since any new ones that get added later will have to be optional for compatibility reasons (until libSwiftPM is properly versioned). WDYT? |
I actually dislike the current pattern in SwiftPM where we have empty implementation for delegates to make them seem optional. sure, it makes life easier for the caller when they first add a delegate but it makes it very difficult to properly evolve the delegate API inc. deprecations as the default empty ones hide the fact that that API changed from the caller making these into runtime issues. in other words, I'd like to see us factor out all the default empty delegate extensions so that the caller can decide explicitly what to ignore and be notified when we deprecate delegate APIs. |
…solver motivation: * significant part of pacakge resolution is spent in the dependency resolver which does not emit events we can use to tell the user what is happening * a follow up PR would use these new events to emit inforamtion when package depedencies are resolved, e.g when running swift package update chnages: * define DependencyResolverDelegate * replace logging in PubgrubDependencyResolver with calling the relevant delegate methods * add TracingDependencyResolverDelegate which back-fills the existing logging behavior in PubgrubDependencyResolver * adjust call-sites open issues: * is this the right API shape? the delegate methods are 1:1 mapping to PubgrubDependencyResolver logging which may or may not be the right API * add tests once API is stable
tomerd
force-pushed
the
feature/resolver-delegate
branch
from
1628832 to
42e0e92
Compare
tomerd
changed the title
|
@swift-ci please smoke test |
tomerd
added
the
ready
That makes sense. That will be easier to do once we adopt semantic versioning. At least some of the empty delegates now are to avoid breaking clients, I think. |
|
This looks great to me. |
…3420) motivation: * significant part of pacakge resolution is spent in the dependency resolver which does not emit events we can use to tell the user what is happening * a follow up PR would use these new events to emit inforamtion when package depedencies are resolved, e.g when running swift package update chnages: * define DependencyResolverDelegate * replace logging in PubgrubDependencyResolver with calling the relevant delegate methods * add TracingDependencyResolverDelegate which back-fills the existing logging behavior in PubgrubDependencyResolver * adjust call-sites
) motivation: * significant part of pacakge resolution is spent in the dependency resolver which does not emit events we can use to tell the user what is happening * a follow up PR would use these new events to emit inforamtion when package depedencies are resolved, e.g when running swift package update chnages: * define DependencyResolverDelegate * replace logging in PubgrubDependencyResolver with calling the relevant delegate methods * add TracingDependencyResolverDelegate which back-fills the existing logging behavior in PubgrubDependencyResolver * adjust call-sites
motivation:
chnages: