From cc112746e402919651ea7fbfca29fb031ba98e37 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Mon, 5 Jun 2017 15:20:01 -0500 Subject: [PATCH] Add constructors for feedback logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The feedback system was originally designed for reporting feedback as GOPATH was scanned, which didn’t work well for logging from the importers. This moves more of the logic around what should be logged back onto the caller and simplifies the ConstraintFeedback.LogFeedback function to print what it is given. --- cmd/dep/glide_importer.go | 11 +++- cmd/dep/godep_importer.go | 11 +++- cmd/dep/gopath_scanner.go | 11 +++- cmd/dep/root_analyzer.go | 50 +------------- .../testdata/glide/expected_import_output.txt | 6 +- internal/feedback/feedback.go | 66 +++++++++++++++---- internal/feedback/feedback_test.go | 63 ++++++++++++++---- 7 files changed, 133 insertions(+), 85 deletions(-) diff --git a/cmd/dep/glide_importer.go b/cmd/dep/glide_importer.go index f6c29c9dd9..c53ff391ea 100644 --- a/cmd/dep/glide_importer.go +++ b/cmd/dep/glide_importer.go @@ -208,6 +208,9 @@ func (g *glideImporter) buildProjectConstraint(pkg glidePackage) (pc gps.Project pc.Ident = gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot(pkg.Name), Source: pkg.Repository} pc.Constraint, err = deduceConstraint(pkg.Reference, pc.Ident, g.sm) + f := fb.NewConstraintFeedback(pc, fb.DepTypeImported) + f.LogFeedback(g.logger) + return } @@ -226,6 +229,10 @@ func (g *glideImporter) buildLockedProject(pkg glideLockedPackage) gps.LockedPro version = revision } - feedback(version, pi.ProjectRoot, fb.DepTypeImported, g.logger) - return gps.NewLockedProject(pi, version, nil) + lp := gps.NewLockedProject(pi, version, nil) + + f := fb.NewLockedProjectFeedback(lp, fb.DepTypeImported) + f.LogFeedback(g.logger) + + return lp } diff --git a/cmd/dep/godep_importer.go b/cmd/dep/godep_importer.go index 3d00acc4b8..e551e873c9 100644 --- a/cmd/dep/godep_importer.go +++ b/cmd/dep/godep_importer.go @@ -159,6 +159,10 @@ func (g *godepImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e func (g *godepImporter) buildProjectConstraint(pkg godepPackage) (pc gps.ProjectConstraint, err error) { pc.Ident = gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot(pkg.ImportPath)} pc.Constraint, err = deduceConstraint(pkg.Comment, pc.Ident, g.sm) + + f := fb.NewConstraintFeedback(pc, fb.DepTypeImported) + f.LogFeedback(g.logger) + return } @@ -175,8 +179,11 @@ func (g *godepImporter) buildLockedProject(pkg godepPackage) gps.LockedProject { version = gps.Revision(pkg.Rev) } - feedback(version, pi.ProjectRoot, fb.DepTypeImported, g.logger) - return gps.NewLockedProject(pi, version, nil) + lp := gps.NewLockedProject(pi, version, nil) + f := fb.NewLockedProjectFeedback(lp, fb.DepTypeImported) + f.LogFeedback(g.logger) + + return lp } // projectExistsInLock checks if the given import path already existing in diff --git a/cmd/dep/gopath_scanner.go b/cmd/dep/gopath_scanner.go index d0b77f2940..f6ca1d157e 100644 --- a/cmd/dep/gopath_scanner.go +++ b/cmd/dep/gopath_scanner.go @@ -91,7 +91,12 @@ func (g *gopathScanner) overlay(rootM *dep.Manifest, rootL *dep.Lock) { } rootM.Constraints[pkg] = prj v := g.pd.ondisk[pkg] - feedback(v, pkg, fb.DepTypeDirect, g.ctx.Err) + + pi := gps.ProjectIdentifier{ProjectRoot: pkg, Source: prj.Source} + f := fb.NewConstraintFeedback(gps.ProjectConstraint{Ident: pi, Constraint: v}, fb.DepTypeDirect) + f.LogFeedback(g.ctx.Err) + f = fb.NewLockedProjectFeedback(gps.NewLockedProject(pi, v, nil), fb.DepTypeDirect) + f.LogFeedback(g.ctx.Err) } // Keep track of which projects have been locked @@ -109,8 +114,8 @@ func (g *gopathScanner) overlay(rootM *dep.Manifest, rootL *dep.Lock) { lockedProjects[pkg] = true if _, isDirect := g.directDeps[string(pkg)]; !isDirect { - v := g.pd.ondisk[pkg] - feedback(v, pkg, fb.DepTypeTransitive, g.ctx.Err) + f := fb.NewLockedProjectFeedback(lp, fb.DepTypeTransitive) + f.LogFeedback(g.ctx.Err) } } diff --git a/cmd/dep/root_analyzer.go b/cmd/dep/root_analyzer.go index 785869b996..e4a9ff89d8 100644 --- a/cmd/dep/root_analyzer.go +++ b/cmd/dep/root_analyzer.go @@ -5,14 +5,12 @@ package main import ( - "encoding/hex" + "io/ioutil" + "log" "github.com/golang/dep" - fb "github.com/golang/dep/internal/feedback" "github.com/golang/dep/internal/gps" "github.com/pkg/errors" - "io/ioutil" - "log" ) // importer handles importing configuration from other dependency managers into @@ -149,50 +147,6 @@ func (a *rootAnalyzer) Info() (string, int) { return name, version } -// feedback logs project constraint as feedback to the user. -func feedback(v gps.Version, pr gps.ProjectRoot, depType string, logger *log.Logger) { - rev, version, branch := gps.VersionComponentStrings(v) - - // Check if it's a valid SHA1 digest and trim to 7 characters. - if len(rev) == 40 { - if _, err := hex.DecodeString(rev); err == nil { - // Valid SHA1 digest - rev = rev[0:7] - } - } - - // Get LockedVersion - var ver string - if version != "" { - ver = version - } else if branch != "" { - ver = branch - } - - cf := &fb.ConstraintFeedback{ - LockedVersion: ver, - Revision: rev, - ProjectPath: string(pr), - DependencyType: depType, - } - - // Get non-revision constraint if available - if c := getProjectPropertiesFromVersion(v).Constraint; c != nil { - cf.Version = c.String() - } - - // Attach ConstraintType for direct/imported deps based on locked version - if cf.DependencyType == fb.DepTypeDirect || cf.DependencyType == fb.DepTypeImported { - if cf.LockedVersion != "" { - cf.ConstraintType = fb.ConsTypeConstraint - } else { - cf.ConstraintType = fb.ConsTypeHint - } - } - - cf.LogFeedback(logger) -} - func lookupVersionForRevision(rev gps.Revision, pi gps.ProjectIdentifier, sm gps.SourceManager) (gps.Version, error) { // Find the version that goes with this revision, if any versions, err := sm.ListVersions(pi) diff --git a/cmd/dep/testdata/glide/expected_import_output.txt b/cmd/dep/testdata/glide/expected_import_output.txt index 7b599e9570..d828374e57 100644 --- a/cmd/dep/testdata/glide/expected_import_output.txt +++ b/cmd/dep/testdata/glide/expected_import_output.txt @@ -1,7 +1,7 @@ Detected glide configuration files... Converting from glide.yaml and glide.lock... - Using ^0.8.1 as initial constraint for imported dep github.com/sdboyer/deptest - Trying v0.8.1 (3f4c3be) as initial lock for imported dep github.com/sdboyer/deptest + Using master as initial constraint for imported dep github.com/sdboyer/deptest Using ^2.0.0 as initial constraint for imported dep github.com/sdboyer/deptestdos + Using master as initial constraint for imported dep github.com/golang/lint + Trying v0.8.1 (3f4c3be) as initial lock for imported dep github.com/sdboyer/deptest Trying v2.0.0 (5c60720) as initial lock for imported dep github.com/sdboyer/deptestdos - Using cb00e56 as initial hint for imported dep github.com/golang/lint diff --git a/internal/feedback/feedback.go b/internal/feedback/feedback.go index 17462097ac..eb6d3d66a9 100644 --- a/internal/feedback/feedback.go +++ b/internal/feedback/feedback.go @@ -5,8 +5,11 @@ package feedback import ( + "encoding/hex" "fmt" "log" + + "github.com/golang/dep/internal/gps" ) // Constraint types @@ -25,23 +28,52 @@ const DepTypeImported = "imported dep" // ConstraintFeedback holds project constraint feedback data type ConstraintFeedback struct { - Version, LockedVersion, Revision, ConstraintType, DependencyType, ProjectPath string + Constraint, LockedVersion, Revision, ConstraintType, DependencyType, ProjectPath string } -// LogFeedback logs the feedback +// NewConstraintFeedback builds a feedback entry for a constraint in the manifest. +func NewConstraintFeedback(pc gps.ProjectConstraint, depType string) *ConstraintFeedback { + cf := &ConstraintFeedback{ + Constraint: pc.Constraint.String(), + ProjectPath: string(pc.Ident.ProjectRoot), + DependencyType: depType, + } + + if _, ok := pc.Constraint.(gps.Revision); ok { + cf.ConstraintType = ConsTypeHint + } else { + cf.ConstraintType = ConsTypeConstraint + } + + return cf +} + +// NewLockedProjectFeedback builds a feedback entry for a project in the lock. +func NewLockedProjectFeedback(lp gps.LockedProject, depType string) *ConstraintFeedback { + cf := &ConstraintFeedback{ + ProjectPath: string(lp.Ident().ProjectRoot), + DependencyType: depType, + } + + switch vt := lp.Version().(type) { + case gps.PairedVersion: + cf.LockedVersion = vt.String() + cf.Revision = vt.Underlying().String() + case gps.UnpairedVersion: // Logically this should never occur, but handle for completeness sake + cf.LockedVersion = vt.String() + case gps.Revision: + cf.Revision = vt.String() + } + + return cf +} + +// LogFeedback logs feedback on changes made to the manifest or lock. func (cf ConstraintFeedback) LogFeedback(logger *log.Logger) { - // "Using" feedback for direct dep - if cf.DependencyType == DepTypeDirect || cf.DependencyType == DepTypeImported { - ver := cf.Version - // revision as version for hint - if cf.ConstraintType == ConsTypeHint { - ver = cf.Revision - } - logger.Printf(" %v", GetUsingFeedback(ver, cf.ConstraintType, cf.DependencyType, cf.ProjectPath)) + if cf.Constraint != "" { + logger.Printf(" %v", GetUsingFeedback(cf.Constraint, cf.ConstraintType, cf.DependencyType, cf.ProjectPath)) } - // No "Locking" feedback for hints. "Locking" feedback only for constraint - // and transitive dep - if cf.ConstraintType != ConsTypeHint { + if cf.LockedVersion != "" && cf.Revision != "" { logger.Printf(" %v", GetLockingFeedback(cf.LockedVersion, cf.Revision, cf.DependencyType, cf.ProjectPath)) } } @@ -62,6 +94,14 @@ func GetUsingFeedback(version, consType, depType, projectPath string) string { // Locking in v1.1.4 (bc29b4f) for direct dep github.com/foo/bar // Locking in master (436f39d) for transitive dep github.com/baz/qux func GetLockingFeedback(version, revision, depType, projectPath string) string { + // Check if it's a valid SHA1 digest and trim to 7 characters. + if len(revision) == 40 { + if _, err := hex.DecodeString(revision); err == nil { + // Valid SHA1 digest + revision = revision[0:7] + } + } + if depType == DepTypeImported { return fmt.Sprintf("Trying %s (%s) as initial lock for %s %s", version, revision, depType, projectPath) } diff --git a/internal/feedback/feedback_test.go b/internal/feedback/feedback_test.go index 1986cfc459..630bb1ac80 100644 --- a/internal/feedback/feedback_test.go +++ b/internal/feedback/feedback_test.go @@ -5,47 +5,82 @@ package feedback import ( + "bytes" + log2 "log" + "strings" "testing" + + "github.com/golang/dep/internal/gps" ) -func TestGetConstraintString(t *testing.T) { +func TestFeedback_Constraint(t *testing.T) { + ver, _ := gps.NewSemverConstraint("^1.0.0") + rev := gps.Revision("1b8edb3") + pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/foo/bar")} + cases := []struct { - feedback string + feedback *ConstraintFeedback want string }{ { - feedback: GetUsingFeedback("^1.0.0", ConsTypeConstraint, DepTypeDirect, "github.com/foo/bar"), + feedback: NewConstraintFeedback(gps.ProjectConstraint{Constraint: ver, Ident: pi}, DepTypeDirect), want: "Using ^1.0.0 as constraint for direct dep github.com/foo/bar", }, { - feedback: GetUsingFeedback("^1.0.0", ConsTypeConstraint, DepTypeImported, "github.com/foo/bar"), + feedback: NewConstraintFeedback(gps.ProjectConstraint{Constraint: ver, Ident: pi}, DepTypeImported), want: "Using ^1.0.0 as initial constraint for imported dep github.com/foo/bar", }, { - feedback: GetUsingFeedback("1b8edb3", ConsTypeHint, DepTypeDirect, "github.com/bar/baz"), - want: "Using 1b8edb3 as hint for direct dep github.com/bar/baz", + feedback: NewConstraintFeedback(gps.ProjectConstraint{Constraint: rev, Ident: pi}, DepTypeDirect), + want: "Using 1b8edb3 as hint for direct dep github.com/foo/bar", }, { - feedback: GetUsingFeedback("1b8edb3", ConsTypeHint, DepTypeImported, "github.com/bar/baz"), - want: "Using 1b8edb3 as initial hint for imported dep github.com/bar/baz", + feedback: NewConstraintFeedback(gps.ProjectConstraint{Constraint: rev, Ident: pi}, DepTypeImported), + want: "Using 1b8edb3 as initial hint for imported dep github.com/foo/bar", }, + } + + for _, c := range cases { + buf := &bytes.Buffer{} + log := log2.New(buf, "", 0) + c.feedback.LogFeedback(log) + got := strings.TrimSpace(buf.String()) + if c.want != got { + t.Errorf("Feedbacks are not expected: \n\t(GOT) '%s'\n\t(WNT) '%s'", got, c.want) + } + } +} + +func TestFeedback_LockedProject(t *testing.T) { + v := gps.NewVersion("v1.1.4").Is("bc29b4f") + b := gps.NewBranch("master").Is("436f39d") + pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/foo/bar")} + + cases := []struct { + feedback *ConstraintFeedback + want string + }{ { - feedback: GetLockingFeedback("v1.1.4", "bc29b4f", DepTypeDirect, "github.com/foo/bar"), + feedback: NewLockedProjectFeedback(gps.NewLockedProject(pi, v, nil), DepTypeDirect), want: "Locking in v1.1.4 (bc29b4f) for direct dep github.com/foo/bar", }, { - feedback: GetLockingFeedback("v1.1.4", "bc29b4f", DepTypeImported, "github.com/foo/bar"), + feedback: NewLockedProjectFeedback(gps.NewLockedProject(pi, v, nil), DepTypeImported), want: "Trying v1.1.4 (bc29b4f) as initial lock for imported dep github.com/foo/bar", }, { - feedback: GetLockingFeedback("master", "436f39d", DepTypeTransitive, "github.com/baz/qux"), - want: "Locking in master (436f39d) for transitive dep github.com/baz/qux", + feedback: NewLockedProjectFeedback(gps.NewLockedProject(pi, b, nil), DepTypeTransitive), + want: "Locking in master (436f39d) for transitive dep github.com/foo/bar", }, } for _, c := range cases { - if c.want != c.feedback { - t.Errorf("Feedbacks are not expected: \n\t(GOT) %v\n\t(WNT) %v", c.feedback, c.want) + buf := &bytes.Buffer{} + log := log2.New(buf, "", 0) + c.feedback.LogFeedback(log) + got := strings.TrimSpace(buf.String()) + if c.want != got { + t.Errorf("Feedbacks are not expected: \n\t(GOT) '%s'\n\t(WNT) '%s'", got, c.want) } } }