Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Add satisfiability check for case variants #1079

Merged
merged 14 commits into from
Sep 18, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions internal/gps/identifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
113 changes: 102 additions & 11 deletions internal/gps/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package gps

import (
"bytes"
"context"
"fmt"
"io/ioutil"
Expand All @@ -13,9 +14,11 @@ import (
"path"
"path/filepath"
"runtime"
"sort"
"sync"
"sync/atomic"
"testing"
"text/tabwriter"
"time"

"github.com/golang/dep/internal/test"
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand All @@ -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,
},
}

Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

NBD, but for cases like this, sometimes I like to define a struct for the test state, with a run method:

type testName struct {...}
func (testName) run(*testing.T) {...}

Then instead of:

f("folded first", folded, casevar1)
f("folded second", casevar1, folded)
f("both unfolded", casevar1, casevar2)

You get:

t.Run("folded first", testName{folded, casevar1}.run)
t.Run("folded second",testName{casevar1, folded}.run)
t.Run("both unfolded", testname{casevar1, casevar2}.run)

Which reads nicely, and avoids the multi-level closure and indentation hell that sometimes comes with these sorts of tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

mm yes, good point, i am being lazy with these. table-based declaration of them is more generally standard and does look better. i keep seeing your changes come in and thinking "i should remember to declare like that 😛 "

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
Expand Down
54 changes: 54 additions & 0 deletions internal/gps/satisfy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down
40 changes: 36 additions & 4 deletions internal/gps/selection.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand Down
Loading