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/manager_test.go b/internal/gps/manager_test.go index 36dc700547..6ca70a5d50 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" @@ -349,8 +352,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,13 +368,33 @@ 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 { 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 @@ -390,16 +413,27 @@ 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"}, + 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: 6, + srccount: 1, }, } @@ -467,13 +501,70 @@ 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() } +func TestFSCaseSensitivityConvergesSources(t *testing.T) { + if testing.Short() { + t.Skip("Skipping slow test in short mode") + } + + f := func(name string, pi1, pi2 ProjectIdentifier) { + t.Run(name, func(t *testing.T) { + t.Parallel() + sm, clean := mkNaiveSM(t) + defer clean() + + sm.SyncSourceFor(pi1) + sg1, err := sm.srcCoord.getSourceGatewayFor(context.Background(), pi1) + 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) + } + + 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 func TestGetInfoListVersionsOrdering(t *testing.T) { // This test is quite slow, skip it on -short diff --git a/internal/gps/satisfy.go b/internal/gps/satisfy.go index 9638d766ff..abac0ea7ef 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.checkRootCaseConflicts(a, dep); err != nil { + return err + } if err = s.checkDepsConstraintsAllowable(a, dep); err != nil { return err } @@ -218,6 +221,57 @@ func (s *solver) checkIdentMatches(a atomWithPackages, cdep completeDep) error { return nil } +// 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) 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(current) + deps := s.sel.getDependenciesOn(curid) + for _, d := range deps { + 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, + 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..d06d8681a0 100644 --- a/internal/gps/selection.go +++ b/internal/gps/selection.go @@ -5,9 +5,19 @@ package gps 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 - 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 { @@ -59,13 +69,35 @@ 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) { + if current, has := s.foldRoots[toFold(string(pr))]; has && pr != current { + return true, current + } + + 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 + deps := s.deps[pr] + if len(deps) == 0 { + s.foldRoots[toFold(string(pr))] = pr + } + + s.deps[pr] = append(deps, dep) } 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))) + } + + dep, s.deps[id.ProjectRoot] = deps[dlen-1], deps[:dlen-1] return dep } diff --git a/internal/gps/solve_basic_test.go b/internal/gps/solve_basic_test.go index fe5eb7a443..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 { @@ -1343,13 +1346,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 +1365,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 +1375,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 +1395,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 +1415,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 +1443,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 +1454,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 +1479,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 5c8f43e303..668ab1dff2 100644 --- a/internal/gps/solve_bimodal_test.go +++ b/internal/gps/solve_bimodal_test.go @@ -683,6 +683,221 @@ var bimodalFixtures = map[string]bimodalFixture{ "a 1.0.0", ), }, + "simple case-only differences": { + 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")), + }, + 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 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"), + pkg("root", "foo", "bar", "Bar")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo")), + 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")}, + }, + }, + }, + }, + broken: "need to implement checking for import case variations *within* the root", + }, + "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")), + }, + 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")}, + }, + }, + }, + }, + broken: "need to implement checking for import case variations *within* a single project", + }, + "case variations across 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")), + }, + 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 (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. + "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")), + dsp(mkDepspec("bar 1.0.0"), + 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 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": { + 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")), + }, + 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": { + ds: []depspec{ + 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", "bar/subpkg"), + pkg("bar/subpkg")), + dsp(mkDepspec("quux 1.0.0"), + 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")}, + }, + }, + }, + }, + }, "alternate net address": { ds: []depspec{ dsp(mkDepspec("root 1.0.0", "foo from bar 2.0.0"), @@ -1105,6 +1320,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 { @@ -1187,14 +1405,36 @@ func newbmSM(bmf bimodalFixture) *bmSourceManager { } func (sm *bmSourceManager) ListPackages(id ProjectIdentifier, v Version) (pkgtree.PackageTree, error) { + // 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 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 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. + 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, froot, root, 1) + } ptree.Packages[pkg.path] = pkgtree.PackageOrErr{ P: pkgtree.Package{ ImportPath: pkg.path, @@ -1212,9 +1452,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 0d281920f5..5b8657744c 100644 --- a/internal/gps/solve_failures.go +++ b/internal/gps/solve_failures.go @@ -71,6 +71,93 @@ 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() +} + +// 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; 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; 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 { + 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 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), diff --git a/internal/gps/solver.go b/internal/gps/solver.go index 88f6e87b69..3553b66344 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), + foldRoots: make(map[string]ProjectRoot), + vu: s.vUnify, } s.unsel = &unselected{ sl: make([]bimodalIdentifier, 0), diff --git a/internal/gps/source.go b/internal/gps/source.go index 5f8b34219b..1d2530d402 100644 --- a/internal/gps/source.go +++ b/internal/gps/source.go @@ -83,41 +83,85 @@ 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)) } + + // 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) + 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 { + // 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 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 +180,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 +193,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 @@ -164,17 +208,32 @@ 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() 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 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 { // URL already had an entry in the main map; use that as the result. doReturn(sa, nil) 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() +}