From 7a32e011e28c77ff0abed52513b536a39352dd01 Mon Sep 17 00:00:00 2001 From: Sunny Date: Sun, 3 Sep 2017 20:49:25 +0530 Subject: [PATCH 1/5] fix(manifest): validate project roots in manifest Validating ProjectRoot(s) require source manager, which is created after loading the project. Hence, ProjectRoot validation can't be done in existing validateManifest. This change adds `ValidateProjectRoots()` which validates only the Constraint and Override names to be valid ProjectRoot. Warnings are issued at stderr when invalid ProjectRoot is found in manifest. `ensure` and `status` call `ValidateProjectRoots()` expecitly. --- cmd/dep/ensure.go | 4 ++ cmd/dep/status.go | 4 ++ manifest.go | 23 ++++++++++ manifest_test.go | 110 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 141 insertions(+) diff --git a/cmd/dep/ensure.go b/cmd/dep/ensure.go index 583ae33e33..9683d2dd67 100644 --- a/cmd/dep/ensure.go +++ b/cmd/dep/ensure.go @@ -160,6 +160,10 @@ func (cmd *ensureCommand) Run(ctx *dep.Ctx, args []string) error { sm.UseDefaultSignalHandling() defer sm.Release() + if err := dep.ValidateProjectRoots(ctx, p.Manifest, sm); err != nil { + return err + } + params := p.MakeParams() if ctx.Verbose { params.TraceLogger = ctx.Err diff --git a/cmd/dep/status.go b/cmd/dep/status.go index 973033662a..a849b15cb3 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -190,6 +190,10 @@ func (cmd *statusCommand) Run(ctx *dep.Ctx, args []string) error { sm.UseDefaultSignalHandling() defer sm.Release() + if err := dep.ValidateProjectRoots(ctx, p.Manifest, sm); err != nil { + return err + } + var buf bytes.Buffer var out outputter switch { diff --git a/manifest.go b/manifest.go index 11472a64a5..74a64da273 100644 --- a/manifest.go +++ b/manifest.go @@ -156,6 +156,29 @@ func validateManifest(s string) ([]error, error) { return warns, nil } +// ValidateProjectRoots validates the project roots present in manifest. +func ValidateProjectRoots(c *Ctx, m *Manifest, sm gps.SourceManager) error { + projectRoots := make([]gps.ProjectRoot, 0, len(m.Constraints)+len(m.Ovr)) + for pr := range m.Constraints { + projectRoots = append(projectRoots, pr) + } + for pr := range m.Ovr { + projectRoots = append(projectRoots, pr) + } + + for _, pr := range projectRoots { + origPR, err := sm.DeduceProjectRoot(string(pr)) + if err != nil { + return errors.Wrapf(err, "could not deduce project root for %s", pr) + } + if origPR != pr { + c.Err.Printf("dep: WARNING: name %q in Gopkg.toml should be project root", pr) + } + } + + return nil +} + // readManifest returns a Manifest read from r and a slice of validation warnings. func readManifest(r io.Reader) (*Manifest, []error, error) { buf := &bytes.Buffer{} diff --git a/manifest_test.go b/manifest_test.go index c79f5e02b1..f206db8b82 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -5,7 +5,10 @@ package dep import ( + "bytes" "errors" + "io/ioutil" + "log" "reflect" "strings" "testing" @@ -372,3 +375,110 @@ func TestValidateManifest(t *testing.T) { } } } + +func TestValidateProjectRoots(t *testing.T) { + cases := []struct { + name string + manifest Manifest + wantError bool + wantWarn []string + }{ + { + name: "empty Manifest", + manifest: Manifest{}, + wantError: false, + wantWarn: []string{}, + }, + { + name: "valid project root", + manifest: Manifest{ + Constraints: map[gps.ProjectRoot]gps.ProjectProperties{ + gps.ProjectRoot("github.com/goland/dep"): { + Constraint: gps.Any(), + }, + }, + }, + wantError: false, + wantWarn: []string{}, + }, + { + name: "invalid project roots in Constraints and Overrides", + manifest: Manifest{ + Constraints: map[gps.ProjectRoot]gps.ProjectProperties{ + gps.ProjectRoot("github.com/golang/dep/foo"): { + Constraint: gps.Any(), + }, + gps.ProjectRoot("github.com/golang/go/xyz"): { + Constraint: gps.Any(), + }, + gps.ProjectRoot("github.com/golang/fmt"): { + Constraint: gps.Any(), + }, + }, + Ovr: map[gps.ProjectRoot]gps.ProjectProperties{ + gps.ProjectRoot("github.com/golang/mock/bar"): { + Constraint: gps.Any(), + }, + gps.ProjectRoot("github.com/golang/mock"): { + Constraint: gps.Any(), + }, + }, + }, + wantError: false, + wantWarn: []string{ + "name \"github.com/golang/dep/foo\" in Gopkg.toml should be project root", + "name \"github.com/golang/mock/bar\" in Gopkg.toml should be project root", + "name \"github.com/golang/go/xyz\" in Gopkg.toml should be project root", + }, + }, + { + name: "invalid source path", + manifest: Manifest{ + Constraints: map[gps.ProjectRoot]gps.ProjectProperties{ + gps.ProjectRoot("github.com/golang"): { + Constraint: gps.Any(), + }, + }, + }, + wantError: true, + wantWarn: []string{}, + }, + } + + h := test.NewHelper(t) + defer h.Cleanup() + + h.TempDir("src") + pwd := h.Path(".") + + // Capture the stderr to verify the warnings + stderrOutput := &bytes.Buffer{} + errLogger := log.New(stderrOutput, "", 0) + ctx := &Ctx{ + GOPATH: pwd, + Out: log.New(ioutil.Discard, "", 0), + Err: errLogger, + } + + sm, err := ctx.SourceManager() + h.Must(err) + defer sm.Release() + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + // Empty the buffer for every case + stderrOutput.Reset() + err := ValidateProjectRoots(ctx, &c.manifest, sm) + if err != nil && !c.wantError { + t.Fatalf("Unexpected error while validating project roots: %q", err) + } + + warnings := stderrOutput.String() + for _, warn := range c.wantWarn { + if !strings.Contains(warnings, warn) { + t.Fatalf("Expected ValidateProjectRoot warnings to contain: %q", warn) + } + } + }) + } +} From ee6e7ad043bbeeeb6085be98e49a725030afdaf5 Mon Sep 17 00:00:00 2001 From: Sunny Date: Mon, 4 Sep 2017 19:13:48 +0530 Subject: [PATCH 2/5] change ProjectRoot Validation warning message --- manifest.go | 2 +- manifest_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/manifest.go b/manifest.go index 74a64da273..f63b67a490 100644 --- a/manifest.go +++ b/manifest.go @@ -172,7 +172,7 @@ func ValidateProjectRoots(c *Ctx, m *Manifest, sm gps.SourceManager) error { return errors.Wrapf(err, "could not deduce project root for %s", pr) } if origPR != pr { - c.Err.Printf("dep: WARNING: name %q in Gopkg.toml should be project root", pr) + c.Err.Printf("dep: WARNING: the name for %q in Gopkg.toml should be changed to %q", pr, origPR) } } diff --git a/manifest_test.go b/manifest_test.go index f206db8b82..227f511558 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -426,9 +426,9 @@ func TestValidateProjectRoots(t *testing.T) { }, wantError: false, wantWarn: []string{ - "name \"github.com/golang/dep/foo\" in Gopkg.toml should be project root", - "name \"github.com/golang/mock/bar\" in Gopkg.toml should be project root", - "name \"github.com/golang/go/xyz\" in Gopkg.toml should be project root", + "the name for \"github.com/golang/dep/foo\" in Gopkg.toml should be changed to \"github.com/golang/dep\"", + "the name for \"github.com/golang/mock/bar\" in Gopkg.toml should be changed to \"github.com/golang/mock\"", + "the name for \"github.com/golang/go/xyz\" in Gopkg.toml should be changed to \"github.com/golang/go\"", }, }, { From 0e4d48c72c3ced9288c7d5f364f72873bf34359b Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 5 Sep 2017 19:45:23 +0530 Subject: [PATCH 3/5] manifest: make ValidateProjectRoots concurrent --- manifest.go | 44 +++++++++++++++++++++++++++++++++----------- manifest_test.go | 22 +++++++++++----------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/manifest.go b/manifest.go index f63b67a490..034a82ef07 100644 --- a/manifest.go +++ b/manifest.go @@ -11,6 +11,7 @@ import ( "reflect" "regexp" "sort" + "sync" "github.com/golang/dep/internal/gps" "github.com/pelletier/go-toml" @@ -22,10 +23,11 @@ const ManifestName = "Gopkg.toml" // Errors var ( - errInvalidConstraint = errors.New("\"constraint\" must be a TOML array of tables") - errInvalidOverride = errors.New("\"override\" must be a TOML array of tables") - errInvalidRequired = errors.New("\"required\" must be a TOML list of strings") - errInvalidIgnored = errors.New("\"ignored\" must be a TOML list of strings") + errInvalidConstraint = errors.New("\"constraint\" must be a TOML array of tables") + errInvalidOverride = errors.New("\"override\" must be a TOML array of tables") + errInvalidRequired = errors.New("\"required\" must be a TOML list of strings") + errInvalidIgnored = errors.New("\"ignored\" must be a TOML list of strings") + errInvalidProjectRoot = errors.New("ProjectRoot name validation failed") ) // Manifest holds manifest file data and implements gps.RootManifest. @@ -166,17 +168,37 @@ func ValidateProjectRoots(c *Ctx, m *Manifest, sm gps.SourceManager) error { projectRoots = append(projectRoots, pr) } + // Channel to receive all the errors + errorCh := make(chan error, len(projectRoots)) + + var wg sync.WaitGroup for _, pr := range projectRoots { - origPR, err := sm.DeduceProjectRoot(string(pr)) - if err != nil { - return errors.Wrapf(err, "could not deduce project root for %s", pr) - } - if origPR != pr { - c.Err.Printf("dep: WARNING: the name for %q in Gopkg.toml should be changed to %q", pr, origPR) + wg.Add(1) + go func(pr gps.ProjectRoot) { + defer wg.Done() + origPR, err := sm.DeduceProjectRoot(string(pr)) + if err != nil { + errorCh <- err + } else if origPR != pr { + errorCh <- fmt.Errorf("the name for %q should be changed to %q", pr, origPR) + } + }(pr) + } + + wg.Wait() + close(errorCh) + + var valErr error + if len(errorCh) > 0 { + valErr = errInvalidProjectRoot + c.Err.Printf("The Following issues were found in Gopkg.toml:\n\n") + for err := range errorCh { + c.Err.Println(" ✗", err.Error()) } + c.Err.Println() } - return nil + return valErr } // readManifest returns a Manifest read from r and a slice of validation warnings. diff --git a/manifest_test.go b/manifest_test.go index 227f511558..b751c9561d 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -380,13 +380,13 @@ func TestValidateProjectRoots(t *testing.T) { cases := []struct { name string manifest Manifest - wantError bool + wantError error wantWarn []string }{ { name: "empty Manifest", manifest: Manifest{}, - wantError: false, + wantError: nil, wantWarn: []string{}, }, { @@ -398,7 +398,7 @@ func TestValidateProjectRoots(t *testing.T) { }, }, }, - wantError: false, + wantError: nil, wantWarn: []string{}, }, { @@ -424,11 +424,11 @@ func TestValidateProjectRoots(t *testing.T) { }, }, }, - wantError: false, + wantError: errInvalidProjectRoot, wantWarn: []string{ - "the name for \"github.com/golang/dep/foo\" in Gopkg.toml should be changed to \"github.com/golang/dep\"", - "the name for \"github.com/golang/mock/bar\" in Gopkg.toml should be changed to \"github.com/golang/mock\"", - "the name for \"github.com/golang/go/xyz\" in Gopkg.toml should be changed to \"github.com/golang/go\"", + "the name for \"github.com/golang/dep/foo\" should be changed to \"github.com/golang/dep\"", + "the name for \"github.com/golang/mock/bar\" should be changed to \"github.com/golang/mock\"", + "the name for \"github.com/golang/go/xyz\" should be changed to \"github.com/golang/go\"", }, }, { @@ -440,7 +440,7 @@ func TestValidateProjectRoots(t *testing.T) { }, }, }, - wantError: true, + wantError: errInvalidProjectRoot, wantWarn: []string{}, }, } @@ -469,14 +469,14 @@ func TestValidateProjectRoots(t *testing.T) { // Empty the buffer for every case stderrOutput.Reset() err := ValidateProjectRoots(ctx, &c.manifest, sm) - if err != nil && !c.wantError { - t.Fatalf("Unexpected error while validating project roots: %q", err) + if err != c.wantError { + t.Fatalf("Unexpected error while validating project roots:\n\t(GOT): %v\n\t(WNT): %v", err, c.wantError) } warnings := stderrOutput.String() for _, warn := range c.wantWarn { if !strings.Contains(warnings, warn) { - t.Fatalf("Expected ValidateProjectRoot warnings to contain: %q", warn) + t.Fatalf("Expected ValidateProjectRoot errors to contain: %q", warn) } } }) From c0d23a6209501ed2308d32fab172e3810738d1ff Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 5 Sep 2017 20:37:06 +0530 Subject: [PATCH 4/5] manifest: move validate goroutine in var and reuse --- manifest.go | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/manifest.go b/manifest.go index 034a82ef07..c8013536f5 100644 --- a/manifest.go +++ b/manifest.go @@ -160,29 +160,28 @@ func validateManifest(s string) ([]error, error) { // ValidateProjectRoots validates the project roots present in manifest. func ValidateProjectRoots(c *Ctx, m *Manifest, sm gps.SourceManager) error { - projectRoots := make([]gps.ProjectRoot, 0, len(m.Constraints)+len(m.Ovr)) - for pr := range m.Constraints { - projectRoots = append(projectRoots, pr) - } - for pr := range m.Ovr { - projectRoots = append(projectRoots, pr) - } - // Channel to receive all the errors - errorCh := make(chan error, len(projectRoots)) + errorCh := make(chan error, len(m.Constraints)+len(m.Ovr)) var wg sync.WaitGroup - for _, pr := range projectRoots { + + validate := func(pr gps.ProjectRoot) { + defer wg.Done() + origPR, err := sm.DeduceProjectRoot(string(pr)) + if err != nil { + errorCh <- err + } else if origPR != pr { + errorCh <- fmt.Errorf("the name for %q should be changed to %q", pr, origPR) + } + } + + for pr := range m.Constraints { wg.Add(1) - go func(pr gps.ProjectRoot) { - defer wg.Done() - origPR, err := sm.DeduceProjectRoot(string(pr)) - if err != nil { - errorCh <- err - } else if origPR != pr { - errorCh <- fmt.Errorf("the name for %q should be changed to %q", pr, origPR) - } - }(pr) + go validate(pr) + } + for pr := range m.Ovr { + wg.Add(1) + go validate(pr) } wg.Wait() From fa811f269644662d7e2f0549c407b9f83cd76131 Mon Sep 17 00:00:00 2001 From: Sunny Date: Sat, 9 Sep 2017 18:25:16 +0530 Subject: [PATCH 5/5] manifest: fix typos --- manifest.go | 2 +- manifest_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/manifest.go b/manifest.go index c8013536f5..2140992210 100644 --- a/manifest.go +++ b/manifest.go @@ -190,7 +190,7 @@ func ValidateProjectRoots(c *Ctx, m *Manifest, sm gps.SourceManager) error { var valErr error if len(errorCh) > 0 { valErr = errInvalidProjectRoot - c.Err.Printf("The Following issues were found in Gopkg.toml:\n\n") + c.Err.Printf("The following issues were found in Gopkg.toml:\n\n") for err := range errorCh { c.Err.Println(" ✗", err.Error()) } diff --git a/manifest_test.go b/manifest_test.go index b751c9561d..7ac24448ca 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -393,7 +393,7 @@ func TestValidateProjectRoots(t *testing.T) { name: "valid project root", manifest: Manifest{ Constraints: map[gps.ProjectRoot]gps.ProjectProperties{ - gps.ProjectRoot("github.com/goland/dep"): { + gps.ProjectRoot("github.com/golang/dep"): { Constraint: gps.Any(), }, },