Merge pull request sdboyer/gps#143 from sdboyer/refactor-hashing

Refactor hashing

bridge.go

@@ -40,6 +40,9 @@ type bridge struct {
 	// held by the solver that it ends up being easier and saner to do this.
 	s *solver
 
+	// Whether to sort version lists for downgrade.
+	down bool
+
 	// Simple, local cache of the root's PackageTree
 	crp *struct {
 		ptree PackageTree
@@ -58,17 +61,18 @@ type bridge struct {
 
 // Global factory func to create a bridge. This exists solely to allow tests to
 // override it with a custom bridge and sm.
-var mkBridge = func(s *solver, sm SourceManager) sourceBridge {
+var mkBridge = func(s *solver, sm SourceManager, down bool) sourceBridge {
 	return &bridge{
 		sm:     sm,
 		s:      s,
+		down:   down,
 		vlists: make(map[ProjectIdentifier][]Version),
 	}
 }
 
 func (b *bridge) GetManifestAndLock(id ProjectIdentifier, v Version) (Manifest, Lock, error) {
-	if id.ProjectRoot == ProjectRoot(b.s.rpt.ImportRoot) {
-		return b.s.rm, b.s.rl, nil
+	if b.s.rd.isRoot(id.ProjectRoot) {
+		return b.s.rd.rm, b.s.rd.rl, nil
 	}
 
 	b.s.mtr.push("b-gmal")
@@ -94,7 +98,7 @@ func (b *bridge) ListVersions(id ProjectIdentifier) ([]Version, error) {
 		return nil, err
 	}
 
-	if b.s.params.Downgrade {
+	if b.down {
 		SortForDowngrade(vl)
 	} else {
 		SortForUpgrade(vl)
@@ -120,7 +124,7 @@ func (b *bridge) SourceExists(id ProjectIdentifier) (bool, error) {
 }
 
 func (b *bridge) vendorCodeExists(id ProjectIdentifier) (bool, error) {
-	fi, err := os.Stat(filepath.Join(b.s.params.RootDir, "vendor", string(id.ProjectRoot)))
+	fi, err := os.Stat(filepath.Join(b.s.rd.dir, "vendor", string(id.ProjectRoot)))
 	if err != nil {
 		return false, err
 	} else if fi.IsDir() {
@@ -279,7 +283,7 @@ func (b *bridge) vtu(id ProjectIdentifier, v Version) versionTypeUnion {
 // The root project is handled separately, as the source manager isn't
 // responsible for that code.
 func (b *bridge) ListPackages(id ProjectIdentifier, v Version) (PackageTree, error) {
-	if id.ProjectRoot == ProjectRoot(b.s.rpt.ImportRoot) {
+	if b.s.rd.isRoot(id.ProjectRoot) {
 		panic("should never call ListPackages on root project")
 	}
 
@@ -327,7 +331,7 @@ func (b *bridge) breakLock() {
 		return
 	}
 
-	for _, lp := range b.s.rl.Projects() {
+	for _, lp := range b.s.rd.rl.Projects() {
 		if _, is := b.s.sel.selected(lp.pi); !is {
 			// TODO(sdboyer) use this as an opportunity to detect
 			// inconsistencies between upstream and the lock (e.g., moved tags)?

constraint_test.go

@@ -848,3 +848,58 @@ func TestVersionUnionPanicOnString(t *testing.T) {
 	}()
 	_ = versionTypeUnion{}.String()
 }
+
+func TestTypedConstraintString(t *testing.T) {
+	// Also tests typedVersionString(), as this nests down into that
+	rev := Revision("flooboofoobooo")
+	v1 := NewBranch("master")
+	v2 := NewBranch("test").Is(rev)
+	v3 := NewVersion("1.0.1")
+	v4 := NewVersion("v2.0.5")
+	v5 := NewVersion("2.0.5.2")
+
+	table := []struct {
+		in  Constraint
+		out string
+	}{
+		{
+			in:  anyConstraint{},
+			out: "any-*",
+		},
+		{
+			in:  noneConstraint{},
+			out: "none-",
+		},
+		{
+			in:  mkSVC("^1.0.0"),
+			out: "svc-^1.0.0",
+		},
+		{
+			in:  v1,
+			out: "b-master",
+		},
+		{
+			in:  v2,
+			out: "b-test-r-" + string(rev),
+		},
+		{
+			in:  v3,
+			out: "sv-1.0.1",
+		},
+		{
+			in:  v4,
+			out: "sv-v2.0.5",
+		},
+		{
+			in:  v5,
+			out: "pv-2.0.5.2",
+		},
+	}
+
+	for _, fix := range table {
+		got := typedConstraintString(fix.in)
+		if got != fix.out {
+			t.Errorf("Typed string for %v (%T) was not expected %q; got %q", fix.in, fix.in, fix.out, got)
+		}
+	}
+}

constraints.go

@@ -32,6 +32,26 @@ type Constraint interface {
 	_private()
 }
 
+// typedConstraintString emits the normal stringified representation of the
+// provided constraint, prefixed with a string that uniquely identifies the type
+// of the constraint.
+func typedConstraintString(c Constraint) string {
+	var prefix string
+
+	switch tc := c.(type) {
+	case Version:
+		return typedVersionString(tc)
+	case semverConstraint:
+		prefix = "svc"
+	case anyConstraint:
+		prefix = "any"
+	case noneConstraint:
+		prefix = "none"
+	}
+
+	return fmt.Sprintf("%s-%s", prefix, c.String())
+}
+
 func (semverConstraint) _private() {}
 func (anyConstraint) _private()    {}
 func (noneConstraint) _private()   {}

hash.go

@@ -3,7 +3,18 @@ package gps
 import (
 	"bytes"
 	"crypto/sha256"
+	"io"
 	"sort"
+	"strings"
+)
+
+// string headers used to demarcate sections in hash input creation
+const (
+	hhConstraints = "-CONSTRAINTS-"
+	hhImportsReqs = "-IMPORTS/REQS-"
+	hhIgnores     = "-IGNORES-"
+	hhOverrides   = "-OVERRIDES-"
+	hhAnalyzer    = "-ANALYZER-"
 )
 
 // HashInputs computes a hash digest of all data in SolveParams and the
@@ -17,99 +28,91 @@ import (
 //
 // (Basically, this is for memoization.)
 func (s *solver) HashInputs() (digest []byte) {
-	buf := new(bytes.Buffer)
-	s.writeHashingInputs(buf)
+	h := sha256.New()
+	s.writeHashingInputs(h)
 
-	hd := sha256.Sum256(buf.Bytes())
+	hd := h.Sum(nil)
 	digest = hd[:]
 	return
 }
 
-func (s *solver) writeHashingInputs(buf *bytes.Buffer) {
-	// Apply overrides to the constraints from the root. Otherwise, the hash
-	// would be computed on the basis of a constraint from root that doesn't
-	// actually affect solving.
-	p := s.ovr.overrideAll(s.rm.DependencyConstraints().merge(s.rm.TestDependencyConstraints()))
-
-	for _, pd := range p {
-		buf.WriteString(string(pd.Ident.ProjectRoot))
-		buf.WriteString(pd.Ident.Source)
-		// FIXME Constraint.String() is a surjective-only transformation - tags
-		// and branches with the same name are written out as the same string.
-		// This could, albeit rarely, result in input collisions when a real
-		// change has occurred.
-		buf.WriteString(pd.Constraint.String())
-	}
-
-	// Write each of the packages, or the errors that were found for a
-	// particular subpath, into the hash. We need to do this in a
-	// deterministic order, so expand and sort the map.
-	var pkgs []PackageOrErr
-	for _, perr := range s.rpt.Packages {
-		pkgs = append(pkgs, perr)
-	}
-	sort.Sort(sortPackageOrErr(pkgs))
-	for _, perr := range pkgs {
-		if perr.Err != nil {
-			buf.WriteString(perr.Err.Error())
-		} else {
-			buf.WriteString(perr.P.Name)
-			buf.WriteString(perr.P.CommentPath)
-			buf.WriteString(perr.P.ImportPath)
-			for _, imp := range perr.P.Imports {
-				if !isStdLib(imp) {
-					buf.WriteString(imp)
-				}
-			}
-			for _, imp := range perr.P.TestImports {
-				if !isStdLib(imp) {
-					buf.WriteString(imp)
-				}
-			}
+func (s *solver) writeHashingInputs(w io.Writer) {
+	writeString := func(s string) {
+		// Skip zero-length string writes; it doesn't affect the real hash
+		// calculation, and keeps misleading newlines from showing up in the
+		// debug output.
+		if s != "" {
+			// All users of writeHashingInputs cannot error on Write(), so just
+			// ignore it
+			w.Write([]byte(s))
 		}
 	}
 
-	// Write any require packages given in the root manifest.
-	if len(s.req) > 0 {
-		// Dump and sort the reqnores
-		req := make([]string, 0, len(s.req))
-		for pkg := range s.req {
-			req = append(req, pkg)
-		}
-		sort.Strings(req)
+	// We write "section headers" into the hash purely to ease scanning when
+	// debugging this input-constructing algorithm; as long as the headers are
+	// constant, then they're effectively a no-op.
+	writeString(hhConstraints)
+
+	// getApplicableConstraints will apply overrides, incorporate requireds,
+	// apply local ignores, drop stdlib imports, and finally trim out
+	// ineffectual constraints.
+	for _, pd := range s.rd.getApplicableConstraints() {
+		writeString(string(pd.Ident.ProjectRoot))
+		writeString(pd.Ident.Source)
+		writeString(typedConstraintString(pd.Constraint))
+	}
 
-		for _, reqp := range req {
-			buf.WriteString(reqp)
-		}
+	// Write out each discrete import, including those derived from requires.
+	writeString(hhImportsReqs)
+	imports := s.rd.externalImportList()
+	sort.Strings(imports)
+	for _, im := range imports {
+		writeString(im)
 	}
 
-	// Add the ignored packages, if any.
-	if len(s.ig) > 0 {
-		// Dump and sort the ignores
-		ig := make([]string, 0, len(s.ig))
-		for pkg := range s.ig {
+	// Add ignores, skipping any that point under the current project root;
+	// those will have already been implicitly incorporated by the import
+	// lister.
+	writeString(hhIgnores)
+	ig := make([]string, 0, len(s.rd.ig))
+	for pkg := range s.rd.ig {
+		if !strings.HasPrefix(pkg, s.rd.rpt.ImportRoot) || !isPathPrefixOrEqual(s.rd.rpt.ImportRoot, pkg) {
 			ig = append(ig, pkg)
 		}
-		sort.Strings(ig)
+	}
+	sort.Strings(ig)
 
-		for _, igp := range ig {
-			buf.WriteString(igp)
-		}
+	for _, igp := range ig {
+		writeString(igp)
 	}
 
-	for _, pc := range s.ovr.asSortedSlice() {
-		buf.WriteString(string(pc.Ident.ProjectRoot))
+	// Overrides *also* need their own special entry distinct from basic
+	// constraints, to represent the unique effects they can have on the entire
+	// solving process beyond root's immediate scope.
+	writeString(hhOverrides)
+	for _, pc := range s.rd.ovr.asSortedSlice() {
+		writeString(string(pc.Ident.ProjectRoot))
 		if pc.Ident.Source != "" {
-			buf.WriteString(pc.Ident.Source)
+			writeString(pc.Ident.Source)
 		}
 		if pc.Constraint != nil {
-			buf.WriteString(pc.Constraint.String())
+			writeString(typedConstraintString(pc.Constraint))
 		}
 	}
 
+	writeString(hhAnalyzer)
 	an, av := s.b.AnalyzerInfo()
-	buf.WriteString(an)
-	buf.WriteString(av.String())
+	writeString(an)
+	writeString(av.String())
+}
+
+// bytes.Buffer wrapper that injects newlines after each call to Write().
+type nlbuf bytes.Buffer
+
+func (buf *nlbuf) Write(p []byte) (n int, err error) {
+	n, _ = (*bytes.Buffer)(buf).Write(p)
+	(*bytes.Buffer)(buf).WriteByte('\n')
+	return n + 1, nil
 }
 
 // HashingInputsAsString returns the raw input data used by Solver.HashInputs()
@@ -118,10 +121,10 @@ func (s *solver) writeHashingInputs(buf *bytes.Buffer) {
 // This is primarily intended for debugging purposes.
 func HashingInputsAsString(s Solver) string {
 	ts := s.(*solver)
-	buf := new(bytes.Buffer)
+	buf := new(nlbuf)
 	ts.writeHashingInputs(buf)
 
-	return buf.String()
+	return (*bytes.Buffer)(buf).String()
 }
 
 type sortPackageOrErr []PackageOrErr