Skip to content

improve dependency resolution diagnostics when resolving non-versioned dependencies #4030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 18, 2022
Merged

improve dependency resolution diagnostics when resolving non-versioned dependencies #4030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a38778f
improve dependency resolution diagnostics when resolving non-versione…
tomerd Jan 14, 2022
75e8d1b
more tests
tomerd Jan 15, 2022
abc0202
possibly maybe
tomerd Jan 15, 2022
e26a79e
cleanup
tomerd Jan 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Sources/Basics/Dictionary+Extensions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,13 @@ extension Dictionary {
return value
}
}

extension OrderedDictionary {
public subscript(key: Key, `default` `default`: Value) -> Value {
set {
self[key] = newValue
} get {
self[key] ?? `default`
}
}
}
4 changes: 2 additions & 2 deletions Sources/PackageGraph/DependencyResolutionNode.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public enum DependencyResolutionNode {
/// They derive from the manifest.
///
/// Tools versions before 5.2 do not know which products belong to which packages, so each product is required from every dependency.
/// Since a non‐existant product ends up with only its implicit dependency on its own package,
/// Since a non‐existent product ends up with only its implicit dependency on its own package,
/// only whichever package contains the product will end up adding additional constraints.
/// See `ProductFilter` and `Manifest.register(...)`.
case product(String, package: PackageReference)
Expand All @@ -52,7 +52,7 @@ public enum DependencyResolutionNode {
///
/// - zero or more dependencies on each external product node required to build any of its targets (vended or not).
/// - zero or more dependencies directly on external empty package nodes.
/// This special case occurs when a dependecy is declared but not used.
/// This special case occurs when a dependency is declared but not used.
/// It is a warning condition, and builds do not actually need these dependencies.
/// However, forcing the graph to resolve and fetch them anyway allows the diagnostics passes access
/// to the information needed in order to provide actionable suggestions to help the user stitch up the dependency declarations properly.
Expand Down
75 changes: 39 additions & 36 deletions Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,33 +132,38 @@ public struct PubgrubDependencyResolver {

/// Execute the resolution algorithm to find a valid assignment of versions.
public func solve(constraints: [Constraint]) -> Result<[DependencyResolver.Binding], Error> {
let root = DependencyResolutionNode.root(package: .root(
identity: .plain("<synthesized-root>"),
path: .root
))
// the graph resolution root
let root: DependencyResolutionNode
if constraints.count == 1, let constraint = constraints.first, constraint.package.kind.isRoot {
// root level package, use it as our resolution root
root = .root(package: constraint.package)
} else {
// more complex setup requires a synthesized root
root = .root(package: .root(
identity: .plain("<synthesized-root>"),
path: .root
))
}

do {
// strips state
return .success(try self.solve(root: root, constraints: constraints).bindings)
} catch {
var error = error

// If version solving failing, build the user-facing diagnostic.
if let pubGrubError = error as? PubgrubError, let rootCause = pubGrubError.rootCause, let incompatibilities = pubGrubError.incompatibilities {
var builder = DiagnosticReportBuilder(
root: root,
incompatibilities: incompatibilities,
provider: self.provider
)

do {
var builder = DiagnosticReportBuilder(
root: root,
incompatibilities: incompatibilities,
provider: self.provider
)
let diagnostic = try builder.makeErrorReport(for: rootCause)
error = PubgrubError.unresolvable(diagnostic)
return.failure(PubgrubError.unresolvable(diagnostic))
} catch {
// failed to construct the report, will report the original error
return .failure(error)
}
}

Copy link
Contributor Author

@tomerd tomerd Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error handling cleanup ^^

return .failure(error)
}
}
Expand Down Expand Up @@ -262,13 +267,13 @@ public struct PubgrubDependencyResolver {
var overriddenPackages: [PackageReference: (version: BoundVersion, products: ProductFilter)] = [:]

// The list of version-based references reachable via local and branch-based references.
// These are added as top-level incompatibilities since they always need to be statisfied.
// These are added as top-level incompatibilities since they always need to be satisfied.
// Some of these might be overridden as we discover local and branch-based references.
var versionBasedDependencies: [DependencyResolutionNode: [VersionBasedConstraint]] = [:]
var versionBasedDependencies = OrderedDictionary<DependencyResolutionNode, [VersionBasedConstraint]>()
Copy link
Contributor

@abertelrud abertelrud Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think switching to ordered collections is great where it makes sense — it will often help understandability if we can make the order of diagnostics stable from run to run, and also, when there's no better order, to show entities in the same order that users specify them (not always possible of course, but sometimes it is).


// Process unversioned constraints in first phase. We go through all of the unversioned packages
// and collect them and their dependencies. This gives us the complete list of unversioned
// packages in the graph since unversioned packages can only be refered by other
// packages in the graph since unversioned packages can only be referred by other
// unversioned packages.
while let constraint = constraints.first(where: { $0.requirement == .unversioned }) {
constraints.remove(constraint)
Expand Down Expand Up @@ -370,7 +375,7 @@ public struct PubgrubDependencyResolver {
case .versionSet(let req):
for node in dependency.nodes() {
let versionedBasedConstraint = VersionBasedConstraint(node: node, req: req)
versionBasedDependencies[node, default: []].append(versionedBasedConstraint)
versionBasedDependencies[.root(package: constraint.package), default: []].append(versionedBasedConstraint)
}
case .revision:
constraints.append(dependency)
Expand All @@ -386,13 +391,11 @@ public struct PubgrubDependencyResolver {

// At this point, we should be left with only version-based requirements in our constraints
// list. Add them to our version-based dependency list.
for dependency in constraints {
switch dependency.requirement {
for constraint in constraints {
switch constraint.requirement {
case .versionSet(let req):
for node in dependency.nodes() {
for node in constraint.nodes() {
let versionedBasedConstraint = VersionBasedConstraint(node: node, req: req)
// FIXME: It would be better to record where this constraint came from, instead of just
// using root.
versionBasedDependencies[root, default: []].append(versionedBasedConstraint)
}
case .revision, .unversioned:
Expand All @@ -401,11 +404,11 @@ public struct PubgrubDependencyResolver {
}

// Finally, compute the root incompatibilities (which will be all version-based).
// note versionBasedDependencies may point to the root package dependencies, or the dependencies of root's non-versioned dependencies
var rootIncompatibilities: [Incompatibility] = []
for (node, constraints) in versionBasedDependencies {
for constraint in constraints {
if overriddenPackages.keys.contains(constraint.node.package) { continue }

let incompat = try Incompatibility(
Term(root, .exact("1.0.0")),
Term(not: constraint.node, constraint.requirement),
Expand Down Expand Up @@ -850,14 +853,20 @@ private struct DiagnosticReportBuilder {

private func description(for incompatibility: Incompatibility) throws -> String {
switch incompatibility.cause {
case .dependency(node: _):
case .dependency(let causeNode):
assert(incompatibility.terms.count == 2)
let depender = incompatibility.terms.first!
let dependee = incompatibility.terms.last!
assert(depender.isPositive)
assert(!dependee.isPositive)

let dependerDesc = try description(for: depender, normalizeRange: true)
let dependerDesc: String
// when depender is the root node, the causeNode may be different as it may represent root's indirect dependencies (e.g. dependencies of root's unversioned dependencies)
if depender.node == self.rootNode, causeNode != self.rootNode {
dependerDesc = causeNode.nameForDiagnostics
} else {
dependerDesc = try description(for: depender, normalizeRange: true)
}
let dependeeDesc = try description(for: dependee)
return "\(dependerDesc) depends on \(dependeeDesc)"
case .noAvailableVersion:
Expand All @@ -871,6 +880,8 @@ private struct DiagnosticReportBuilder {
let term = incompatibility.terms.first!
assert(term.isPositive)
return "\(term.node.nameForDiagnostics) is \(term.requirement)"
case .conflict where incompatibility.terms.count == 1 && incompatibility.terms.first?.node == self.rootNode:
return "dependencies could not be resolved"
case .conflict:
break
case .versionBasedDependencyContainsUnversionedDependency(let versionedDependency, let unversionedDependency):
Expand All @@ -880,10 +891,6 @@ private struct DiagnosticReportBuilder {
return "\(try description(for: term, normalizeRange: true)) contains incompatible tools version (\(version))"
}

if isFailure(incompatibility) {
return "dependencies could not be resolved"
}

let terms = incompatibility.terms
if terms.count == 1 {
let term = terms.first!
Expand Down Expand Up @@ -957,11 +964,6 @@ private struct DiagnosticReportBuilder {
return !lineNumbers.keys.contains(complex)
}

// FIXME: This is duplicated and wrong.
private func isFailure(_ incompatibility: Incompatibility) -> Bool {
return incompatibility.terms.count == 1 && incompatibility.terms.first?.node.package.identity == .plain("<synthesized-root>")
}
Copy link
Contributor Author

@tomerd tomerd Jan 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated cleanup


private func description(for term: Term, normalizeRange: Bool = false) throws -> String {
let name = term.node.nameForDiagnostics

Expand Down Expand Up @@ -1393,6 +1395,7 @@ private extension PackageRequirement {

private extension DependencyResolutionNode {
var nameForDiagnostics: String {
return "'\(package.identity)'"
return "'\(self.package.identity)'"
}
}

Loading