From e64c82e377fd18f59579857078650caeac5d13b9 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 29 Aug 2017 00:13:04 -0400 Subject: [PATCH 01/14] Add satisfiability check for case variants --- internal/gps/satisfy.go | 28 ++++++++++++++++++++++ internal/gps/selection.go | 35 ++++++++++++++++++++++++++- internal/gps/solve_failures.go | 43 ++++++++++++++++++++++++++++++++++ internal/gps/solver.go | 5 ++-- 4 files changed, 108 insertions(+), 3 deletions(-) diff --git a/internal/gps/satisfy.go b/internal/gps/satisfy.go index 9638d766ff..c7031e65e7 100644 --- a/internal/gps/satisfy.go +++ b/internal/gps/satisfy.go @@ -54,6 +54,9 @@ func (s *solver) check(a atomWithPackages, pkgonly bool) error { if err = s.checkIdentMatches(a, dep); err != nil { return err } + if err = s.checkCaseConflicts(a, dep); err != nil { + return err + } if err = s.checkDepsConstraintsAllowable(a, dep); err != nil { return err } @@ -218,6 +221,31 @@ func (s *solver) checkIdentMatches(a atomWithPackages, cdep completeDep) error { return nil } +// checkCaseConflicts ensures that the ProjectRoot specified in the completeDep +// does not have case conflicts with any existing dependencies. +// +// We only need to check the ProjectRoot, rather than any packages therein, as +// the later check for package existence is case-sensitive. +func (s *solver) checkCaseConflicts(a atomWithPackages, cdep completeDep) error { + pr := cdep.workingConstraint.Ident.ProjectRoot + hasConflict, current := s.sel.findCaseConflicts(pr) + if !hasConflict { + return nil + } + + curid, _ := s.sel.getIdentFor(pr) + deps := s.sel.getDependenciesOn(curid) + for _, d := range deps { + s.fail(d.depender.id) + } + + return &caseMismatchFailure{ + goal: dependency{depender: a.a, dep: cdep}, + current: current, + failsib: deps, + } +} + // checkPackageImportsFromDepExist ensures that, if the dep is already selected, // the newly-required set of packages being placed on it exist and are valid. func (s *solver) checkPackageImportsFromDepExist(a atomWithPackages, cdep completeDep) error { diff --git a/internal/gps/selection.go b/internal/gps/selection.go index e29d83fe53..02ea1f2d5b 100644 --- a/internal/gps/selection.go +++ b/internal/gps/selection.go @@ -4,9 +4,12 @@ package gps +import "strings" + type selection struct { projects []selected deps map[ProjectRoot][]dependency + prLenMap map[int][]ProjectRoot vu *versionUnifier } @@ -59,13 +62,43 @@ func (s *selection) popSelection() (atomWithPackages, bool) { return sel.a, sel.first } +func (s *selection) findCaseConflicts(pr ProjectRoot) (bool, ProjectRoot) { + prlist, has := s.prLenMap[len(pr)] + if !has { + return false, "" + } + + // TODO(sdboyer) bug here if it's possible that strings.ToLower() could + // change the length of the string + lowpr := strings.ToLower(string(pr)) + for _, existing := range prlist { + if lowpr != strings.ToLower(string(existing)) { + continue + } + // If the converted strings match, then whatever we figure out here will + // be definitive - we needn't walk the rest of the slice. + if pr == existing { + return false, "" + } else { + return true, existing + } + } + + return false, "" +} + func (s *selection) pushDep(dep dependency) { - s.deps[dep.dep.Ident.ProjectRoot] = append(s.deps[dep.dep.Ident.ProjectRoot], dep) + pr := dep.dep.Ident.ProjectRoot + s.deps[pr] = append(s.deps[pr], dep) + s.prLenMap[len(pr)] = append(s.prLenMap[len(pr)], pr) } func (s *selection) popDep(id ProjectIdentifier) (dep dependency) { deps := s.deps[id.ProjectRoot] dep, s.deps[id.ProjectRoot] = deps[len(deps)-1], deps[:len(deps)-1] + + prlist := s.prLenMap[len(id.ProjectRoot)] + s.prLenMap[len(id.ProjectRoot)] = prlist[:len(prlist)-1] return dep } diff --git a/internal/gps/solve_failures.go b/internal/gps/solve_failures.go index 0d281920f5..3a35872693 100644 --- a/internal/gps/solve_failures.go +++ b/internal/gps/solve_failures.go @@ -71,6 +71,49 @@ func (e *noVersionError) traceString() string { return buf.String() } +// caseMismatchFailure occurs when there are import paths that differ only by +// case. The compiler disallows this case. +type caseMismatchFailure struct { + // goal is the depender atom that tried to introduce the case-varying name, + // along with the case-varying name. + goal dependency + // current is the specific casing of a ProjectRoot that is presently + // selected for all possible case variations of its contained unicode code + // points. + current ProjectRoot + // failsib is the list of active dependencies that have determined the + // specific casing for the target project. + failsib []dependency +} + +func (e *caseMismatchFailure) Error() string { + if len(e.failsib) == 1 { + str := "Could not introduce %s due to a case-only variation: it depends on %q, but %q was already established as the case variant for that project root by depender %s" + return fmt.Sprintf(str, a2vs(e.goal.depender), e.goal.dep.Ident.ProjectRoot, e.current, a2vs(e.failsib[0].depender)) + } + + var buf bytes.Buffer + + str := "Could not introduce %s due to a case-only variation: it depends on %q, but %q was already established as the case variant for that project root by the following other dependers:\n" + fmt.Fprintf(&buf, str, e.goal.dep.Ident.ProjectRoot, e.current, a2vs(e.goal.depender)) + + for _, c := range e.failsib { + fmt.Fprintf(&buf, "\t%s\n", a2vs(c.depender)) + } + + return buf.String() +} + +func (e *caseMismatchFailure) traceString() string { + var buf bytes.Buffer + fmt.Fprintf(&buf, "case-only variation in dependency on %q; %q already established by:\n", e.goal.dep.Ident.ProjectRoot, e.current) + for _, f := range e.failsib { + fmt.Fprintf(&buf, "%s\n", a2vs(f.depender)) + } + + return buf.String() +} + // disjointConstraintFailure occurs when attempting to introduce an atom that // itself has an acceptable version, but one of its dependency constraints is // disjoint with one or more dependency constraints already active for that diff --git a/internal/gps/solver.go b/internal/gps/solver.go index 88f6e87b69..e737326817 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -307,8 +307,9 @@ func Prepare(params SolveParameters, sm SourceManager) (Solver, error) { // Initialize stacks and queues s.sel = &selection{ - deps: make(map[ProjectRoot][]dependency), - vu: s.vUnify, + deps: make(map[ProjectRoot][]dependency), + prLenMap: make(map[int][]ProjectRoot), + vu: s.vUnify, } s.unsel = &unselected{ sl: make([]bimodalIdentifier, 0), From 5c96c1022c0516563132115e824501982b311fe5 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Wed, 30 Aug 2017 22:54:35 -0400 Subject: [PATCH 02/14] Use case folding to normalize lookup map key This lifts the exact folding algorithm and use pattern followed by the toolchain up into gps. Not only does it solve the strings.ToLower() inadequacy, but it means we're using the exact same logic the go compiler does to decide this same question. --- internal/gps/selection.go | 57 +++++++++++++++++++-------------------- internal/gps/solver.go | 6 ++--- internal/gps/strings.go | 51 +++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 32 deletions(-) create mode 100644 internal/gps/strings.go diff --git a/internal/gps/selection.go b/internal/gps/selection.go index 02ea1f2d5b..70ae24b4cd 100644 --- a/internal/gps/selection.go +++ b/internal/gps/selection.go @@ -4,13 +4,20 @@ package gps -import "strings" - type selection struct { + // projects is a stack of the atoms that have currently been selected by the + // solver. It can also be thought of as the vertex set of the current + // selection graph. projects []selected - deps map[ProjectRoot][]dependency - prLenMap map[int][]ProjectRoot - vu *versionUnifier + // deps records the set of dependers on a given ProjectRoot. It is + // essentially an adjacency list of *inbound* edges. + deps map[ProjectRoot][]dependency + // foldRoots records a mapping from a canonical, case-folded form of + // ProjectRoots to the particular case variant that has currently been + // selected. + foldRoots map[string]ProjectRoot + // The versoinUnifier in use for this solve run. + vu *versionUnifier } type selected struct { @@ -62,26 +69,12 @@ func (s *selection) popSelection() (atomWithPackages, bool) { return sel.a, sel.first } +// findCaseConflicts checks to see if the given ProjectRoot has a +// case-insensitive overlap with another, different ProjectRoot that's already +// been picked. func (s *selection) findCaseConflicts(pr ProjectRoot) (bool, ProjectRoot) { - prlist, has := s.prLenMap[len(pr)] - if !has { - return false, "" - } - - // TODO(sdboyer) bug here if it's possible that strings.ToLower() could - // change the length of the string - lowpr := strings.ToLower(string(pr)) - for _, existing := range prlist { - if lowpr != strings.ToLower(string(existing)) { - continue - } - // If the converted strings match, then whatever we figure out here will - // be definitive - we needn't walk the rest of the slice. - if pr == existing { - return false, "" - } else { - return true, existing - } + if current, has := s.foldRoots[toFold(string(pr))]; has && pr != current { + return true, current } return false, "" @@ -89,16 +82,22 @@ func (s *selection) findCaseConflicts(pr ProjectRoot) (bool, ProjectRoot) { func (s *selection) pushDep(dep dependency) { pr := dep.dep.Ident.ProjectRoot - s.deps[pr] = append(s.deps[pr], dep) - s.prLenMap[len(pr)] = append(s.prLenMap[len(pr)], pr) + deps := s.deps[pr] + s.deps[pr] = append(deps, dep) + + if len(deps) == 1 { + s.foldRoots[toFold(string(pr))] = pr + } } func (s *selection) popDep(id ProjectIdentifier) (dep dependency) { deps := s.deps[id.ProjectRoot] - dep, s.deps[id.ProjectRoot] = deps[len(deps)-1], deps[:len(deps)-1] + dlen := len(deps) + if dlen == 1 { + delete(s.foldRoots, toFold(string(id.ProjectRoot))) + } - prlist := s.prLenMap[len(id.ProjectRoot)] - s.prLenMap[len(id.ProjectRoot)] = prlist[:len(prlist)-1] + dep, s.deps[id.ProjectRoot] = deps[dlen-1], deps[:dlen-1] return dep } diff --git a/internal/gps/solver.go b/internal/gps/solver.go index e737326817..3553b66344 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -307,9 +307,9 @@ func Prepare(params SolveParameters, sm SourceManager) (Solver, error) { // Initialize stacks and queues s.sel = &selection{ - deps: make(map[ProjectRoot][]dependency), - prLenMap: make(map[int][]ProjectRoot), - vu: s.vUnify, + deps: make(map[ProjectRoot][]dependency), + foldRoots: make(map[string]ProjectRoot), + vu: s.vUnify, } s.unsel = &unselected{ sl: make([]bimodalIdentifier, 0), diff --git a/internal/gps/strings.go b/internal/gps/strings.go new file mode 100644 index 0000000000..6ca7b3d9f4 --- /dev/null +++ b/internal/gps/strings.go @@ -0,0 +1,51 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package gps + +import ( + "bytes" + "unicode" + "unicode/utf8" +) + +// toFold returns a string with the property that strings.EqualFold(s, t) iff +// ToFold(s) == ToFold(t) This lets us test a large set of strings for +// fold-equivalent duplicates without making a quadratic number of calls to +// EqualFold. Note that strings.ToUpper and strings.ToLower do not have the +// desired property in some corner cases. +// +// This is hoisted from toolchain internals: src/cmd/go/internal/str/str.go +func toFold(s string) string { + // Fast path: all ASCII, no upper case. + // Most paths look like this already. + for i := 0; i < len(s); i++ { + c := s[i] + if c >= utf8.RuneSelf || 'A' <= c && c <= 'Z' { + goto Slow + } + } + return s + +Slow: + var buf bytes.Buffer + for _, r := range s { + // SimpleFold(x) cycles to the next equivalent rune > x + // or wraps around to smaller values. Iterate until it wraps, + // and we've found the minimum value. + for { + r0 := r + r = unicode.SimpleFold(r0) + if r <= r0 { + break + } + } + // Exception to allow fast path above: A-Z => a-z + if 'A' <= r && r <= 'Z' { + r += 'a' - 'A' + } + buf.WriteRune(r) + } + return buf.String() +} From 65c8f22216b34fd06ad92f829f1e943858ce0b05 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 31 Aug 2017 01:14:10 -0400 Subject: [PATCH 03/14] Fixes off-by-one error; rename method --- internal/gps/identifier.go | 3 +++ internal/gps/satisfy.go | 8 ++++---- internal/gps/selection.go | 6 +++--- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/internal/gps/identifier.go b/internal/gps/identifier.go index 5b2c9d4f68..77fe357f7e 100644 --- a/internal/gps/identifier.go +++ b/internal/gps/identifier.go @@ -217,6 +217,9 @@ type completeDep struct { pl []string } +// dependency represents an incomplete edge in the depgraph. It has a +// fully-realized atom as the depender (the tail/source of the edge), and a set +// of requirements that any atom to be attached at the head/target must satisfy. type dependency struct { depender atom dep completeDep diff --git a/internal/gps/satisfy.go b/internal/gps/satisfy.go index c7031e65e7..8f1bfe21c2 100644 --- a/internal/gps/satisfy.go +++ b/internal/gps/satisfy.go @@ -54,7 +54,7 @@ func (s *solver) check(a atomWithPackages, pkgonly bool) error { if err = s.checkIdentMatches(a, dep); err != nil { return err } - if err = s.checkCaseConflicts(a, dep); err != nil { + if err = s.checkRootCaseConflicts(a, dep); err != nil { return err } if err = s.checkDepsConstraintsAllowable(a, dep); err != nil { @@ -221,19 +221,19 @@ func (s *solver) checkIdentMatches(a atomWithPackages, cdep completeDep) error { return nil } -// checkCaseConflicts ensures that the ProjectRoot specified in the completeDep +// checkRootCaseConflicts ensures that the ProjectRoot specified in the completeDep // does not have case conflicts with any existing dependencies. // // We only need to check the ProjectRoot, rather than any packages therein, as // the later check for package existence is case-sensitive. -func (s *solver) checkCaseConflicts(a atomWithPackages, cdep completeDep) error { +func (s *solver) checkRootCaseConflicts(a atomWithPackages, cdep completeDep) error { pr := cdep.workingConstraint.Ident.ProjectRoot hasConflict, current := s.sel.findCaseConflicts(pr) if !hasConflict { return nil } - curid, _ := s.sel.getIdentFor(pr) + curid, _ := s.sel.getIdentFor(current) deps := s.sel.getDependenciesOn(curid) for _, d := range deps { s.fail(d.depender.id) diff --git a/internal/gps/selection.go b/internal/gps/selection.go index 70ae24b4cd..d06d8681a0 100644 --- a/internal/gps/selection.go +++ b/internal/gps/selection.go @@ -83,11 +83,11 @@ func (s *selection) findCaseConflicts(pr ProjectRoot) (bool, ProjectRoot) { func (s *selection) pushDep(dep dependency) { pr := dep.dep.Ident.ProjectRoot deps := s.deps[pr] - s.deps[pr] = append(deps, dep) - - if len(deps) == 1 { + if len(deps) == 0 { s.foldRoots[toFold(string(pr))] = pr } + + s.deps[pr] = append(deps, dep) } func (s *selection) popDep(id ProjectIdentifier) (dep dependency) { From 538f5d33f0bf2b299b53cd05e6f92f479d750183 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 31 Aug 2017 01:15:07 -0400 Subject: [PATCH 04/14] Add a bunch more test cases --- internal/gps/solve_bimodal_test.go | 137 +++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) diff --git a/internal/gps/solve_bimodal_test.go b/internal/gps/solve_bimodal_test.go index 5c8f43e303..4ea97d4bdd 100644 --- a/internal/gps/solve_bimodal_test.go +++ b/internal/gps/solve_bimodal_test.go @@ -683,6 +683,143 @@ var bimodalFixtures = map[string]bimodalFixture{ "a 1.0.0", ), }, + "simple case-only variations": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo", "bar")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo", "Bar")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar")), + dsp(mkDepspec("Bar 1.0.0"), + pkg("Bar")), + }, + fail: &noVersionError{ + pn: mkPI("foo"), + fails: []failedVersion{ + { + v: NewVersion("1.0.0"), + f: &caseMismatchFailure{ + goal: mkDep("foo 1.0.0", "Bar 1.0.0", "Bar"), + current: ProjectRoot("bar"), + failsib: []dependency{mkDep("root", "bar 1.0.0", "bar")}, + }, + }, + }, + }, + }, + "case variations within root": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo", "bar", "Bar")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar")), + dsp(mkDepspec("Bar 1.0.0"), + pkg("Bar")), + }, + fail: &noVersionError{ + pn: mkPI("foo"), + fails: []failedVersion{ + { + v: NewVersion("1.0.0"), + f: &caseMismatchFailure{ + goal: mkDep("foo 1.0.0", "Bar 1.0.0", "Bar"), + current: ProjectRoot("bar"), + failsib: []dependency{mkDep("root", "foo 1.0.0", "foo")}, + }, + }, + }, + }, + }, + "case variations within single dep": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo", "bar", "Bar")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar")), + dsp(mkDepspec("Bar 1.0.0"), + pkg("Bar")), + }, + fail: &noVersionError{ + pn: mkPI("foo"), + fails: []failedVersion{ + { + v: NewVersion("1.0.0"), + f: &caseMismatchFailure{ + goal: mkDep("foo 1.0.0", "Bar 1.0.0", "Bar"), + current: ProjectRoot("bar"), + failsib: []dependency{mkDep("root", "foo 1.0.0", "foo")}, + }, + }, + }, + }, + }, + "case variations within multiple deps": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo", "bar")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo", "bar", "baz")), + dsp(mkDepspec("baz 1.0.0"), + pkg("baz", "Bar")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar")), + dsp(mkDepspec("Bar 1.0.0"), + pkg("Bar")), + }, + fail: &noVersionError{ + pn: mkPI("baz"), + fails: []failedVersion{ + { + v: NewVersion("1.0.0"), + f: &caseMismatchFailure{ + goal: mkDep("baz 1.0.0", "Bar 1.0.0", "Bar"), + current: ProjectRoot("bar"), + failsib: []dependency{ + mkDep("root", "bar 1.0.0", "bar"), + mkDep("foo 1.0.0", "bar 1.0.0", "bar"), + }, + }, + }, + }, + }, + }, + // This isn't actually as crazy as it might seem, as the root is defined by + // the addresser, not the addressee. It would occur if, e.g. something + // imports github.com/Sirupsen/logrus, as the contained subpackage at + // github.com/Sirupsen/logrus/hooks/syslog imports + // github.com/sirupsen/logrus. The only reason that doesn't blow up all the + // time is that most people only import the root package, not the syslog + // subpackage. + "case variations from self": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo", "bar")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar", "Bar")), + dsp(mkDepspec("Bar 1.0.0"), + pkg("Bar")), + }, + fail: &noVersionError{ + pn: mkPI("foo"), + fails: []failedVersion{ + { + v: NewVersion("1.0.0"), + f: &caseMismatchFailure{ + goal: mkDep("foo 1.0.0", "Bar 1.0.0", "Bar"), + current: ProjectRoot("bar"), + failsib: []dependency{mkDep("root", "foo 1.0.0", "foo")}, + }, + }, + }, + }, + }, "alternate net address": { ds: []depspec{ dsp(mkDepspec("root 1.0.0", "foo from bar 2.0.0"), From 8670e66f5ae6c0ddaeaf3a408e6e44306f96f66e Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 1 Sep 2017 19:15:01 -0400 Subject: [PATCH 05/14] Add wrongCaseFailure May or may not end up using this right away, but it'll be in place for when we have the slightly stronger failure case of a project being addressed with an incorrect case, as indicated by the project's way of referencing its own packages. --- internal/gps/solve_failures.go | 44 ++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/internal/gps/solve_failures.go b/internal/gps/solve_failures.go index 3a35872693..48f0c3a4af 100644 --- a/internal/gps/solve_failures.go +++ b/internal/gps/solve_failures.go @@ -114,6 +114,50 @@ func (e *caseMismatchFailure) traceString() string { return buf.String() } +// wrongCaseFailure occurs when one or more projects - A, B, ... - depend on +// another project - Z - with an incorrect case variant, as indicated by the +// case variant used internally by Z to reference its own packages. +// +// For example, github.com/sirupsen/logrus/hooks/syslog references itself via +// github.com/sirupsen/logrus, establishing that as the canonical case variant. +type wrongCaseFailure struct { + // correct is the canonical representation of the ProjectRoot + correct ProjectRoot + // goal is the incorrectly-referenced target project + goal dependency + // badcase is the list of active dependencies that have specified an + // incorrect ProjectRoot casing for the project in question. + badcase []dependency +} + +func (e *wrongCaseFailure) Error() string { + if len(e.badcase) == 1 { + str := "Could not introduce %s; its packages import each other using %q, establishing that as correct, but %s tried to import it as %q" + return fmt.Sprintf(str, a2vs(e.goal.depender), e.correct, a2vs(e.badcase[0].depender), e.badcase[0].dep.Ident.ProjectRoot) + } + + var buf bytes.Buffer + + str := "Could not introduce %s; its packages import each other using %q, establishing that as correct, but the following projects tried to import it as %q" + return fmt.Sprintf(str, a2vs(e.goal.depender), e.correct, e.badcase[0].dep.Ident.ProjectRoot) + + for _, c := range e.badcase { + fmt.Fprintf(&buf, "\t%s\n", a2vs(c.depender)) + } + + return buf.String() +} + +func (e *wrongCaseFailure) traceString() string { + var buf bytes.Buffer + fmt.Fprintf(&buf, "internal imports establish %q as correct casing; %q was used by:\n", e.correct, e.goal.dep.Ident.ProjectRoot) + for _, f := range e.badcase { + fmt.Fprintf(&buf, "%s\n", a2vs(f.depender)) + } + + return buf.String() +} + // disjointConstraintFailure occurs when attempting to introduce an atom that // itself has an acceptable version, but one of its dependency constraints is // disjoint with one or more dependency constraints already active for that From 6592101eebfd95616a8bfa2a02a076a03aac667a Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 2 Sep 2017 00:08:01 -0400 Subject: [PATCH 06/14] Case insensitivity magic for roots in fixtures This effectively makes them case-insensitive, case-preserving. --- internal/gps/solve_basic_test.go | 24 ++++++++++++++---------- internal/gps/solve_bimodal_test.go | 23 +++++++++++++++++------ internal/gps/solve_failures.go | 2 +- 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/internal/gps/solve_basic_test.go b/internal/gps/solve_basic_test.go index fe5eb7a443..267801a7f9 100644 --- a/internal/gps/solve_basic_test.go +++ b/internal/gps/solve_basic_test.go @@ -1343,13 +1343,13 @@ func (sm *depspecSourceManager) GetManifestAndLock(id ProjectIdentifier, v Versi v = pv.Unpair() } + src := toFold(id.normalizedSource()) for _, ds := range sm.specs { - if id.normalizedSource() == string(ds.n) && v.Matches(ds.v) { + if src == string(ds.n) && v.Matches(ds.v) { return ds, dummyLock{}, nil } } - // TODO(sdboyer) proper solver-type errors return nil, nil, fmt.Errorf("Project %s at version %s could not be found", id, v) } @@ -1362,7 +1362,7 @@ func (sm *depspecSourceManager) ExternalReach(id ProjectIdentifier, v Version) ( } func (sm *depspecSourceManager) ListPackages(id ProjectIdentifier, v Version) (pkgtree.PackageTree, error) { - pid := pident{n: ProjectRoot(id.normalizedSource()), v: v} + pid := pident{n: ProjectRoot(toFold(id.normalizedSource())), v: v} if pv, ok := v.(PairedVersion); ok && pv.Revision() == "FAKEREV" { // An empty rev may come in here because that's what we produce in // ListVersions(). If that's what we see, then just pretend like we have @@ -1372,7 +1372,7 @@ func (sm *depspecSourceManager) ListPackages(id ProjectIdentifier, v Version) (p if r, exists := sm.rm[pid]; exists { return pkgtree.PackageTree{ - ImportRoot: string(pid.n), + ImportRoot: id.normalizedSource(), Packages: map[string]pkgtree.PackageOrErr{ string(pid.n): { P: pkgtree.Package{ @@ -1392,7 +1392,7 @@ func (sm *depspecSourceManager) ListPackages(id ProjectIdentifier, v Version) (p for pid, r := range sm.rm { if uv.Matches(pid.v) { return pkgtree.PackageTree{ - ImportRoot: string(pid.n), + ImportRoot: id.normalizedSource(), Packages: map[string]pkgtree.PackageOrErr{ string(pid.n): { P: pkgtree.Package{ @@ -1412,8 +1412,9 @@ func (sm *depspecSourceManager) ListPackages(id ProjectIdentifier, v Version) (p func (sm *depspecSourceManager) ListVersions(id ProjectIdentifier) ([]PairedVersion, error) { var pvl []PairedVersion + src := toFold(id.normalizedSource()) for _, ds := range sm.specs { - if id.normalizedSource() != string(ds.n) { + if src != string(ds.n) { continue } @@ -1439,8 +1440,9 @@ func (sm *depspecSourceManager) ListVersions(id ProjectIdentifier) ([]PairedVers } func (sm *depspecSourceManager) RevisionPresentIn(id ProjectIdentifier, r Revision) (bool, error) { + src := toFold(id.normalizedSource()) for _, ds := range sm.specs { - if id.normalizedSource() == string(ds.n) && r == ds.v { + if src == string(ds.n) && r == ds.v { return true, nil } } @@ -1449,8 +1451,9 @@ func (sm *depspecSourceManager) RevisionPresentIn(id ProjectIdentifier, r Revisi } func (sm *depspecSourceManager) SourceExists(id ProjectIdentifier) (bool, error) { + src := toFold(id.normalizedSource()) for _, ds := range sm.specs { - if id.normalizedSource() == string(ds.n) { + if src == string(ds.n) { return true, nil } } @@ -1473,10 +1476,11 @@ func (sm *depspecSourceManager) ExportProject(id ProjectIdentifier, v Version, t } func (sm *depspecSourceManager) DeduceProjectRoot(ip string) (ProjectRoot, error) { + fip := toFold(ip) for _, ds := range sm.allSpecs() { n := string(ds.n) - if ip == n || strings.HasPrefix(ip, n+"/") { - return ProjectRoot(n), nil + if fip == n || strings.HasPrefix(fip, n+"/") { + return ProjectRoot(ip[:len(n)]), nil } } return "", fmt.Errorf("Could not find %s, or any parent, in list of known fixtures", ip) diff --git a/internal/gps/solve_bimodal_test.go b/internal/gps/solve_bimodal_test.go index 4ea97d4bdd..f8638890e6 100644 --- a/internal/gps/solve_bimodal_test.go +++ b/internal/gps/solve_bimodal_test.go @@ -691,8 +691,6 @@ var bimodalFixtures = map[string]bimodalFixture{ pkg("foo", "Bar")), dsp(mkDepspec("bar 1.0.0"), pkg("bar")), - dsp(mkDepspec("Bar 1.0.0"), - pkg("Bar")), }, fail: &noVersionError{ pn: mkPI("foo"), @@ -1324,14 +1322,26 @@ func newbmSM(bmf bimodalFixture) *bmSourceManager { } func (sm *bmSourceManager) ListPackages(id ProjectIdentifier, v Version) (pkgtree.PackageTree, error) { + src, fsrc := id.normalizedSource(), toFold(id.normalizedSource()) for k, ds := range sm.specs { // Cheat for root, otherwise we blow up b/c version is empty - if id.normalizedSource() == string(ds.n) && (k == 0 || ds.v.Matches(v)) { + if fsrc == string(ds.n) && (k == 0 || ds.v.Matches(v)) { + var replace bool + if src != string(ds.n) { + // We're in a case-varying lookup; ensure we replace the actual + // leading ProjectRoot portion of import paths with the literal + // string from the input. + replace = true + } + ptree := pkgtree.PackageTree{ - ImportRoot: id.normalizedSource(), + ImportRoot: src, Packages: make(map[string]pkgtree.PackageOrErr), } for _, pkg := range ds.pkgs { + if replace { + pkg.path = strings.Replace(pkg.path, fsrc, src, 1) + } ptree.Packages[pkg.path] = pkgtree.PackageOrErr{ P: pkgtree.Package{ ImportPath: pkg.path, @@ -1349,9 +1359,10 @@ func (sm *bmSourceManager) ListPackages(id ProjectIdentifier, v Version) (pkgtre } func (sm *bmSourceManager) GetManifestAndLock(id ProjectIdentifier, v Version, an ProjectAnalyzer) (Manifest, Lock, error) { + src := toFold(id.normalizedSource()) for _, ds := range sm.specs { - if id.normalizedSource() == string(ds.n) && v.Matches(ds.v) { - if l, exists := sm.lm[id.normalizedSource()+" "+v.String()]; exists { + if src == string(ds.n) && v.Matches(ds.v) { + if l, exists := sm.lm[src+" "+v.String()]; exists { return ds, l, nil } return ds, dummyLock{}, nil diff --git a/internal/gps/solve_failures.go b/internal/gps/solve_failures.go index 48f0c3a4af..22f0995b70 100644 --- a/internal/gps/solve_failures.go +++ b/internal/gps/solve_failures.go @@ -139,7 +139,7 @@ func (e *wrongCaseFailure) Error() string { var buf bytes.Buffer str := "Could not introduce %s; its packages import each other using %q, establishing that as correct, but the following projects tried to import it as %q" - return fmt.Sprintf(str, a2vs(e.goal.depender), e.correct, e.badcase[0].dep.Ident.ProjectRoot) + fmt.Fprintf(&buf, str, a2vs(e.goal.depender), e.correct, e.badcase[0].dep.Ident.ProjectRoot) for _, c := range e.badcase { fmt.Fprintf(&buf, "\t%s\n", a2vs(c.depender)) From 3d369c50a97e2eb77c262e77a8a09b8728819461 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 2 Sep 2017 03:13:30 -0400 Subject: [PATCH 07/14] Add the wrongCaseFailure, and scads more fixtures --- internal/gps/satisfy.go | 26 +++++++ internal/gps/solve_basic_test.go | 3 + internal/gps/solve_bimodal_test.go | 105 +++++++++++++++++++++++------ internal/gps/solve_failures.go | 4 +- internal/gps/solve_test.go | 6 ++ 5 files changed, 122 insertions(+), 22 deletions(-) diff --git a/internal/gps/satisfy.go b/internal/gps/satisfy.go index 8f1bfe21c2..abac0ea7ef 100644 --- a/internal/gps/satisfy.go +++ b/internal/gps/satisfy.go @@ -239,6 +239,32 @@ func (s *solver) checkRootCaseConflicts(a atomWithPackages, cdep completeDep) er s.fail(d.depender.id) } + // If a project has multiple packages that import each other, we treat that + // as establishing a canonical case variant for the ProjectRoot. It's possible, + // however, that that canonical variant is not the same one that others + // imported it under. If that's the situation, then we'll have arrived here + // when visiting the project, not its dependers, having misclassified its + // internal imports as external. That means the atomWithPackages will + // be the wrong case variant induced by the importers, and the cdep will be + // a link pointing back at the canonical case variant. + // + // If this is the case, use a special failure, wrongCaseFailure, that + // makes a stronger statement as to the correctness of case variants. + // + // TODO(sdboyer) This approach to marking failure is less than great, as + // this will mark the current atom as failed, as well, causing the + // backtracker to work through it. While that could prove fruitful, it's + // quite likely just to be wasted effort. Addressing this - if that's a good + // idea - would entail creating another path back out of checking to enable + // backjumping directly to the incorrect importers. + if current == a.a.id.ProjectRoot { + return &wrongCaseFailure{ + correct: pr, + goal: dependency{depender: a.a, dep: cdep}, + badcase: deps, + } + } + return &caseMismatchFailure{ goal: dependency{depender: a.a, dep: cdep}, current: current, diff --git a/internal/gps/solve_basic_test.go b/internal/gps/solve_basic_test.go index 267801a7f9..956fa9ae7a 100644 --- a/internal/gps/solve_basic_test.go +++ b/internal/gps/solve_basic_test.go @@ -399,6 +399,9 @@ type basicFixture struct { changeall bool // individual projects to change changelist []ProjectRoot + // if the fixture is currently broken/expected to fail, this has a message + // recording why + broken string } func (f basicFixture) name() string { diff --git a/internal/gps/solve_bimodal_test.go b/internal/gps/solve_bimodal_test.go index f8638890e6..5107811eac 100644 --- a/internal/gps/solve_bimodal_test.go +++ b/internal/gps/solve_bimodal_test.go @@ -683,7 +683,7 @@ var bimodalFixtures = map[string]bimodalFixture{ "a 1.0.0", ), }, - "simple case-only variations": { + "simple case-only differences": { ds: []depspec{ dsp(mkDepspec("root 0.0.0"), pkg("root", "foo", "bar")), @@ -706,6 +706,23 @@ var bimodalFixtures = map[string]bimodalFixture{ }, }, }, + "case variations acceptable with agreement": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo", "Bar", "baz")), + dsp(mkDepspec("baz 1.0.0"), + pkg("baz", "Bar")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar")), + }, + r: mksolution( + "foo 1.0.0", + "Bar 1.0.0", + "baz 1.0.0", + ), + }, "case variations within root": { ds: []depspec{ dsp(mkDepspec("root 0.0.0"), @@ -714,8 +731,6 @@ var bimodalFixtures = map[string]bimodalFixture{ pkg("foo")), dsp(mkDepspec("bar 1.0.0"), pkg("bar")), - dsp(mkDepspec("Bar 1.0.0"), - pkg("Bar")), }, fail: &noVersionError{ pn: mkPI("foo"), @@ -730,6 +745,7 @@ var bimodalFixtures = map[string]bimodalFixture{ }, }, }, + broken: "need to implement checking for import case variations *within* the root", }, "case variations within single dep": { ds: []depspec{ @@ -739,8 +755,6 @@ var bimodalFixtures = map[string]bimodalFixture{ pkg("foo", "bar", "Bar")), dsp(mkDepspec("bar 1.0.0"), pkg("bar")), - dsp(mkDepspec("Bar 1.0.0"), - pkg("Bar")), }, fail: &noVersionError{ pn: mkPI("foo"), @@ -755,8 +769,9 @@ var bimodalFixtures = map[string]bimodalFixture{ }, }, }, + broken: "need to implement checking for import case variations *within* a single project", }, - "case variations within multiple deps": { + "case variations across multiple deps": { ds: []depspec{ dsp(mkDepspec("root 0.0.0"), pkg("root", "foo", "bar")), @@ -766,8 +781,6 @@ var bimodalFixtures = map[string]bimodalFixture{ pkg("baz", "Bar")), dsp(mkDepspec("bar 1.0.0"), pkg("bar")), - dsp(mkDepspec("Bar 1.0.0"), - pkg("Bar")), }, fail: &noVersionError{ pn: mkPI("baz"), @@ -787,37 +800,86 @@ var bimodalFixtures = map[string]bimodalFixture{ }, }, // This isn't actually as crazy as it might seem, as the root is defined by - // the addresser, not the addressee. It would occur if, e.g. something - // imports github.com/Sirupsen/logrus, as the contained subpackage at + // the addresser, not the addressee. It would occur (to provide a + // real-as-of-this-writing example) if something imports + // github.com/Sirupsen/logrus, as the contained subpackage at // github.com/Sirupsen/logrus/hooks/syslog imports // github.com/sirupsen/logrus. The only reason that doesn't blow up all the // time is that most people only import the root package, not the syslog // subpackage. - "case variations from self": { + "canonical case is established by mutual self-imports": { ds: []depspec{ dsp(mkDepspec("root 0.0.0"), pkg("root", "foo")), dsp(mkDepspec("foo 1.0.0"), - pkg("foo", "bar")), + pkg("foo", "Bar")), dsp(mkDepspec("bar 1.0.0"), - pkg("bar", "Bar")), - dsp(mkDepspec("Bar 1.0.0"), - pkg("Bar")), + pkg("bar", "bar/subpkg"), + pkg("bar/subpkg")), }, fail: &noVersionError{ - pn: mkPI("foo"), + pn: mkPI("Bar"), fails: []failedVersion{ { v: NewVersion("1.0.0"), - f: &caseMismatchFailure{ - goal: mkDep("foo 1.0.0", "Bar 1.0.0", "Bar"), - current: ProjectRoot("bar"), - failsib: []dependency{mkDep("root", "foo 1.0.0", "foo")}, + f: &wrongCaseFailure{ + correct: ProjectRoot("bar"), + goal: mkDep("Bar 1.0.0", "bar 1.0.0", "bar"), + badcase: []dependency{mkDep("foo 1.0.0", "Bar 1.0.0", "Bar/subpkg")}, }, }, }, }, }, + "canonical case only applies if relevant imports are activated": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo", "Bar/subpkg")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar", "bar/subpkg"), + pkg("bar/subpkg")), + }, + r: mksolution( + "foo 1.0.0", + mklp("Bar 1.0.0", "subpkg"), + ), + }, + "simple case-only variations plus source variance": { + // COPYPASTA BELOW, FIX IT + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo", "Bar")), // TODO align the froms + dsp(mkDepspec("foo 1.0.0", "Bar from quux 1.0.0"), + pkg("foo", "Bar")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar")), + dsp(mkDepspec("quux 1.0.0"), + pkg("bar")), + }, + r: mksolution( + "foo 1.0.0", + "Bar from quux 1.0.0", + ), + }, + "case-only variations plus source variance with internal canonicality": { + // COPYPASTA BELOW, FIX IT + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo", "Bar")), + dsp(mkDepspec("foo 1.0.0", "Bar from quux 1.0.0"), + pkg("foo", "Bar")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar")), + dsp(mkDepspec("quux 1.0.0"), + pkg("bar")), + }, + r: mksolution( + "foo 1.0.0", + "Bar from quux 1.0.0", + ), + }, "alternate net address": { ds: []depspec{ dsp(mkDepspec("root 1.0.0", "foo from bar 2.0.0"), @@ -1240,6 +1302,9 @@ type bimodalFixture struct { ignore []string // pkgs to require require []string + // if the fixture is currently broken/expected to fail, this has a message + // recording why + broken string } func (f bimodalFixture) name() string { diff --git a/internal/gps/solve_failures.go b/internal/gps/solve_failures.go index 22f0995b70..dbcd79d094 100644 --- a/internal/gps/solve_failures.go +++ b/internal/gps/solve_failures.go @@ -132,13 +132,13 @@ type wrongCaseFailure struct { func (e *wrongCaseFailure) Error() string { if len(e.badcase) == 1 { - str := "Could not introduce %s; its packages import each other using %q, establishing that as correct, but %s tried to import it as %q" + str := "Could not introduce %s; mutual imports by its packages establish %q as the canonical casing for root, but %s tried to import it as %q" return fmt.Sprintf(str, a2vs(e.goal.depender), e.correct, a2vs(e.badcase[0].depender), e.badcase[0].dep.Ident.ProjectRoot) } var buf bytes.Buffer - str := "Could not introduce %s; its packages import each other using %q, establishing that as correct, but the following projects tried to import it as %q" + str := "Could not introduce %s; mutual imports by its packages establish %q as the canonical casing for root, but the following projects tried to import it as %q" fmt.Fprintf(&buf, str, a2vs(e.goal.depender), e.correct, e.badcase[0].dep.Ident.ProjectRoot) for _, c := range e.badcase { diff --git a/internal/gps/solve_test.go b/internal/gps/solve_test.go index 99744e8f1d..810b2ff2b8 100644 --- a/internal/gps/solve_test.go +++ b/internal/gps/solve_test.go @@ -60,6 +60,9 @@ func TestBasicSolves(t *testing.T) { func solveBasicsAndCheck(fix basicFixture, t *testing.T) (res Solution, err error) { sm := newdepspecSM(fix.ds, nil) + if fix.broken != "" { + t.Skip(fix.broken) + } params := SolveParameters{ RootDir: string(fix.ds[0].n), @@ -103,6 +106,9 @@ func TestBimodalSolves(t *testing.T) { func solveBimodalAndCheck(fix bimodalFixture, t *testing.T) (res Solution, err error) { sm := newbmSM(fix) + if fix.broken != "" { + t.Skip(fix.broken) + } params := SolveParameters{ RootDir: string(fix.ds[0].n), From 56ce3a4513a295c153d2067faa24ec0ca605d927 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 2 Sep 2017 12:18:07 -0400 Subject: [PATCH 08/14] Add test for import path case-sense handling in SM --- internal/gps/manager_test.go | 46 ++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index 36dc700547..0af227eaa2 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -474,6 +474,52 @@ func TestGetSources(t *testing.T) { clean() } +func TestFSCaseSensitivityConvergesSources(t *testing.T) { + if testing.Short() { + t.Skip("Skipping slow test in short mode") + } + + sm, clean := mkNaiveSM(t) + defer clean() + + pi1 := mkPI("github.com/sdboyer/deptest").normalize() + sm.SyncSourceFor(pi1) + sg1, err := sm.srcCoord.getSourceGatewayFor(context.Background(), pi1) + if err != nil { + t.Fatal(err) + } + + pi2 := mkPI("github.com/Sdboyer/deptest").normalize() + sm.SyncSourceFor(pi2) + sg2, err := sm.srcCoord.getSourceGatewayFor(context.Background(), pi2) + if err != nil { + t.Fatal(err) + } + + path1 := sg1.src.(*gitSource).repo.LocalPath() + t.Log("path1:", path1) + stat1, err := os.Stat(path1) + if err != nil { + t.Fatal(err) + } + path2 := sg2.src.(*gitSource).repo.LocalPath() + t.Log("path2:", path2) + stat2, err := os.Stat(path2) + if err != nil { + t.Fatal(err) + } + + same, count := os.SameFile(stat1, stat2), len(sm.srcCoord.srcs) + if same && count != 1 { + t.Log("are same, count", count) + t.Fatal("on case-insensitive filesystem, case-varying sources should have been folded together but were not") + } + if !same && count != 2 { + t.Log("not same, count", count) + t.Fatal("on case-sensitive filesystem, case-varying sources should not have been folded together, but were") + } +} + // Regression test for #32 func TestGetInfoListVersionsOrdering(t *testing.T) { // This test is quite slow, skip it on -short From 70d2f5b6950c69d130d12cfb96f91dbe9c80d2be Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 5 Sep 2017 01:45:16 -0400 Subject: [PATCH 09/14] Tests for combined source & case variance --- internal/gps/solve_bimodal_test.go | 62 ++++++++++++++++++++++-------- internal/gps/solve_failures.go | 4 +- 2 files changed, 47 insertions(+), 19 deletions(-) diff --git a/internal/gps/solve_bimodal_test.go b/internal/gps/solve_bimodal_test.go index 5107811eac..668ab1dff2 100644 --- a/internal/gps/solve_bimodal_test.go +++ b/internal/gps/solve_bimodal_test.go @@ -847,10 +847,9 @@ var bimodalFixtures = map[string]bimodalFixture{ ), }, "simple case-only variations plus source variance": { - // COPYPASTA BELOW, FIX IT ds: []depspec{ dsp(mkDepspec("root 0.0.0"), - pkg("root", "foo", "Bar")), // TODO align the froms + pkg("root", "foo", "bar")), dsp(mkDepspec("foo 1.0.0", "Bar from quux 1.0.0"), pkg("foo", "Bar")), dsp(mkDepspec("bar 1.0.0"), @@ -858,27 +857,46 @@ var bimodalFixtures = map[string]bimodalFixture{ dsp(mkDepspec("quux 1.0.0"), pkg("bar")), }, - r: mksolution( - "foo 1.0.0", - "Bar from quux 1.0.0", - ), + fail: &noVersionError{ + pn: mkPI("foo"), + fails: []failedVersion{ + { + v: NewVersion("1.0.0"), + f: &caseMismatchFailure{ + goal: mkDep("foo 1.0.0", "Bar from quux 1.0.0", "Bar"), + current: ProjectRoot("bar"), + failsib: []dependency{mkDep("root", "bar 1.0.0", "bar")}, + }, + }, + }, + }, }, "case-only variations plus source variance with internal canonicality": { - // COPYPASTA BELOW, FIX IT ds: []depspec{ - dsp(mkDepspec("root 0.0.0"), + dsp(mkDepspec("root 0.0.0", "Bar from quux 1.0.0"), pkg("root", "foo", "Bar")), dsp(mkDepspec("foo 1.0.0", "Bar from quux 1.0.0"), pkg("foo", "Bar")), dsp(mkDepspec("bar 1.0.0"), - pkg("bar")), + pkg("bar", "bar/subpkg"), + pkg("bar/subpkg")), dsp(mkDepspec("quux 1.0.0"), - pkg("bar")), + pkg("bar", "bar/subpkg"), + pkg("bar/subpkg")), + }, + fail: &noVersionError{ + pn: mkPI("Bar"), + fails: []failedVersion{ + { + v: NewVersion("1.0.0"), + f: &wrongCaseFailure{ + correct: ProjectRoot("bar"), + goal: mkDep("Bar from quux 1.0.0", "bar 1.0.0", "bar"), + badcase: []dependency{mkDep("root", "Bar 1.0.0", "Bar/subpkg")}, + }, + }, + }, }, - r: mksolution( - "foo 1.0.0", - "Bar from quux 1.0.0", - ), }, "alternate net address": { ds: []depspec{ @@ -1387,12 +1405,22 @@ func newbmSM(bmf bimodalFixture) *bmSourceManager { } func (sm *bmSourceManager) ListPackages(id ProjectIdentifier, v Version) (pkgtree.PackageTree, error) { - src, fsrc := id.normalizedSource(), toFold(id.normalizedSource()) + // Deal with address-based root-switching with both case folding and + // alternate sources. + var src, fsrc, root, froot string + src, fsrc = id.normalizedSource(), toFold(id.normalizedSource()) + if id.Source != "" { + root = string(id.ProjectRoot) + froot = toFold(root) + } else { + root, froot = src, fsrc + } + for k, ds := range sm.specs { // Cheat for root, otherwise we blow up b/c version is empty if fsrc == string(ds.n) && (k == 0 || ds.v.Matches(v)) { var replace bool - if src != string(ds.n) { + if root != string(ds.n) { // We're in a case-varying lookup; ensure we replace the actual // leading ProjectRoot portion of import paths with the literal // string from the input. @@ -1405,7 +1433,7 @@ func (sm *bmSourceManager) ListPackages(id ProjectIdentifier, v Version) (pkgtre } for _, pkg := range ds.pkgs { if replace { - pkg.path = strings.Replace(pkg.path, fsrc, src, 1) + pkg.path = strings.Replace(pkg.path, froot, root, 1) } ptree.Packages[pkg.path] = pkgtree.PackageOrErr{ P: pkgtree.Package{ diff --git a/internal/gps/solve_failures.go b/internal/gps/solve_failures.go index dbcd79d094..5b8657744c 100644 --- a/internal/gps/solve_failures.go +++ b/internal/gps/solve_failures.go @@ -132,13 +132,13 @@ type wrongCaseFailure struct { func (e *wrongCaseFailure) Error() string { if len(e.badcase) == 1 { - str := "Could not introduce %s; mutual imports by its packages establish %q as the canonical casing for root, but %s tried to import it as %q" + str := "Could not introduce %s; imports amongst its packages establish %q as the canonical casing for root, but %s tried to import it as %q" return fmt.Sprintf(str, a2vs(e.goal.depender), e.correct, a2vs(e.badcase[0].depender), e.badcase[0].dep.Ident.ProjectRoot) } var buf bytes.Buffer - str := "Could not introduce %s; mutual imports by its packages establish %q as the canonical casing for root, but the following projects tried to import it as %q" + str := "Could not introduce %s; imports amongst its packages establish %q as the canonical casing for root, but the following projects tried to import it as %q" fmt.Fprintf(&buf, str, a2vs(e.goal.depender), e.correct, e.badcase[0].dep.Ident.ProjectRoot) for _, c := range e.badcase { From bddd3f4bbac9f9e1c5d85c2d59eb7b3491a0f4b7 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 5 Sep 2017 09:44:23 -0400 Subject: [PATCH 10/14] Treat ProjectRoot as case-insensitive in srcCoord --- internal/gps/manager_test.go | 78 ++++++++++++++++++++---------------- internal/gps/source.go | 49 ++++++++++++++++------ 2 files changed, 81 insertions(+), 46 deletions(-) diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index 0af227eaa2..8a7b4d787b 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -479,45 +479,55 @@ func TestFSCaseSensitivityConvergesSources(t *testing.T) { t.Skip("Skipping slow test in short mode") } - sm, clean := mkNaiveSM(t) - defer clean() + f := func(name string, pi1, pi2 ProjectIdentifier) { + t.Run(name, func(t *testing.T) { + t.Parallel() + sm, clean := mkNaiveSM(t) + defer clean() - pi1 := mkPI("github.com/sdboyer/deptest").normalize() - sm.SyncSourceFor(pi1) - sg1, err := sm.srcCoord.getSourceGatewayFor(context.Background(), pi1) - if err != nil { - t.Fatal(err) - } + sm.SyncSourceFor(pi1) + sg1, err := sm.srcCoord.getSourceGatewayFor(context.Background(), pi1) + if err != nil { + t.Fatal(err) + } - pi2 := mkPI("github.com/Sdboyer/deptest").normalize() - sm.SyncSourceFor(pi2) - sg2, err := sm.srcCoord.getSourceGatewayFor(context.Background(), pi2) - if err != nil { - t.Fatal(err) - } + sm.SyncSourceFor(pi2) + sg2, err := sm.srcCoord.getSourceGatewayFor(context.Background(), pi2) + if err != nil { + t.Fatal(err) + } - path1 := sg1.src.(*gitSource).repo.LocalPath() - t.Log("path1:", path1) - stat1, err := os.Stat(path1) - if err != nil { - t.Fatal(err) - } - path2 := sg2.src.(*gitSource).repo.LocalPath() - t.Log("path2:", path2) - stat2, err := os.Stat(path2) - if err != nil { - t.Fatal(err) - } + path1 := sg1.src.(*gitSource).repo.LocalPath() + t.Log("path1:", path1) + stat1, err := os.Stat(path1) + if err != nil { + t.Fatal(err) + } + path2 := sg2.src.(*gitSource).repo.LocalPath() + t.Log("path2:", path2) + stat2, err := os.Stat(path2) + if err != nil { + t.Fatal(err) + } - same, count := os.SameFile(stat1, stat2), len(sm.srcCoord.srcs) - if same && count != 1 { - t.Log("are same, count", count) - t.Fatal("on case-insensitive filesystem, case-varying sources should have been folded together but were not") - } - if !same && count != 2 { - t.Log("not same, count", count) - t.Fatal("on case-sensitive filesystem, case-varying sources should not have been folded together, but were") + same, count := os.SameFile(stat1, stat2), len(sm.srcCoord.srcs) + if same && count != 1 { + t.Log("are same, count", count) + t.Fatal("on case-insensitive filesystem, case-varying sources should have been folded together but were not") + } + if !same && count != 2 { + t.Log("not same, count", count) + t.Fatal("on case-sensitive filesystem, case-varying sources should not have been folded together, but were") + } + }) } + + folded := mkPI("github.com/sdboyer/deptest").normalize() + casevar1 := mkPI("github.com/Sdboyer/deptest").normalize() + casevar2 := mkPI("github.com/SdboyeR/deptest").normalize() + f("folded first", folded, casevar1) + f("folded second", casevar1, folded) + f("both unfolded", casevar1, casevar2) } // Regression test for #32 diff --git a/internal/gps/source.go b/internal/gps/source.go index 5f8b34219b..bb683a1fc4 100644 --- a/internal/gps/source.go +++ b/internal/gps/source.go @@ -85,39 +85,58 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project } sc.srcmut.RUnlock() + // Without a direct match, we must fold the input name to a generally + // stable, caseless variant and primarily work from that. This ensures that + // on case-insensitive filesystems, we do not end up with multiple + // sourceGateways for paths that vary only by case. We perform folding + // unconditionally, independent of whether the underlying fs is + // case-sensitive, in order to ensure uniform behavior. + // + // This has significant implications. It is effectively deciding that the + // ProjectRoot portion of import paths are case-insensitive, which is by no + // means an invariant maintained by all hosting systems. If this presents a + // problem in practice, then we can explore expanding the deduction system + // to include case-sensitivity-for-roots metadata and treat it on a + // host-by-host basis. Such cases would still be rejected by the Go + // toolchain's compiler, though, and case-sensitivity in root names is + // likely to be at least frowned on if not disallowed by most hosting + // systems. So we follow this path, which is both a vastly simpler solution + // and one that seems quite likely to work in practice. + foldedNormalName := toFold(normalizedName) + // No gateway exists for this path yet; set up a proto, being careful to fold - // together simultaneous attempts on the same path. + // together simultaneous attempts on the same case-folded path. sc.psrcmut.Lock() - if chans, has := sc.protoSrcs[normalizedName]; has { + if chans, has := sc.protoSrcs[foldedNormalName]; has { // Another goroutine is already working on this normalizedName. Fold // in with that work by attaching our return channels to the list. rc := srcReturnChans{ ret: make(chan *sourceGateway, 1), err: make(chan error, 1), } - sc.protoSrcs[normalizedName] = append(chans, rc) + sc.protoSrcs[foldedNormalName] = append(chans, rc) sc.psrcmut.Unlock() return rc.awaitReturn() } - sc.protoSrcs[normalizedName] = []srcReturnChans{} + sc.protoSrcs[foldedNormalName] = []srcReturnChans{} sc.psrcmut.Unlock() doReturn := func(sg *sourceGateway, err error) { sc.psrcmut.Lock() if sg != nil { - for _, rc := range sc.protoSrcs[normalizedName] { + for _, rc := range sc.protoSrcs[foldedNormalName] { rc.ret <- sg } } else if err != nil { - for _, rc := range sc.protoSrcs[normalizedName] { + for _, rc := range sc.protoSrcs[foldedNormalName] { rc.err <- err } } else { panic("sg and err both nil") } - delete(sc.protoSrcs, normalizedName) + delete(sc.protoSrcs, foldedNormalName) sc.psrcmut.Unlock() } @@ -136,7 +155,7 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project // and bailing out if we find an entry. var srcGate *sourceGateway sc.srcmut.RLock() - if url, has := sc.nameToURL[normalizedName]; has { + if url, has := sc.nameToURL[foldedNormalName]; has { if srcGate, has := sc.srcs[url]; has { sc.srcmut.RUnlock() doReturn(srcGate, nil) @@ -149,10 +168,10 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project srcGate = newSourceGateway(pd.mb, sc.supervisor, sc.cachedir) // The normalized name is usually different from the source URL- e.g. - // github.com/golang/dep/internal/gps vs. https://github.com/golang/dep/internal/gps. But it's - // possible to arrive here with a full URL as the normalized name - and - // both paths *must* lead to the same sourceGateway instance in order to - // ensure disk access is correctly managed. + // github.com/sdboyer/gps vs. https://github.com/sdboyer/gps. But it's + // possible to arrive here with a full URL as the normalized name - and both + // paths *must* lead to the same sourceGateway instance in order to ensure + // disk access is correctly managed. // // Therefore, we now must query the sourceGateway to get the actual // sourceURL it's operating on, and ensure it's *also* registered at @@ -175,6 +194,12 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project sc.nameToURL[url] = url } + // Make sure we have both the folded and unfolded names recorded in the map, + // should they differ. + if normalizedName != foldedNormalName { + sc.nameToURL[foldedNormalName] = url + } + if sa, has := sc.srcs[url]; has { // URL already had an entry in the main map; use that as the result. doReturn(sa, nil) From a665a6e85ad5cf701c9d9a0eb6a9af564cc0c5b8 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 5 Sep 2017 09:52:13 -0400 Subject: [PATCH 11/14] Accommodate case folding in older mgr test --- internal/gps/manager_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index 8a7b4d787b..5dec172d7b 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -467,9 +467,10 @@ func TestGetSources(t *testing.T) { }) // nine entries (of which three are dupes): for each vcs, raw import path, - // the https url, and the http url - if len(sm.srcCoord.nameToURL) != 9 { - t.Errorf("Should have nine discrete entries in the nameToURL map, got %v", len(sm.srcCoord.nameToURL)) + // the https url, and the http url. also three more from case folding of + // github.com/Masterminds/VCSTestRepo -> github.com/masterminds/vcstestrepo + if len(sm.srcCoord.nameToURL) != 12 { + t.Errorf("Should have twelve discrete entries in the nameToURL map, got %v", len(sm.srcCoord.nameToURL)) } clean() } From 9e038e69dc5accf451d32bd1b8742f83eabe9b9e Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sun, 10 Sep 2017 23:22:07 -0400 Subject: [PATCH 12/14] Ensure URLs are folded as well Also add an up-front check for case variant map lookup misses, and update the map accordingly. --- internal/gps/manager_test.go | 26 ++++++++++++++++++-------- internal/gps/source.go | 34 +++++++++++++++++++++++++++++----- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index 5dec172d7b..5e3c13e8e8 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -349,8 +349,8 @@ func TestMgrMethodsFailWithBadPath(t *testing.T) { } type sourceCreationTestFixture struct { - roots []ProjectIdentifier - urlcount, srccount int + roots []ProjectIdentifier + namecount, srccount int } func (f sourceCreationTestFixture) run(t *testing.T) { @@ -365,8 +365,8 @@ func (f sourceCreationTestFixture) run(t *testing.T) { } } - if len(sm.srcCoord.nameToURL) != f.urlcount { - t.Errorf("want %v names in the name->url map, but got %v. contents: \n%v", f.urlcount, len(sm.srcCoord.nameToURL), sm.srcCoord.nameToURL) + if len(sm.srcCoord.nameToURL) != f.namecount { + t.Errorf("want %v names in the name->url map, but got %v. contents: \n%v", f.namecount, len(sm.srcCoord.nameToURL), sm.srcCoord.nameToURL) } if len(sm.srcCoord.srcs) != f.srccount { @@ -390,16 +390,26 @@ func TestSourceCreationCounts(t *testing.T) { mkPI("gopkg.in/sdboyer/gpkt.v2"), mkPI("gopkg.in/sdboyer/gpkt.v3"), }, - urlcount: 6, - srccount: 3, + namecount: 6, + srccount: 3, }, "gopkgin separation from github": { roots: []ProjectIdentifier{ mkPI("gopkg.in/sdboyer/gpkt.v1"), mkPI("github.com/sdboyer/gpkt"), }, - urlcount: 4, - srccount: 2, + namecount: 4, + srccount: 2, + }, + "case variance across path and URL-based access": { + roots: []ProjectIdentifier{ + ProjectIdentifier{ProjectRoot: ProjectRoot("github.com/sdboyer/gpkt"), Source: "https://github.com/Sdboyer/gpkt"}, + mkPI("github.com/sdboyer/gpkt"), + ProjectIdentifier{ProjectRoot: ProjectRoot("github.com/sdboyer/gpkt"), Source: "https://github.com/sdboyeR/gpkt"}, + mkPI("github.com/sdboyeR/gpkt"), + }, + namecount: 5, + srccount: 1, }, } diff --git a/internal/gps/source.go b/internal/gps/source.go index bb683a1fc4..9cb89ce4ac 100644 --- a/internal/gps/source.go +++ b/internal/gps/source.go @@ -83,7 +83,6 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project } panic(fmt.Sprintf("%q was URL for %q in nameToURL, but no corresponding srcGate in srcs map", url, normalizedName)) } - sc.srcmut.RUnlock() // Without a direct match, we must fold the input name to a generally // stable, caseless variant and primarily work from that. This ensures that @@ -103,6 +102,31 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project // systems. So we follow this path, which is both a vastly simpler solution // and one that seems quite likely to work in practice. foldedNormalName := toFold(normalizedName) + if foldedNormalName != normalizedName { + // If the folded name differs from the input name, then there may + // already be an entry for it in the nameToURL map, so check again. + if url, has := sc.nameToURL[foldedNormalName]; has { + // There was a match on the canonical folded variant. Upgrade to a + // write lock, so that future calls on this name don't need to + // burn cycles on folding. + sc.srcmut.RUnlock() + sc.srcmut.Lock() + // It may be possible that another goroutine could interleave + // between the unlock and re-lock. Even if they do, though, they'll + // only have recorded the same url value as we have here. In other + // words, these operations commute, so we can safely write here + // without checking again. + sc.nameToURL[normalizedName] = url + + srcGate, has := sc.srcs[url] + sc.srcmut.Unlock() + if has { + return srcGate, nil + } + panic(fmt.Sprintf("%q was URL for %q in nameToURL, but no corresponding srcGate in srcs map", url, normalizedName)) + } + } + sc.srcmut.RUnlock() // No gateway exists for this path yet; set up a proto, being careful to fold // together simultaneous attempts on the same case-folded path. @@ -140,7 +164,7 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project sc.psrcmut.Unlock() } - pd, err := sc.deducer.deduceRootPath(ctx, normalizedName) + pd, err := sc.deducer.deduceRootPath(ctx, foldedNormalName) if err != nil { // As in the deducer, don't cache errors so that externally-driven retry // strategies can be constructed. @@ -189,15 +213,15 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project defer sc.srcmut.Unlock() // Record the name -> URL mapping, making sure that we also get the // self-mapping. - sc.nameToURL[normalizedName] = url - if url != normalizedName { + sc.nameToURL[foldedNormalName] = url + if url != foldedNormalName { sc.nameToURL[url] = url } // Make sure we have both the folded and unfolded names recorded in the map, // should they differ. if normalizedName != foldedNormalName { - sc.nameToURL[foldedNormalName] = url + sc.nameToURL[normalizedName] = url } if sa, has := sc.srcs[url]; has { From 101c2d966432f738b0aa9d4ce7f9db93801131f6 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sun, 17 Sep 2017 22:11:51 -0400 Subject: [PATCH 13/14] gps: Add helpful debugging info to test logging Keeping track of what maps to what in the sourceGateway setup can be really tricky with all the combinations; in the event of failures in this test, this will show the mapping tables, which helps a lot with understanding the actual final state. --- internal/gps/manager_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index 5e3c13e8e8..b8137ca733 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -5,6 +5,7 @@ package gps import ( + "bytes" "context" "fmt" "io/ioutil" @@ -13,9 +14,11 @@ import ( "path" "path/filepath" "runtime" + "sort" "sync" "sync/atomic" "testing" + "text/tabwriter" "time" "github.com/golang/dep/internal/test" @@ -372,6 +375,26 @@ func (f sourceCreationTestFixture) run(t *testing.T) { if len(sm.srcCoord.srcs) != f.srccount { t.Errorf("want %v gateways in the sources map, but got %v", f.srccount, len(sm.srcCoord.srcs)) } + + var keys []string + for k := range sm.srcCoord.nameToURL { + keys = append(keys, k) + } + sort.Strings(keys) + + var buf bytes.Buffer + w := tabwriter.NewWriter(&buf, 0, 4, 2, ' ', 0) + fmt.Fprint(w, "NAME\tMAPPED URL\n") + for _, r := range keys { + fmt.Fprintf(w, "%s\t%s\n", r, sm.srcCoord.nameToURL[r]) + } + w.Flush() + t.Log("\n", buf.String()) + + t.Log("SRC KEYS") + for k := range sm.srcCoord.srcs { + t.Log(k) + } } // This test is primarily about making sure that the logic around folding From cbf031832db8d39b85a89710aeff6802901e14a2 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sun, 17 Sep 2017 22:13:42 -0400 Subject: [PATCH 14/14] gps: don't fold inputs to root deduction But, still preserve the rule that we record the canonical folded URL in memory, so that we can have non-canonical inputs come in first and still converge with subsequent canonical, or other-case-variant forms later. --- internal/gps/manager_test.go | 3 ++- internal/gps/source.go | 20 +++++++++++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index b8137ca733..6ca70a5d50 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -427,11 +427,12 @@ func TestSourceCreationCounts(t *testing.T) { "case variance across path and URL-based access": { roots: []ProjectIdentifier{ ProjectIdentifier{ProjectRoot: ProjectRoot("github.com/sdboyer/gpkt"), Source: "https://github.com/Sdboyer/gpkt"}, + ProjectIdentifier{ProjectRoot: ProjectRoot("github.com/sdboyer/gpkt"), Source: "https://github.com/SdbOyer/gpkt"}, mkPI("github.com/sdboyer/gpkt"), ProjectIdentifier{ProjectRoot: ProjectRoot("github.com/sdboyer/gpkt"), Source: "https://github.com/sdboyeR/gpkt"}, mkPI("github.com/sdboyeR/gpkt"), }, - namecount: 5, + namecount: 6, srccount: 1, }, } diff --git a/internal/gps/source.go b/internal/gps/source.go index 9cb89ce4ac..1d2530d402 100644 --- a/internal/gps/source.go +++ b/internal/gps/source.go @@ -102,7 +102,8 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project // systems. So we follow this path, which is both a vastly simpler solution // and one that seems quite likely to work in practice. foldedNormalName := toFold(normalizedName) - if foldedNormalName != normalizedName { + notFolded := foldedNormalName != normalizedName + if notFolded { // If the folded name differs from the input name, then there may // already be an entry for it in the nameToURL map, so check again. if url, has := sc.nameToURL[foldedNormalName]; has { @@ -164,7 +165,7 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project sc.psrcmut.Unlock() } - pd, err := sc.deducer.deduceRootPath(ctx, foldedNormalName) + pd, err := sc.deducer.deduceRootPath(ctx, normalizedName) if err != nil { // As in the deducer, don't cache errors so that externally-driven retry // strategies can be constructed. @@ -207,6 +208,14 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project return nil, err } + // If the normalizedName and foldedNormalName differ, then we're pretty well + // guaranteed that returned URL will also need folding into canonical form. + var unfoldedURL string + if notFolded { + unfoldedURL = url + url = toFold(url) + } + // We know we have a working srcGateway at this point, and need to // integrate it back into the main map. sc.srcmut.Lock() @@ -218,10 +227,11 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project sc.nameToURL[url] = url } - // Make sure we have both the folded and unfolded names recorded in the map, - // should they differ. - if normalizedName != foldedNormalName { + // Make sure we have both the folded and unfolded names and URLs recorded in + // the map, if the input needed folding. + if notFolded { sc.nameToURL[normalizedName] = url + sc.nameToURL[unfoldedURL] = url } if sa, has := sc.srcs[url]; has {