From 4724b1f0054ae7d5ab49b512a619fdc79dc897cb Mon Sep 17 00:00:00 2001 From: Ibrahim AshShohail Date: Fri, 29 Sep 2017 11:49:42 +0300 Subject: [PATCH 1/5] nits: letter casing and test table names Signed-off-by: Ibrahim AshShohail --- manifest.go | 12 +++---- manifest_test.go | 82 ++++++++++++++++++++++++++++++++---------------- 2 files changed, 61 insertions(+), 33 deletions(-) diff --git a/manifest.go b/manifest.go index 2f0f169ceb..5dd1348bc7 100644 --- a/manifest.go +++ b/manifest.go @@ -67,7 +67,7 @@ func validateManifest(s string) ([]error, error) { // Load the TomlTree from string tree, err := toml.Load(s) if err != nil { - return warns, errors.Wrap(err, "Unable to load TomlTree from string") + return warns, errors.Wrap(err, "unable to load TomlTree from string") } // Convert tree to a map manifest := tree.ToMap() @@ -114,7 +114,7 @@ func validateManifest(s string) ([]error, error) { } default: // unknown/invalid key - warns = append(warns, fmt.Errorf("Invalid key %q in %q", key, prop)) + warns = append(warns, fmt.Errorf("invalid key %q in %q", key, prop)) } } } @@ -152,7 +152,7 @@ func validateManifest(s string) ([]error, error) { } } default: - warns = append(warns, fmt.Errorf("Unknown field in manifest: %v", prop)) + warns = append(warns, fmt.Errorf("unknown field in manifest: %v", prop)) } } @@ -206,18 +206,18 @@ func readManifest(r io.Reader) (*Manifest, []error, error) { buf := &bytes.Buffer{} _, err := buf.ReadFrom(r) if err != nil { - return nil, nil, errors.Wrap(err, "Unable to read byte stream") + return nil, nil, errors.Wrap(err, "unable to read byte stream") } warns, err := validateManifest(buf.String()) if err != nil { - return nil, warns, errors.Wrap(err, "Manifest validation failed") + return nil, warns, errors.Wrap(err, "manifest validation failed") } raw := rawManifest{} err = toml.Unmarshal(buf.Bytes(), &raw) if err != nil { - return nil, warns, errors.Wrap(err, "Unable to parse the manifest as TOML") + return nil, warns, errors.Wrap(err, "unable to parse the manifest as TOML") } m, err := fromRawManifest(raw) diff --git a/manifest_test.go b/manifest_test.go index 76e03768dc..7076a3f255 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -25,7 +25,7 @@ func TestReadManifest(t *testing.T) { defer mf.Close() got, _, err := readManifest(mf) if err != nil { - t.Fatalf("Should have read Manifest correctly, but got err %q", err) + t.Fatalf("should have read manifest correctly, but got err %q", err) } c, _ := gps.NewSemverConstraint("^0.12.0") @@ -80,7 +80,7 @@ func TestWriteManifest(t *testing.T) { got, err := m.MarshalTOML() if err != nil { - t.Fatalf("Error while marshaling valid manifest to TOML: %q", err) + t.Fatalf("error while marshaling valid manifest to TOML: %q", err) } if string(got) != want { @@ -89,7 +89,7 @@ func TestWriteManifest(t *testing.T) { t.Fatal(err) } } else { - t.Errorf("Valid manifest did not marshal to TOML as expected:\n\t(GOT): %s\n\t(WNT): %s", string(got), want) + t.Errorf("valid manifest did not marshal to TOML as expected:\n\t(GOT): %s\n\t(WNT): %s", string(got), want) } } } @@ -113,20 +113,22 @@ func TestReadManifestErrors(t *testing.T) { defer mf.Close() _, _, err = readManifest(mf) if err == nil { - t.Errorf("Reading manifest with %s should have caused error, but did not", tst.name) + t.Errorf("reading manifest with %s should have caused error, but did not", tst.name) } else if !strings.Contains(err.Error(), tst.name) { - t.Errorf("Unexpected error %q; expected %s error", err, tst.name) + t.Errorf("unexpected error %q; expected %s error", err, tst.name) } } } func TestValidateManifest(t *testing.T) { cases := []struct { + name string tomlString string wantWarn []error wantError error }{ { + name: "valid required", tomlString: ` required = ["github.com/foo/bar"] `, @@ -134,6 +136,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "invalid required", tomlString: ` required = "github.com/foo/bar" `, @@ -141,6 +144,7 @@ func TestValidateManifest(t *testing.T) { wantError: errInvalidRequired, }, { + name: "empty required", tomlString: ` required = [] `, @@ -148,6 +152,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "invalid required list", tomlString: ` required = [1, 2, 3] `, @@ -155,6 +160,7 @@ func TestValidateManifest(t *testing.T) { wantError: errInvalidRequired, }, { + name: "invalid required format", tomlString: ` [[required]] name = "foo" @@ -163,6 +169,7 @@ func TestValidateManifest(t *testing.T) { wantError: errInvalidRequired, }, { + name: "valid ignored", tomlString: ` ignored = ["foo"] `, @@ -170,6 +177,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "invalid ignored", tomlString: ` ignored = "foo" `, @@ -177,6 +185,7 @@ func TestValidateManifest(t *testing.T) { wantError: errInvalidIgnored, }, { + name: "empty ignored", tomlString: ` ignored = [] `, @@ -184,6 +193,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "invalid ignored list", tomlString: ` ignored = [1, 2, 3] `, @@ -191,6 +201,7 @@ func TestValidateManifest(t *testing.T) { wantError: errInvalidIgnored, }, { + name: "invalid ignored format", tomlString: ` [[ignored]] name = "foo" @@ -199,6 +210,7 @@ func TestValidateManifest(t *testing.T) { wantError: errInvalidIgnored, }, { + name: "valid metadata", tomlString: ` [metadata] authors = "foo" @@ -208,6 +220,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "invalid metadata", tomlString: ` foo = "some-value" version = 14 @@ -220,13 +233,14 @@ func TestValidateManifest(t *testing.T) { version = "" `, wantWarn: []error{ - errors.New("Unknown field in manifest: foo"), - errors.New("Unknown field in manifest: bar"), - errors.New("Unknown field in manifest: version"), + errors.New("unknown field in manifest: foo"), + errors.New("unknown field in manifest: bar"), + errors.New("unknown field in manifest: version"), }, wantError: nil, }, { + name: "invalid metadata format", tomlString: ` metadata = "project-name" @@ -237,6 +251,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "plain constraint", tomlString: ` [[constraint]] name = "github.com/foo/bar" @@ -245,6 +260,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "empty constraint", tomlString: ` [[constraint]] `, @@ -252,6 +268,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "invalid constraint", tomlString: ` constraint = "foo" `, @@ -259,6 +276,7 @@ func TestValidateManifest(t *testing.T) { wantError: errInvalidConstraint, }, { + name: "invalid constraint list", tomlString: ` constraint = ["foo", "bar"] `, @@ -266,6 +284,7 @@ func TestValidateManifest(t *testing.T) { wantError: errInvalidConstraint, }, { + name: "valid override", tomlString: ` [[override]] name = "github.com/foo/bar" @@ -274,6 +293,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "empty override", tomlString: ` [[override]] `, @@ -281,6 +301,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "invalid override", tomlString: ` override = "bar" `, @@ -288,6 +309,7 @@ func TestValidateManifest(t *testing.T) { wantError: errInvalidOverride, }, { + name: "invalid override list", tomlString: ` override = ["foo", "bar"] `, @@ -295,6 +317,7 @@ func TestValidateManifest(t *testing.T) { wantError: errInvalidOverride, }, { + name: "invalid fields", tomlString: ` [[constraint]] name = "github.com/foo/bar" @@ -306,14 +329,15 @@ func TestValidateManifest(t *testing.T) { nick = "foo" `, wantWarn: []error{ - errors.New("Invalid key \"location\" in \"constraint\""), - errors.New("Invalid key \"link\" in \"constraint\""), - errors.New("Invalid key \"nick\" in \"override\""), + errors.New("invalid key \"location\" in \"constraint\""), + errors.New("invalid key \"link\" in \"constraint\""), + errors.New("invalid key \"nick\" in \"override\""), errors.New("metadata in \"constraint\" should be a TOML table"), }, wantError: nil, }, { + name: "constraint metadata", tomlString: ` [[constraint]] name = "github.com/foo/bar" @@ -325,6 +349,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "invalid revision", tomlString: ` [[constraint]] name = "github.com/foo/bar" @@ -334,6 +359,7 @@ func TestValidateManifest(t *testing.T) { wantError: nil, }, { + name: "invalid hg revision", tomlString: ` [[constraint]] name = "foobar.com/hg" @@ -355,24 +381,26 @@ func TestValidateManifest(t *testing.T) { } for _, c := range cases { - errs, err := validateManifest(c.tomlString) + t.Run(c.name, func(t *testing.T) { + errs, err := validateManifest(c.tomlString) - // compare validation errors - if err != c.wantError { - t.Fatalf("Manifest errors are not as expected: \n\t(GOT) %v \n\t(WNT) %v", err, c.wantError) - } + // compare validation errors + if err != c.wantError { + t.Fatalf("manifest errors are not as expected: \n\t(GOT) %v \n\t(WNT) %v", err, c.wantError) + } - // compare length of error slice - if len(errs) != len(c.wantWarn) { - t.Fatalf("Number of manifest errors are not as expected: \n\t(GOT) %v errors(%v)\n\t(WNT) %v errors(%v).", len(errs), errs, len(c.wantWarn), c.wantWarn) - } + // compare length of error slice + if len(errs) != len(c.wantWarn) { + t.Fatalf("number of manifest errors are not as expected: \n\t(GOT) %v errors(%v)\n\t(WNT) %v errors(%v).", len(errs), errs, len(c.wantWarn), c.wantWarn) + } - // check if the expected errors exist in actual errors slice - for _, er := range errs { - if !contains(c.wantWarn, er) { - t.Fatalf("Manifest errors are not as expected: \n\t(MISSING) %v\n\t(FROM) %v", er, c.wantWarn) + // check if the expected errors exist in actual errors slice + for _, er := range errs { + if !contains(c.wantWarn, er) { + t.Fatalf("manifest errors are not as expected: \n\t(MISSING) %v\n\t(FROM) %v", er, c.wantWarn) + } } - } + }) } } @@ -470,13 +498,13 @@ func TestValidateProjectRoots(t *testing.T) { stderrOutput.Reset() err := ValidateProjectRoots(ctx, &c.manifest, sm) if err != c.wantError { - t.Fatalf("Unexpected error while validating project roots:\n\t(GOT): %v\n\t(WNT): %v", 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 errors to contain: %q", warn) + t.Fatalf("expected ValidateProjectRoot errors to contain: %q", warn) } } }) From 13aa6fcff1ae94834aa7af3ff17753fbf8341171 Mon Sep 17 00:00:00 2001 From: Ibrahim AshShohail Date: Fri, 29 Sep 2017 11:49:51 +0300 Subject: [PATCH 2/5] dep: add prune options to manifests Signed-off-by: Ibrahim AshShohail --- Gopkg.toml | 5 ++ analyzer_test.go | 2 +- gps/prune.go | 5 +- manifest.go | 209 +++++++++++++++++++++++++++++++++++++++++------ manifest_test.go | 39 ++++++++- 5 files changed, 231 insertions(+), 29 deletions(-) diff --git a/Gopkg.toml b/Gopkg.toml index 054a3cf59a..2e181e69ab 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -30,3 +30,8 @@ [[constraint]] name = "github.com/golang/protobuf" branch = "master" + +[prune] + non-go = true + go-tests = true + unused-packages = true diff --git a/analyzer_test.go b/analyzer_test.go index 4e453620a4..40b7307422 100644 --- a/analyzer_test.go +++ b/analyzer_test.go @@ -38,7 +38,7 @@ func TestAnalyzerDeriveManifestAndLock(t *testing.T) { t.Fatal(err) } } else { - t.Fatalf("expected %s\n got %s", want, string(got)) + t.Fatalf("(WNT):\n%s\n(GOT):\n%s", want, string(got)) } } diff --git a/gps/prune.go b/gps/prune.go index 91a362d31f..013acc7d6a 100644 --- a/gps/prune.go +++ b/gps/prune.go @@ -16,9 +16,12 @@ import ( // PruneOptions represents the pruning options used to write the dependecy tree. type PruneOptions uint8 +// PruneProjectOptions is map of prune options per project name. +type PruneProjectOptions map[ProjectRoot]PruneOptions + const ( // PruneNestedVendorDirs indicates if nested vendor directories should be pruned. - PruneNestedVendorDirs = 1 << iota + PruneNestedVendorDirs PruneOptions = 1 << iota // PruneUnusedPackages indicates if unused Go packages should be pruned. PruneUnusedPackages // PruneNonGoFiles indicates if non-Go files should be pruned. diff --git a/manifest.go b/manifest.go index 5dd1348bc7..847f76ac75 100644 --- a/manifest.go +++ b/manifest.go @@ -24,26 +24,39 @@ 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") - errInvalidProjectRoot = errors.New("ProjectRoot name validation failed") + errInvalidConstraint = errors.Errorf("%q must be a TOML array of tables", "constraint") + errInvalidOverride = errors.Errorf("%q must be a TOML array of tables", "override") + errInvalidRequired = errors.Errorf("%q must be a TOML list of strings", "required") + errInvalidIgnored = errors.Errorf("%q must be a TOML list of strings", "ignored") + errInvalidPrune = errors.Errorf("%q must be a TOML table of booleans", "prune") + + errInvalidProjectRoot = errors.New("ProjectRoot name validation failed") + errInvalidPruneValue = errors.New("prune options values must be booleans") + errInvalidPruneProject = errors.Errorf("%q must be a TOML array of tables", "prune.project") + errPruneSubProject = errors.New("prune projects should not contain sub projects") + + errInvalidRootPruneValue = errors.New("root prune options must be omitted instead of being set to false") + errInvalidPruneProjectName = errors.Errorf("%q in %q must be a string", "name", "prune.project") ) // Manifest holds manifest file data and implements gps.RootManifest. type Manifest struct { Constraints gps.ProjectConstraints Ovr gps.ProjectConstraints - Ignored []string - Required []string + + Ignored []string + Required []string + + PruneOptions gps.PruneOptions + PruneProjectOptions gps.PruneProjectOptions } type rawManifest struct { - Constraints []rawProject `toml:"constraint,omitempty"` - Overrides []rawProject `toml:"override,omitempty"` - Ignored []string `toml:"ignored,omitempty"` - Required []string `toml:"required,omitempty"` + Constraints []rawProject `toml:"constraint,omitempty"` + Overrides []rawProject `toml:"override,omitempty"` + Ignored []string `toml:"ignored,omitempty"` + Required []string `toml:"required,omitempty"` + PruneOptions rawPruneOptions `toml:"prune,omitempty"` } type rawProject struct { @@ -54,11 +67,33 @@ type rawProject struct { Source string `toml:"source,omitempty"` } -// NewManifest instantiates a new manifest. +type rawPruneOptions struct { + UnusedPackages bool `toml:"unused-packages,omitempty"` + NonGoFiles bool `toml:"non-go,omitempty"` + GoTests bool `toml:"go-tests,omitempty"` + + Projects []rawPruneProjectOptions `toml:"project,omitempty"` +} + +type rawPruneProjectOptions struct { + Name string `toml:"name"` + UnusedPackages bool `toml:"unused-packages,omitempty"` + NonGoFiles bool `toml:"non-go,omitempty"` + GoTests bool `toml:"go-tests,omitempty"` +} + +const ( + pruneOptionUnusedPackages = "unused-packages" + pruneOptionGoTests = "go-tests" + pruneOptionNonGo = "non-go" +) + +// NewManifest instantites a new manifest. func NewManifest() *Manifest { return &Manifest{ - Constraints: make(gps.ProjectConstraints), - Ovr: make(gps.ProjectConstraints), + Constraints: make(gps.ProjectConstraints), + Ovr: make(gps.ProjectConstraints), + PruneOptions: gps.PruneNestedVendorDirs, } } @@ -151,6 +186,12 @@ func validateManifest(s string) ([]error, error) { return warns, errInvalidRequired } } + case "prune": + pruneWarns, err := validatePruneOptions(val, true) + warns = append(warns, pruneWarns...) + if err != nil { + return warns, err + } default: warns = append(warns, fmt.Errorf("unknown field in manifest: %v", prop)) } @@ -159,6 +200,70 @@ func validateManifest(s string) ([]error, error) { return warns, nil } +func validatePruneOptions(val interface{}, root bool) (warns []error, err error) { + if reflect.TypeOf(val).Kind() != reflect.Map { + return warns, errInvalidPrune + } + + for key, value := range val.(map[string]interface{}) { + switch key { + case pruneOptionNonGo, pruneOptionGoTests, pruneOptionUnusedPackages: + if option, ok := value.(bool); !ok { + return warns, errInvalidPruneValue + } else if root && !option { + return warns, errInvalidRootPruneValue + } + case "name": + if root { + warns = append(warns, errors.Errorf("%q should not include a name", "prune")) + } else if _, ok := value.(string); !ok { + return warns, errInvalidPruneProjectName + } + case "project": + if !root { + return warns, errPruneSubProject + } + if reflect.TypeOf(value).Kind() != reflect.Slice { + return warns, errInvalidPruneProject + } + for _, project := range value.([]interface{}) { + projectWarns, err := validatePruneOptions(project, false) + warns = append(warns, projectWarns...) + if err != nil { + return nil, err + } + } + + default: + if root { + warns = append(warns, errors.Errorf("unknown field %q in %q", key, "prune")) + } else { + warns = append(warns, errors.Errorf("unknown field %q in %q", key, "prune.project")) + } + } + } + + return warns, err +} + +func checkRedundantPruneOptions(raw rawManifest) (warns []error) { + rootOptions := raw.PruneOptions + + for _, project := range raw.PruneOptions.Projects { + if rootOptions.GoTests && project.GoTests { + warns = append(warns, errors.Errorf("redundant prune option %q set for %q", pruneOptionGoTests, project.Name)) + } + if rootOptions.NonGoFiles && project.NonGoFiles { + warns = append(warns, errors.Errorf("redundant prune option %q set for %q", pruneOptionNonGo, project.Name)) + } + if rootOptions.UnusedPackages && project.UnusedPackages { + warns = append(warns, errors.Errorf("redundant prune option %q set for %q", pruneOptionUnusedPackages, project.Name)) + } + } + + return warns +} + // ValidateProjectRoots validates the project roots present in manifest. func ValidateProjectRoots(c *Ctx, m *Manifest, sm gps.SourceManager) error { // Channel to receive all the errors @@ -184,6 +289,10 @@ func ValidateProjectRoots(c *Ctx, m *Manifest, sm gps.SourceManager) error { wg.Add(1) go validate(pr) } + for pr := range m.PruneProjectOptions { + wg.Add(1) + go validate(pr) + } wg.Wait() close(errorCh) @@ -220,6 +329,8 @@ func readManifest(r io.Reader) (*Manifest, []error, error) { return nil, warns, errors.Wrap(err, "unable to parse the manifest as TOML") } + warns = append(warns, checkRedundantPruneOptions(raw)...) + m, err := fromRawManifest(raw) return m, warns, err } @@ -254,9 +365,43 @@ func fromRawManifest(raw rawManifest) (*Manifest, error) { m.Ovr[name] = prj } + m.PruneOptions, m.PruneProjectOptions = fromRawPruneOptions(raw.PruneOptions) + return m, nil } +func fromRawPruneOptions(raw rawPruneOptions) (gps.PruneOptions, gps.PruneProjectOptions) { + rootOptions := gps.PruneNestedVendorDirs + pruneProjects := make(gps.PruneProjectOptions) + + if raw.UnusedPackages { + rootOptions |= gps.PruneUnusedPackages + } + if raw.GoTests { + rootOptions |= gps.PruneGoTestFiles + } + if raw.NonGoFiles { + rootOptions |= gps.PruneNonGoFiles + } + + for _, p := range raw.Projects { + pr := gps.ProjectRoot(p.Name) + pruneProjects[pr] = gps.PruneNestedVendorDirs + + if raw.UnusedPackages { + pruneProjects[pr] |= gps.PruneUnusedPackages + } + if raw.GoTests { + pruneProjects[pr] |= gps.PruneGoTestFiles + } + if raw.NonGoFiles { + pruneProjects[pr] |= gps.PruneNonGoFiles + } + } + + return rootOptions, pruneProjects +} + // toProject interprets the string representations of project information held in // a rawProject, converting them into a proper gps.ProjectProperties. An // error is returned if the rawProject contains some invalid combination - @@ -288,17 +433,27 @@ func toProject(raw rawProject) (n gps.ProjectRoot, pp gps.ProjectProperties, err } pp.Source = raw.Source + return n, pp, nil } +// MarshalTOML serializes this manifest into TOML via an intermediate raw form. +func (m *Manifest) MarshalTOML() ([]byte, error) { + raw := m.toRaw() + result, err := toml.Marshal(raw) + return result, errors.Wrap(err, "unable to marshal the lock to a TOML string") +} + // toRaw converts the manifest into a representation suitable to write to the manifest file func (m *Manifest) toRaw() rawManifest { raw := rawManifest{ - Constraints: make([]rawProject, 0, len(m.Constraints)), - Overrides: make([]rawProject, 0, len(m.Ovr)), - Ignored: m.Ignored, - Required: m.Required, + Constraints: make([]rawProject, 0, len(m.Constraints)), + Overrides: make([]rawProject, 0, len(m.Ovr)), + Ignored: m.Ignored, + Required: m.Required, + PruneOptions: rawPruneOptions{}, } + for n, prj := range m.Constraints { raw.Constraints = append(raw.Constraints, toRawProject(n, prj)) } @@ -309,6 +464,8 @@ func (m *Manifest) toRaw() rawManifest { } sort.Sort(sortedRawProjects(raw.Overrides)) + // TODO(ibrasho): write out prune options. + return raw } @@ -329,13 +486,6 @@ func (s sortedRawProjects) Less(i, j int) bool { return l.Source < r.Source } -// MarshalTOML serializes this manifest into TOML via an intermediate raw form. -func (m *Manifest) MarshalTOML() ([]byte, error) { - raw := m.toRaw() - result, err := toml.Marshal(raw) - return result, errors.Wrap(err, "Unable to marshal the lock to a TOML string") -} - func toRawProject(name gps.ProjectRoot, project gps.ProjectProperties) rawProject { raw := rawProject{ Name: string(name), @@ -363,6 +513,7 @@ func toRawProject(name gps.ProjectRoot, project gps.ProjectProperties) rawProjec // Has to be a semver range. raw.Version = project.Constraint.ImpliedCaretString() } + return raw } @@ -407,3 +558,11 @@ func (m *Manifest) RequiredPackages() map[string]bool { return mp } + +func (m *Manifest) PruneOptionsFor(pr gps.ProjectRoot) gps.PruneOptions { + if po, ok := m.PruneProjectOptions[pr]; ok { + return po + } + + return m.PruneOptions +} diff --git a/manifest_test.go b/manifest_test.go index 7076a3f255..a8aed3a279 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -44,7 +44,11 @@ func TestReadManifest(t *testing.T) { Constraint: gps.NewBranch("master"), }, }, - Ignored: []string{"github.com/foo/bar"}, + Ignored: []string{"github.com/foo/bar"}, + PruneOptions: gps.PruneNestedVendorDirs | gps.PruneNonGoFiles, + PruneProjectOptions: gps.PruneProjectOptions{ + gps.ProjectRoot("github.com/golang/dep"): gps.PruneNestedVendorDirs, + }, } if !reflect.DeepEqual(got.Constraints, want.Constraints) { @@ -78,6 +82,11 @@ func TestWriteManifest(t *testing.T) { } m.Ignored = []string{"github.com/foo/bar"} + m.PruneOptions = gps.PruneNestedVendorDirs | gps.PruneNonGoFiles + m.PruneProjectOptions = gps.PruneProjectOptions{ + gps.ProjectRoot("github.com/golang/dep"): gps.PruneNestedVendorDirs, + } + got, err := m.MarshalTOML() if err != nil { t.Fatalf("error while marshaling valid manifest to TOML: %q", err) @@ -89,7 +98,7 @@ func TestWriteManifest(t *testing.T) { t.Fatal(err) } } else { - t.Errorf("valid manifest did not marshal to TOML as expected:\n\t(GOT): %s\n\t(WNT): %s", string(got), want) + t.Errorf("valid manifest did not marshal to TOML as expected:\n(GOT):\n%s\n(WNT):\n%s", string(got), want) } } } @@ -368,6 +377,32 @@ func TestValidateManifest(t *testing.T) { wantWarn: []error{errors.New("revision \"8d43f8c0b836\" should not be in abbreviated form")}, wantError: nil, }, + { + name: "valid prune options", + tomlString: ` + [[constraint]] + name = "github.com/foo/bar" + version = "1.0.0" + + [prune] + non-go = true + `, + wantWarn: []error{}, + wantError: nil, + }, + { + name: "invalid root prune options", + tomlString: ` + [[constraint]] + name = "github.com/foo/bar" + version = "1.0.0" + + [prune] + non-go = false + `, + wantWarn: []error{}, + wantError: errInvalidRootPruneValue, + }, } // contains for error From 7ed904963661f3c631802c90594ebcf3f41de58a Mon Sep 17 00:00:00 2001 From: Ibrahim AshShohail Date: Sat, 28 Oct 2017 02:52:33 +0300 Subject: [PATCH 3/5] dep: update tests for adding prune to manifest Signed-off-by: Ibrahim AshShohail --- gps/prune.go | 2 - manifest.go | 5 +++ manifest_test.go | 110 ++++++++++++++++++++++++++++++++++++----------- 3 files changed, 90 insertions(+), 27 deletions(-) diff --git a/gps/prune.go b/gps/prune.go index 013acc7d6a..84ac74eb6c 100644 --- a/gps/prune.go +++ b/gps/prune.go @@ -140,7 +140,6 @@ func pruneUnusedPackages(lp LockedProject, projectDir string, logger *log.Logger // calculateUnusedPackages generates a list of unused packages in lp. func calculateUnusedPackages(lp LockedProject, projectDir string) (map[string]struct{}, error) { - // TODO(ibrasho): optimize this... unused := make(map[string]struct{}) imported := make(map[string]struct{}) for _, pkg := range lp.Packages() { @@ -175,7 +174,6 @@ func calculateUnusedPackages(lp LockedProject, projectDir string) (map[string]st // collectUnusedPackagesFiles returns a slice of all files in the unused packages in projectDir. func collectUnusedPackagesFiles(projectDir string, unusedPackages map[string]struct{}) ([]string, error) { - // TODO(ibrasho): is this useful? files := make([]string, 0, len(unusedPackages)) err := filepath.Walk(projectDir, func(path string, info os.FileInfo, err error) error { diff --git a/manifest.go b/manifest.go index 847f76ac75..c5e3f419cf 100644 --- a/manifest.go +++ b/manifest.go @@ -226,6 +226,7 @@ func validatePruneOptions(val interface{}, root bool) (warns []error, err error) if reflect.TypeOf(value).Kind() != reflect.Slice { return warns, errInvalidPruneProject } + for _, project := range value.([]interface{}) { projectWarns, err := validatePruneOptions(project, false) warns = append(warns, projectWarns...) @@ -559,6 +560,10 @@ func (m *Manifest) RequiredPackages() map[string]bool { return mp } +// PruneOptionsFor returns the prune options for the passed project root. +// +// It will return the root prune options if the project does not have specific +// options or if it does not exists in the manifest. func (m *Manifest) PruneOptionsFor(pr gps.ProjectRoot) gps.PruneOptions { if po, ok := m.PruneProjectOptions[pr]; ok { return po diff --git a/manifest_test.go b/manifest_test.go index a8aed3a279..531ae85e05 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -7,6 +7,7 @@ package dep import ( "bytes" "errors" + "fmt" "io/ioutil" "log" "reflect" @@ -47,7 +48,8 @@ func TestReadManifest(t *testing.T) { Ignored: []string{"github.com/foo/bar"}, PruneOptions: gps.PruneNestedVendorDirs | gps.PruneNonGoFiles, PruneProjectOptions: gps.PruneProjectOptions{ - gps.ProjectRoot("github.com/golang/dep"): gps.PruneNestedVendorDirs, + gps.ProjectRoot("github.com/golang/dep"): gps.PruneNestedVendorDirs, + gps.ProjectRoot("github.com/babble/brook"): gps.PruneNestedVendorDirs | gps.PruneGoTestFiles, }, } @@ -82,11 +84,6 @@ func TestWriteManifest(t *testing.T) { } m.Ignored = []string{"github.com/foo/bar"} - m.PruneOptions = gps.PruneNestedVendorDirs | gps.PruneNonGoFiles - m.PruneProjectOptions = gps.PruneProjectOptions{ - gps.ProjectRoot("github.com/golang/dep"): gps.PruneNestedVendorDirs, - } - got, err := m.MarshalTOML() if err != nil { t.Fatalf("error while marshaling valid manifest to TOML: %q", err) @@ -364,7 +361,9 @@ func TestValidateManifest(t *testing.T) { name = "github.com/foo/bar" revision = "b86ad16" `, - wantWarn: []error{errors.New("revision \"b86ad16\" should not be in abbreviated form")}, + wantWarn: []error{ + errors.New("revision \"b86ad16\" should not be in abbreviated form"), + }, wantError: nil, }, { @@ -380,10 +379,6 @@ func TestValidateManifest(t *testing.T) { { name: "valid prune options", tomlString: ` - [[constraint]] - name = "github.com/foo/bar" - version = "1.0.0" - [prune] non-go = true `, @@ -393,26 +388,37 @@ func TestValidateManifest(t *testing.T) { { name: "invalid root prune options", tomlString: ` - [[constraint]] - name = "github.com/foo/bar" - version = "1.0.0" - [prune] non-go = false `, wantWarn: []error{}, wantError: errInvalidRootPruneValue, }, - } + { + name: "root options should not contain a name", + tomlString: ` + [prune] + go-tests = true + name = "github.com/golang/dep" + `, + wantWarn: []error{ + fmt.Errorf("%q should not include a name", "prune"), + }, + wantError: nil, + }, + { + name: "invalid prune project", + tomlString: ` + [prune] + non-go = true - // contains for error - contains := func(s []error, e error) bool { - for _, a := range s { - if a.Error() == e.Error() { - return true - } - } - return false + [prune.project] + name = "github.com/org/project" + non-go = true + `, + wantWarn: []error{}, + wantError: errInvalidPruneProject, + }, } for _, c := range cases { @@ -431,7 +437,7 @@ func TestValidateManifest(t *testing.T) { // check if the expected errors exist in actual errors slice for _, er := range errs { - if !contains(c.wantWarn, er) { + if !containsErr(c.wantWarn, er) { t.Fatalf("manifest errors are not as expected: \n\t(MISSING) %v\n\t(FROM) %v", er, c.wantWarn) } } @@ -439,6 +445,47 @@ func TestValidateManifest(t *testing.T) { } } +func TestCheckRedundantPruneOptions(t *testing.T) { + cases := []struct { + name string + pruneOptions rawPruneOptions + wantWarn []error + }{ + { + name: "redundant project prune options", + pruneOptions: rawPruneOptions{ + NonGoFiles: true, + Projects: []rawPruneProjectOptions{ + rawPruneProjectOptions{ + Name: "github.com/org/project", + NonGoFiles: true, + }, + }, + }, + wantWarn: []error{ + fmt.Errorf("redundant prune option %q set for %q", "non-go", "github.com/org/project"), + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + errs := checkRedundantPruneOptions(rawManifest{PruneOptions: c.pruneOptions}) + + // compare length of error slice + if len(errs) != len(c.wantWarn) { + t.Fatalf("number of manifest errors are not as expected:\n\t(GOT) %v errors(%v)\n\t(WNT) %v errors(%v).", len(errs), errs, len(c.wantWarn), c.wantWarn) + } + + for _, er := range errs { + if !containsErr(c.wantWarn, er) { + t.Fatalf("manifest errors are not as expected:\n\t(MISSING)\n%v\n\t(FROM)\n%v", er, c.wantWarn) + } + } + }) + } +} + func TestValidateProjectRoots(t *testing.T) { cases := []struct { name string @@ -545,3 +592,16 @@ func TestValidateProjectRoots(t *testing.T) { }) } } + +func TestPruneOptionsFor(t *testing.T) { + +} + +func containsErr(s []error, e error) bool { + for _, a := range s { + if a.Error() == e.Error() { + return true + } + } + return false +} From 09e6f3bb8dec3310d426cf5576620eefecb851e2 Mon Sep 17 00:00:00 2001 From: Ibrahim AshShohail Date: Fri, 17 Nov 2017 10:24:58 +0300 Subject: [PATCH 4/5] dep: convert some errors to a variable Signed-off-by: Ibrahim AshShohail --- manifest.go | 25 ++++++++++++++----------- manifest_test.go | 6 ++++-- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/manifest.go b/manifest.go index c5e3f419cf..6115a5f482 100644 --- a/manifest.go +++ b/manifest.go @@ -24,17 +24,20 @@ const ManifestName = "Gopkg.toml" // Errors var ( - errInvalidConstraint = errors.Errorf("%q must be a TOML array of tables", "constraint") - errInvalidOverride = errors.Errorf("%q must be a TOML array of tables", "override") - errInvalidRequired = errors.Errorf("%q must be a TOML list of strings", "required") - errInvalidIgnored = errors.Errorf("%q must be a TOML list of strings", "ignored") - errInvalidPrune = errors.Errorf("%q must be a TOML table of booleans", "prune") - - errInvalidProjectRoot = errors.New("ProjectRoot name validation failed") - errInvalidPruneValue = errors.New("prune options values must be booleans") + errInvalidConstraint = errors.Errorf("%q must be a TOML array of tables", "constraint") + errInvalidOverride = errors.Errorf("%q must be a TOML array of tables", "override") + errInvalidRequired = errors.Errorf("%q must be a TOML list of strings", "required") + errInvalidIgnored = errors.Errorf("%q must be a TOML list of strings", "ignored") + errInvalidPrune = errors.Errorf("%q must be a TOML table of booleans", "prune") errInvalidPruneProject = errors.Errorf("%q must be a TOML array of tables", "prune.project") - errPruneSubProject = errors.New("prune projects should not contain sub projects") + errInvalidMetadata = errors.New("metadata should be a TOML table") + errInvalidProjectRoot = errors.New("ProjectRoot name validation failed") + + errInvalidPruneValue = errors.New("prune options values must be booleans") + errPruneSubProject = errors.New("prune projects should not contain sub projects") + + errRootPruneContainsName = errors.Errorf("%q should not include a name", "prune") errInvalidRootPruneValue = errors.New("root prune options must be omitted instead of being set to false") errInvalidPruneProjectName = errors.Errorf("%q in %q must be a string", "name", "prune.project") ) @@ -115,7 +118,7 @@ func validateManifest(s string) ([]error, error) { case "metadata": // Check if metadata is of Map type if reflect.TypeOf(val).Kind() != reflect.Map { - warns = append(warns, errors.New("metadata should be a TOML table")) + warns = append(warns, errInvalidMetadata) } case "constraint", "override": valid := true @@ -215,7 +218,7 @@ func validatePruneOptions(val interface{}, root bool) (warns []error, err error) } case "name": if root { - warns = append(warns, errors.Errorf("%q should not include a name", "prune")) + warns = append(warns, errRootPruneContainsName) } else if _, ok := value.(string); !ok { return warns, errInvalidPruneProjectName } diff --git a/manifest_test.go b/manifest_test.go index 531ae85e05..93eb95ab83 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -253,7 +253,9 @@ func TestValidateManifest(t *testing.T) { [[constraint]] name = "github.com/foo/bar" `, - wantWarn: []error{errors.New("metadata should be a TOML table")}, + wantWarn: []error{ + errInvalidMetadata, + }, wantError: nil, }, { @@ -402,7 +404,7 @@ func TestValidateManifest(t *testing.T) { name = "github.com/golang/dep" `, wantWarn: []error{ - fmt.Errorf("%q should not include a name", "prune"), + errRootPruneContainsName, }, wantError: nil, }, From b439b8dbb8783c33f6dc98e433a579bb535e802c Mon Sep 17 00:00:00 2001 From: Ibrahim AshShohail Date: Fri, 17 Nov 2017 10:48:01 +0300 Subject: [PATCH 5/5] ignore (*Manifest).PruneOptionsFor is unused error Signed-off-by: Ibrahim AshShohail --- hack/lint.bash | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hack/lint.bash b/hack/lint.bash index 7dbf5bd8d0..c474d6b7cc 100755 --- a/hack/lint.bash +++ b/hack/lint.bash @@ -9,4 +9,4 @@ set -e PKGS=$(go list ./... | grep -vF /vendor/) go vet $PKGS golint $PKGS -megacheck -unused.exported -ignore "github.com/golang/dep/internal/test/test.go:U1000 github.com/golang/dep/gps/prune.go:U1000" $PKGS +megacheck -unused.exported -ignore "github.com/golang/dep/internal/test/test.go:U1000 github.com/golang/dep/gps/prune.go:U1000 github.com/golang/dep/manifest.go:U1000" $PKGS