From f82bccb757d5aa54f5206e3a77dbca028000f3f5 Mon Sep 17 00:00:00 2001 From: Sam Boyer Date: Mon, 4 Apr 2016 15:32:22 -0400 Subject: [PATCH] Improve tests and logic for solve failure handling --- bestiary_test.go | 19 ++++--- errors.go | 94 +++++++++++++++++++++++++++---- solve_test.go | 141 +++++++++++++++++++++++++++++++++++------------ solver.go | 81 ++++++++++++++------------- version_queue.go | 20 +++++-- 5 files changed, 261 insertions(+), 94 deletions(-) diff --git a/bestiary_test.go b/bestiary_test.go index 8e033757..88910c70 100644 --- a/bestiary_test.go +++ b/bestiary_test.go @@ -114,6 +114,8 @@ type fixture struct { downgrade bool // lock file simulator, if one's to be used at all l Lock + // projects expected to have errors, if any + errp []string } // mklock makes a fixLock, suitable to act as a lock file @@ -354,6 +356,15 @@ var fixtures = []fixture{ "foo 1.0.0", ), }, + { + n: "no version that matches requirement", + ds: []depspec{ + dsv("root 0.0.0", "foo >=1.0.0, <2.0.0"), + dsv("foo 2.0.0"), + dsv("foo 2.1.3"), + }, + errp: []string{"foo", "root"}, + }, } type depspecSourceManager struct { @@ -552,14 +563,6 @@ func rootDependency() { } func unsolvable() { - testResolve("no version that matches requirement", { - "myapp 0.0.0": { - "foo": ">=1.0.0 <2.0.0" - }, - "foo 2.0.0": {}, - "foo 2.1.3": {} - }, error: noVersion(["myapp", "foo"])); - testResolve("no version that matches combined constraint", { "myapp 0.0.0": { "foo": "1.0.0", diff --git a/errors.go b/errors.go index 281fc79f..86e1e4c9 100644 --- a/errors.go +++ b/errors.go @@ -1,5 +1,10 @@ package vsolver +import ( + "bytes" + "fmt" +) + type errorLevel uint8 // TODO consistent, sensible way of handling 'type' and 'severity' - or figure @@ -30,23 +35,92 @@ func (e *solveError) Error() string { } type noVersionError struct { - pn ProjectName - v string - c Constraint - deps []Dependency + pn ProjectName + fails []failedVersion } func (e *noVersionError) Error() string { - // TODO compose a message out of the data we have - return "" + if len(e.fails) == 0 { + return fmt.Sprintf("No versions could be found for project %q.", e.pn) + } + + var buf bytes.Buffer + fmt.Fprintf(&buf, "Could not find any versions of %s that met constraints:\n", e.pn) + for _, f := range e.fails { + fmt.Fprintf(&buf, "\t%s: %s", f.v.Info, f.f.Error()) + } + + return buf.String() } type disjointConstraintFailure struct { - pn ProjectName - deps []Dependency + goal Dependency + failsib []Dependency + nofailsib []Dependency + c Constraint } func (e *disjointConstraintFailure) Error() string { - // TODO compose a message out of the data we have - return "" + if len(e.failsib) == 1 { + str := "Could not introduce %s at %s, as it has a dependency on %s with constraint %s, which has no overlap with existing constraint %s from %s at %s" + return fmt.Sprintf(str, e.goal.Depender.Name, e.goal.Depender.Version.Info, e.goal.Dep.Name, e.goal.Dep.Constraint.Body(), e.failsib[0].Dep.Constraint.Body(), e.failsib[0].Depender.Name, e.failsib[0].Depender.Version.Info) + } + + var buf bytes.Buffer + + var sibs []Dependency + if len(e.failsib) > 1 { + sibs = e.failsib + + str := "Could not introduce %s at %s, as it has a dependency on %s with constraint %s, which has no overlap with the following existing constraints:\n" + fmt.Fprintf(&buf, str, e.goal.Depender.Name, e.goal.Depender.Version.Info, e.goal.Dep.Name, e.goal.Dep.Constraint.Body()) + } else { + sibs = e.nofailsib + + str := "Could not introduce %s at %s, as it has a dependency on %s with constraint %s, which does not overlap with the intersection of existing constraints from other currently selected packages:\n" + fmt.Fprintf(&buf, str, e.goal.Depender.Name, e.goal.Depender.Version.Info, e.goal.Dep.Name, e.goal.Dep.Constraint.Body()) + } + + for _, c := range sibs { + fmt.Fprintf(&buf, "\t%s at %s with constraint %s\n", c.Depender.Name, c.Depender.Version.Info, c.Dep.Constraint.Body()) + } + + return buf.String() +} + +// Indicates that an atom could not be introduced because one of its dep +// constraints does not admit the currently-selected version of the target +// project. +type constraintNotAllowedFailure struct { + goal Dependency + v Version +} + +func (e *constraintNotAllowedFailure) Error() string { + str := "Could not introduce %s at %s, as it has a dependency on %s with constraint %s, which does not allow the currently selected version of %s" + return fmt.Sprintf(str, e.goal.Depender.Name, e.goal.Depender.Version.Info, e.goal.Dep.Name, e.goal.Dep.Constraint, e.v.Info) +} + +type versionNotAllowedFailure struct { + goal ProjectAtom + failparent []Dependency + c Constraint +} + +func (e *versionNotAllowedFailure) Error() string { + if len(e.failparent) == 1 { + str := "Could not introduce %s at %s, as it is not allowed by constraint %s from project %s." + return fmt.Sprintf(str, e.goal.Name, e.goal.Version.Info, e.failparent[0].Dep.Constraint.Body(), e.failparent[0].Depender.Name) + } + + var buf bytes.Buffer + + str := "Could not introduce %s at %s, as it is not allowed by constraints from the following projects:\n" + fmt.Fprintf(&buf, str, e.goal.Name, e.goal.Version.Info) + + for _, f := range e.failparent { + fmt.Fprintf(&buf, "\t%s at %s with constraint %s\n", f.Depender.Name, f.Depender.Version.Info, f.Dep.Constraint.Body()) + } + + return buf.String() } diff --git a/solve_test.go b/solve_test.go index 369f6edf..5d46eb48 100644 --- a/solve_test.go +++ b/solve_test.go @@ -1,6 +1,7 @@ package vsolver import ( + "strings" "testing" "github.com/Sirupsen/logrus" @@ -36,49 +37,121 @@ func solveAndBasicChecks(fix fixture, t *testing.T) Result { } result := s.Solve(p, nil) - if result.SolveFailure != nil { - t.Errorf("(fixture: %q) Solver failed; error was type %T, text: %q", fix.n, result.SolveFailure, result.SolveFailure) - return result - } + if len(fix.errp) > 0 { + if result.SolveFailure == nil { + t.Errorf("(fixture: %q) Solver succeeded, but expected failure") + } - if fix.maxAttempts > 0 && result.Attempts > fix.maxAttempts { - t.Errorf("(fixture: %q) Solver completed in %v attempts, but expected %v or fewer", result.Attempts, fix.maxAttempts) - } + switch fail := result.SolveFailure.(type) { + case *noVersionError: + if fix.errp[0] != string(fail.pn) { + t.Errorf("Expected failure on project %s, but was on project %s", fail.pn, fix.errp[0]) + } - // Dump result projects into a map for easier interrogation - rp := make(map[string]string) - for _, p := range result.Projects { - rp[string(p.Name)] = p.Version.Info - } + ep := make(map[string]struct{}) + for _, p := range fix.errp[1:] { + ep[p] = struct{}{} + } - fixlen, rlen := len(fix.r), len(rp) - if fixlen != rlen { - // Different length, so they definitely disagree - t.Errorf("(fixture: %q) Solver reported %v package results, result expected %v", fix.n, rlen, fixlen) - } + found := make(map[string]struct{}) + for _, vf := range fail.fails { + for _, f := range getFailureCausingProjects(vf.f) { + found[f] = struct{}{} + } + } - // Whether or not len is same, still have to verify that results agree - // Walk through fixture/expected results first - for p, v := range fix.r { - if av, exists := rp[p]; !exists { - t.Errorf("(fixture: %q) Project %q expected but missing from results", fix.n, p) - } else { - // delete result from map so we skip it on the reverse pass - delete(rp, p) - if v != av { - t.Errorf("(fixture: %q) Expected version %q of project %q, but actual version was %q", fix.n, v, p, av) + var missing []string + var extra []string + for p, _ := range found { + if _, has := ep[p]; !has { + extra = append(extra, p) + } + } + if len(extra) > 0 { + t.Errorf("Expected solve failures due to projects %s, but solve failures also arose from %s", strings.Join(fix.errp[1:], ", "), strings.Join(extra, ", ")) + } + + for p, _ := range ep { + if _, has := found[p]; !has { + missing = append(missing, p) + } + } + if len(missing) > 0 { + t.Errorf("Expected solve failures due to projects %s, but %s had no failures", strings.Join(fix.errp[1:], ", "), strings.Join(missing, ", ")) + } + + default: + // TODO round these out + panic("unhandled solve failure type") + } + } else { + if result.SolveFailure != nil { + t.Errorf("(fixture: %q) Solver failed; error was type %T, text: %q", fix.n, result.SolveFailure, result.SolveFailure) + return result + } + + if fix.maxAttempts > 0 && result.Attempts > fix.maxAttempts { + t.Errorf("(fixture: %q) Solver completed in %v attempts, but expected %v or fewer", result.Attempts, fix.maxAttempts) + } + + // Dump result projects into a map for easier interrogation + rp := make(map[string]string) + for _, p := range result.Projects { + rp[string(p.Name)] = p.Version.Info + } + + fixlen, rlen := len(fix.r), len(rp) + if fixlen != rlen { + // Different length, so they definitely disagree + t.Errorf("(fixture: %q) Solver reported %v package results, result expected %v", fix.n, rlen, fixlen) + } + + // Whether or not len is same, still have to verify that results agree + // Walk through fixture/expected results first + for p, v := range fix.r { + if av, exists := rp[p]; !exists { + t.Errorf("(fixture: %q) Project %q expected but missing from results", fix.n, p) + } else { + // delete result from map so we skip it on the reverse pass + delete(rp, p) + if v != av { + t.Errorf("(fixture: %q) Expected version %q of project %q, but actual version was %q", fix.n, v, p, av) + } + } + } + + // Now walk through remaining actual results + for p, v := range rp { + if fv, exists := fix.r[p]; !exists { + t.Errorf("(fixture: %q) Unexpected project %q present in results", fix.n, p) + } else if v != fv { + t.Errorf("(fixture: %q) Got version %q of project %q, but expected version was %q", fix.n, v, p, fv) } } } - // Now walk through remaining actual results - for p, v := range rp { - if fv, exists := fix.r[p]; !exists { - t.Errorf("(fixture: %q) Unexpected project %q present in results", fix.n, p) - } else if v != fv { - t.Errorf("(fixture: %q) Got version %q of project %q, but expected version was %q", fix.n, v, p, fv) + return result + +} + +func getFailureCausingProjects(err error) (projs []string) { + switch e := err.(type) { + case *noVersionError: + projs = append(projs, string(e.pn)) + case *disjointConstraintFailure: + for _, f := range e.failsib { + projs = append(projs, string(f.Depender.Name)) } + case *versionNotAllowedFailure: + for _, f := range e.failparent { + projs = append(projs, string(f.Depender.Name)) + } + case *constraintNotAllowedFailure: + // No sane way of knowing why the currently selected version is + // selected, so do nothing + default: + panic("unknown failtype") } - return result + return } diff --git a/solver.go b/solver.go index 651a023c..f997f701 100644 --- a/solver.go +++ b/solver.go @@ -7,14 +7,6 @@ import ( "github.com/Sirupsen/logrus" ) -//type SolveFailure uint - -//const ( -// Indicates that no version solution could be found -//NoVersionSolution SolveFailure = 1 << iota -//IncompatibleVersionType -//) - func NewSolver(sm SourceManager, l *logrus.Logger) Solver { if l == nil { l = logrus.New() @@ -169,15 +161,16 @@ func (s *solver) createVersionQueue(ref ProjectName) (*versionQueue, error) { return q, s.findValidVersion(q) } -// findValidVersion walks through a versionQueue until it finds a version that's -// valid, as adjudged by the current constraints. +// findValidVersion walks through a versionQueue until it finds a version that +// satisfies the constraints held in the current state of the solver. func (s *solver) findValidVersion(q *versionQueue) error { - var err error if emptyVersion == q.current() { // TODO this case shouldn't be reachable, but panic here as a canary panic("version queue is empty, should not happen") } + faillen := len(q.fails) + if s.l.Level >= logrus.DebugLevel { s.l.WithFields(logrus.Fields{ "name": q.ref, @@ -185,25 +178,24 @@ func (s *solver) findValidVersion(q *versionQueue) error { "allLoaded": q.allLoaded, }).Debug("Beginning search through versionQueue for a valid version") } - for { - err = s.satisfiable(ProjectAtom{ + cur := q.current() + err := s.satisfiable(ProjectAtom{ Name: q.ref, - Version: q.current(), + Version: cur, }) if err == nil { // we have a good version, can return safely if s.l.Level >= logrus.DebugLevel { s.l.WithFields(logrus.Fields{ "name": q.ref, - "version": q.current().Info, + "version": cur.Info, }).Debug("Found acceptable version, returning out") } return nil } - err = q.advance() - if err != nil { + if q.advance(err) != nil { // Error on advance, have to bail out if s.l.Level >= logrus.WarnLevel { s.l.WithFields(logrus.Fields{ @@ -215,19 +207,26 @@ func (s *solver) findValidVersion(q *versionQueue) error { } if q.isExhausted() { // Queue is empty, bail with error - err = newSolveError(fmt.Sprintf("Exhausted queue for %q without finding a satisfactory version.", q.ref), mustResolve) if s.l.Level >= logrus.InfoLevel { - s.l.WithFields(logrus.Fields{ - "name": q.ref, - "err": err, - }).Info("Version queue was completely exhausted, marking project as failed") + s.l.WithField("name", q.ref).Info("Version queue was completely exhausted, marking project as failed") } break } } s.fail(s.sel.getDependenciesOn(q.ref)[0].Depender.Name) - return err + + // Return a compound error of all the new errors encountered during this + // attempt to find a new, valid version + var fails []failedVersion + if len(q.fails) > faillen { + fails = q.fails[faillen+1:] + } + + return &noVersionError{ + pn: q.ref, + fails: fails, + } } func (s *solver) getLockVersionIfValid(ref ProjectName) *ProjectAtom { @@ -291,6 +290,7 @@ func (s *solver) satisfiable(pi ProjectAtom) error { } deps := s.sel.getDependenciesOn(pi.Name) + var failparent []Dependency for _, dep := range deps { if !dep.Dep.Constraint.Admits(pi.Version) { if s.l.Level >= logrus.DebugLevel { @@ -301,14 +301,14 @@ func (s *solver) satisfiable(pi ProjectAtom) error { }).Debug("Marking other, selected project with conflicting constraint as failed") } s.fail(dep.Depender.Name) + failparent = append(failparent, dep) } } - // TODO msg - return &noVersionError{ - pn: pi.Name, - c: constraint, - deps: deps, + return &versionNotAllowedFailure{ + goal: pi, + failparent: failparent, + c: constraint, } } @@ -345,6 +345,8 @@ func (s *solver) satisfiable(pi ProjectAtom) error { } // No admissible versions - visit all siblings and identify the disagreement(s) + var failsib []Dependency + var nofailsib []Dependency for _, sibling := range siblings { if !sibling.Dep.Constraint.AdmitsAny(dep.Constraint) { if s.l.Level >= logrus.DebugLevel { @@ -354,16 +356,20 @@ func (s *solver) satisfiable(pi ProjectAtom) error { "depname": sibling.Depender.Name, "sibconstraint": sibling.Dep.Constraint.Body(), "newconstraint": dep.Constraint.Body(), - }).Debug("Marking other, selected project as failed because its constraint is disjoint with our input") + }).Debug("Marking other, selected project as failed because its constraint is disjoint with our testee") } s.fail(sibling.Depender.Name) + failsib = append(failsib, sibling) + } else { + nofailsib = append(nofailsib, sibling) } } - // TODO msg return &disjointConstraintFailure{ - pn: dep.Name, - deps: append(siblings, Dependency{Depender: pi, Dep: dep}), + goal: Dependency{Depender: pi, Dep: dep}, + failsib: failsib, + nofailsib: nofailsib, + c: constraint, } } @@ -380,11 +386,9 @@ func (s *solver) satisfiable(pi ProjectAtom) error { } s.fail(dep.Name) - // TODO msg - return &noVersionError{ - pn: dep.Name, - c: dep.Constraint, - deps: append(siblings, Dependency{Depender: pi, Dep: dep}), + return &constraintNotAllowedFailure{ + goal: Dependency{Depender: pi, Dep: dep}, + v: selected.Version, } } @@ -485,7 +489,8 @@ func (s *solver) backtrack() bool { s.unselectLast() // Advance the queue past the current version, which we know is bad - if q.advance() == nil && !q.isExhausted() { + // TODO is it feasible to make available the failure reason here? + if q.advance(nil) == nil && !q.isExhausted() { // Search for another acceptable version of this failed dep in its queue if s.findValidVersion(q) == nil { if s.l.Level >= logrus.InfoLevel { diff --git a/version_queue.go b/version_queue.go index 248d8794..39c29093 100644 --- a/version_queue.go +++ b/version_queue.go @@ -5,9 +5,15 @@ import ( "strings" ) +type failedVersion struct { + v Version + f error +} + type versionQueue struct { ref ProjectName pi []Version + fails []failedVersion sm SourceManager failed bool hasLock, allLoaded bool @@ -45,14 +51,20 @@ func (vq *versionQueue) current() Version { return Version{} } -func (vq *versionQueue) advance() (err error) { +func (vq *versionQueue) advance(fail error) (err error) { // The current version may have failed, but the next one hasn't vq.failed = false + if len(vq.pi) == 0 { + return + } + + vq.fails = append(vq.fails, failedVersion{ + v: vq.pi[0], + f: fail, + }) if vq.allLoaded { - if len(vq.pi) > 0 { - vq.pi = vq.pi[1:] - } + vq.pi = vq.pi[1:] return }