From 8932dc2077a7d08f4931ea2a8b0841aeec3d71d2 Mon Sep 17 00:00:00 2001 From: Slava Kim Date: Tue, 6 May 2014 18:53:52 -0700 Subject: [PATCH] wip: error reporting --- .../constraint-solver-tests.js | 7 +- .../constraint-solver/constraint-solver.js | 2 +- .../constraint-solver/constraints-list.js | 23 ++-- packages/constraint-solver/resolver.js | 114 ++++++++++++------ 4 files changed, 91 insertions(+), 55 deletions(-) diff --git a/packages/constraint-solver/constraint-solver-tests.js b/packages/constraint-solver/constraint-solver-tests.js index 442348807e..a1d5c46926 100644 --- a/packages/constraint-solver/constraint-solver-tests.js +++ b/packages/constraint-solver/constraint-solver-tests.js @@ -99,10 +99,10 @@ var t_progagateExact = function (deps, expected) { currentTest.equal(resolvedDeps, expected); }; -var FAIL = function (deps) { +var FAIL = function (deps, regexp) { currentTest.throws(function () { var resolvedDeps = resolver.resolve(deps); - }); + }, regexp); }; Tinytest.add("constraint solver - exact dependencies", function (test) { @@ -112,10 +112,11 @@ Tinytest.add("constraint solver - exact dependencies", function (test) { t_progagateExact({ "sparky-forms": "=1.1.2", "sparkle": "=2.1.1" }, { "sparky-forms": "1.1.2", "forms": "1.0.1", "sparkle": "2.1.1" }); t_progagateExact({ "awesome-dropdown": "=1.5.0" }, { "awesome-dropdown": "1.5.0", "dropdown": "1.2.2" }); - FAIL({ "sparky-forms": "=1.1.2", "sparkle": "=1.0.0" }); + FAIL({ "sparky-forms": "=1.1.2", "sparkle": "=1.0.0" }, /(.*sparkle.*sparky-forms.*)|(.*sparky-forms.*sparkle.*).*jquery-widgets/); // something that isn't available for your architecture FAIL({ "sparky-forms": "=1.1.2", "sparkle": "=2.0.0" }); FAIL({ "sparky-forms": "=0.0.1" }); + FAIL({ "sparky-forms-nonexistent": "0.0.1" }, /Cannot find anything about.*sparky-forms-nonexistent/); }); Tinytest.add("constraint solver - simple exact + regular deps", function (test) { diff --git a/packages/constraint-solver/constraint-solver.js b/packages/constraint-solver/constraint-solver.js index 871382f9ae..60207ea555 100644 --- a/packages/constraint-solver/constraint-solver.js +++ b/packages/constraint-solver/constraint-solver.js @@ -171,7 +171,7 @@ ConstraintSolver.PackagesResolver.prototype._splitDepsToConstraints = }); if (_.isEmpty(slicesForPackage)) - throw new Error("Resolver has no knowldge about package: " + packageName); + throw new Error("Cannot find anything about package -- " + packageName); _.each(slicesForPackage, function (sliceName) { dependencies.push(sliceName); diff --git a/packages/constraint-solver/constraints-list.js b/packages/constraint-solver/constraints-list.js index a1f4ad41f2..e3a1ca4151 100644 --- a/packages/constraint-solver/constraints-list.js +++ b/packages/constraint-solver/constraints-list.js @@ -65,9 +65,14 @@ ConstraintSolver.ConstraintsList.prototype.push = function (c) { return newList; }; -ConstraintSolver.ConstraintsList.prototype.forPackage = function (name) { +ConstraintSolver.ConstraintsList.prototype.forPackage = function (name, iter) { var self = this; - return mori.get(self.byName, name); + var forPackage = mori.get(self.byName, name); + var exact = mori.get(forPackage, "exact"); + var inexact = mori.get(forPackage, "inexact"); + + mori.each(exact, iter); + mori.each(inexact, iter); }; // doesn't break on the false return value @@ -117,21 +122,15 @@ ConstraintSolver.ConstraintsList.prototype.union = function (anotherList) { // XXX Returns a regular array, not a ConstraintsList. ConstraintSolver.ConstraintsList.prototype.violatedConstraints = function (uv) { var self = this; - var forPackage = mori.get(self.byName, uv.name); - var exact = mori.get(forPackage, "exact"); - var inexact = mori.get(forPackage, "inexact"); var violated = []; - mori.each(exact, function (c) { - if (! mori.last(c).isSatisified(uv)) { - violated.push(c); - } - }); - mori.each(inexact, function (c) { - if (! mori.last(c).isSatisified(uv)) { + + self.forPackage(uv.name, function (c) { + if (! mori.last(c).isSatisfied(uv)) { violated.push(c); } }); + return violated; }; diff --git a/packages/constraint-solver/resolver.js b/packages/constraint-solver/resolver.js index d9ddd7053e..19bac3f1f8 100644 --- a/packages/constraint-solver/resolver.js +++ b/packages/constraint-solver/resolver.js @@ -84,6 +84,11 @@ ConstraintSolver.Resolver.prototype.resolve = }, options); var rootDependencies = _.clone(dependencies); + // required for error reporting later + var constraintAncestor = {}; + _.each(constraints, function (c) { + constraintAncestor[c.toString()] = c.name; + }); dependencies = ConstraintSolver.DependenciesList.fromArray(dependencies, true); constraints = ConstraintSolver.ConstraintsList.fromArray(constraints); @@ -97,7 +102,8 @@ ConstraintSolver.Resolver.prototype.resolve = // - dependencies: DependenciesList // - constraints: ConstraintsList // - choices: array of UnitVersion - var startState = self._propagateExactTransDeps(appUV, dependencies, constraints, choices); + // - constraintAncestor: mapping Constraint.toString() -> Constraint + var startState = self._propagateExactTransDeps(appUV, dependencies, constraints, choices, constraintAncestor); startState.choices = _.filter(startState.choices, function (uv) { return uv.name !== "target"; }); if (options.stopAfterFirstPropagation) @@ -154,8 +160,8 @@ ConstraintSolver.Resolver.prototype.resolve = // - dependencies: DependenciesList - remaining dependencies // - constraints: ConstraintsList - constraints to satisfy // - choices: array of UnitVersion - current fixed set of choices -// - constraintAncestors: Constraint (string representation) -> -// DependencyList. Used for error reporting to indicate which direct +// - constraintAncestor: Constraint (string representation) -> +// Dependency name. Used for error reporting to indicate which direct // dependencies have caused a failure. For every constraint, this is // the list of direct dependencies which led to this constraint being // present. @@ -174,7 +180,7 @@ ConstraintSolver.Resolver.prototype._stateNeighbors = var dependencies = state.dependencies; var constraints = state.constraints; var choices = state.choices; - var constraintAncestors = state.constraintAncestors; + var constraintAncestor = state.constraintAncestor; var candidateName = dependencies.peek(); dependencies = dependencies.remove(candidateName); @@ -184,58 +190,60 @@ ConstraintSolver.Resolver.prototype._stateNeighbors = return _.isEmpty(constraints.violatedConstraints(uv)); }); - if (_.isEmpty(candidateVersions)) { - var uv = self.unitsVersions[candidateName][0]; - if (! uv) { - // XXX Some stupid error message, this should not happen - } else { - var violatedConstraint = _.filter( - constraints.violatedConstraints(uv)[0], - function (constraint) { - return constraint.name === uv.name; - } - ); - // XXX Should we return information about all the violated - // constraints instead of just one? - var directDeps = constraintAncestors[violatedConstraint.toString()]; + var generateError = function (uv, violatedConstraints) { + var directDepsString = ""; + + _.each(violatedConstraints, function (c) { + directDepsString += constraintAncestor[c.toString()] + + + "(" + c.toString() + "), "; + }); + return { success: false, // XXX We really want to say "directDep1 depends on X@1.0 and // directDep2 depends on X@2.0" - failureMsg: "Direct dependencies " + directDeps.toString() + - " conflict on " + uv.name + ". " + violatedConstraint.toString() + failureMsg: "Direct dependencies " + directDepsString + + " conflict on " + uv.name }; - } + }; + if (_.isEmpty(candidateVersions)) { + var uv = self.unitsVersions[candidateName][0]; - return { success: false, - failureMsg: "Cannot choose satisfying versions of package -- " - + candidateName }; + if (! uv) + return { success: false, failureMsg: "Cannot find anything about package -- " + candidateName }; + + return generateError(uv, constraints.violatedConstraints(uv)); } - var lastInvalidNeighbor = null; + var firstError = null; var neighbors = _.chain(candidateVersions).map(function (uv) { var nChoices = _.clone(choices); + var nConstraintAncestors = _.clone(constraintAncestor); nChoices.push(uv); - return self._propagateExactTransDeps(uv, dependencies, constraints, nChoices); + return self._propagateExactTransDeps(uv, dependencies, constraints, nChoices, nConstraintAncestors); }).filter(function (state) { - var isValid = - choicesDontViolateConstraints(state.choices, state.constraints); + var vcfc = + violatedConstraintsForSomeChoice(state.choices, state.constraints); - if (! isValid) - lastInvalidNeighbor = state; + if (! vcfc) + return true; - return isValid; + firstError = generateError(vcfc.choice, vcfc.constraints); + return false; }).value(); + if (firstError) + return firstError; + + // Should never be true as !!firstError === !neighbors.length but still check + // just in case. if (! neighbors.length) return { success: false, failureMsg: "None of the versions unit produces a sensible result -- " - + candidateName, - triedUnitVersions: candidateVersions, - lastInvalidNeighbor: lastInvalidNeighbor }; + + candidateName }; return { success: true, neighbors: neighbors }; }; @@ -247,7 +255,7 @@ ConstraintSolver.Resolver.prototype._stateNeighbors = // propagated (i.e. doesn't try to propagate anything not related to the passed // unit version). ConstraintSolver.Resolver.prototype._propagateExactTransDeps = - function (uv, dependencies, constraints, choices) { + function (uv, dependencies, constraints, choices, constraintAncestor) { var self = this; // XXX representing a queue as an array with push/shift operations is not @@ -256,6 +264,9 @@ ConstraintSolver.Resolver.prototype._propagateExactTransDeps = // Boolean map to avoid adding the same stuff to queue over and over again. // Keeps the time complexity the same but can save some memory. var isEnqueued = {}; + // For keeping track of new choices in this iteration + var oldChoice = {}; + _.each(choices, function (uv) { oldChoice[uv.name] = uv; }); queue.push(uv); isEnqueued[uv.name] = true; @@ -312,17 +323,42 @@ ConstraintSolver.Resolver.prototype._propagateExactTransDeps = }); } + // Update the constraintAncestor table + _.each(choices, function (uv) { + if (oldChoice[uv.name]) + return; + + var relevantConstraint = null; + constraints.forPackage(uv.name, function (c) { relevantConstraint = c; }); + if (! constraintAncestor[relevantConstraint.toString()]) + console.log(relevantConstraint.toString(), constraintAncestor); + + uv.constraints.each(function (c) { + if (! constraintAncestor[c.toString()]) + constraintAncestor[c.toString()] = constraintAncestor[relevantConstraint.toString()]; + }); + }); + return { dependencies: dependencies, constraints: constraints, - choices: choices + choices: choices, + constraintAncestor: constraintAncestor }; }; -var choicesDontViolateConstraints = function (choices, constraints) { - return _.all(choices, function (uv) { - return _.isEmpty(constraints.violatedConstraints(uv)); +var violatedConstraintsForSomeChoice = function (choices, constraints) { + var ret = null; + _.each(choices, function (choice) { + if (ret) + return; + + var violatedConstraints = constraints.violatedConstraints(choice); + if (! _.isEmpty(violatedConstraints)) + ret = { constraints: violatedConstraints, choice: choice }; }); + + return ret; }; ////////////////////////////////////////////////////////////////////////////////