From 6cec5a8873c513c9ed1adf9dd6bc24e723e82ef5 Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 7 Sep 2017 16:07:18 +0530 Subject: [PATCH 1/8] fix(status): concurrent BasicStatus creation This change adds concurrent BasicStatus creation, maintaining the order of status output. --- cmd/dep/status.go | 158 +++++++++++++++++++++++++++++----------------- 1 file changed, 99 insertions(+), 59 deletions(-) diff --git a/cmd/dep/status.go b/cmd/dep/status.go index a849b15cb3..f209e28f4b 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -13,6 +13,7 @@ import ( "io/ioutil" "log" "sort" + "sync" "text/tabwriter" "github.com/golang/dep" @@ -366,82 +367,121 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana logger.Println("Checking upstream projects:") + // BasicStatus channel to collect all the BasicStatus + bsCh := make(chan *BasicStatus, len(slp)) + + // Error channel to collect all the errors + errorCh := make(chan error, len(slp)) + + var wg sync.WaitGroup + for i, proj := range slp { + wg.Add(1) logger.Printf("(%d/%d) %s\n", i+1, len(slp), proj.Ident().ProjectRoot) - bs := BasicStatus{ - ProjectRoot: string(proj.Ident().ProjectRoot), - PackageCount: len(proj.Packages()), - } + go func(proj gps.LockedProject) { + defer wg.Done() - // Get children only for specific outputers - // in order to avoid slower status process - switch out.(type) { - case *dotOutput: - ptr, err := sm.ListPackages(proj.Ident(), proj.Version()) - - if err != nil { - return digestMismatch, hasMissingPkgs, errors.Wrapf(err, "analysis of %s package failed", proj.Ident().ProjectRoot) + bs := BasicStatus{ + ProjectRoot: string(proj.Ident().ProjectRoot), + PackageCount: len(proj.Packages()), } - prm, _ := ptr.ToReachMap(true, false, false, nil) - bs.Children = prm.FlattenFn(paths.IsStandardImportPath) - } + // Get children only for specific outputers + // in order to avoid slower status process + switch out.(type) { + case *dotOutput: + ptr, err := sm.ListPackages(proj.Ident(), proj.Version()) - // Split apart the version from the lock into its constituent parts - switch tv := proj.Version().(type) { - case gps.UnpairedVersion: - bs.Version = tv - case gps.Revision: - bs.Revision = tv - case gps.PairedVersion: - bs.Version = tv.Unpair() - bs.Revision = tv.Revision() - } + if err != nil { + errorCh <- err + } - // Check if the manifest has an override for this project. If so, - // set that as the constraint. - if pp, has := p.Manifest.Ovr[proj.Ident().ProjectRoot]; has && pp.Constraint != nil { - bs.hasOverride = true - bs.Constraint = pp.Constraint - } else { - bs.Constraint = gps.Any() - for _, c := range cm[bs.ProjectRoot] { - bs.Constraint = c.Intersect(bs.Constraint) + prm, _ := ptr.ToReachMap(true, false, false, nil) + bs.Children = prm.FlattenFn(paths.IsStandardImportPath) + } + + // Split apart the version from the lock into its constituent parts + switch tv := proj.Version().(type) { + case gps.UnpairedVersion: + bs.Version = tv + case gps.Revision: + bs.Revision = tv + case gps.PairedVersion: + bs.Version = tv.Unpair() + bs.Revision = tv.Revision() } - } - // Only if we have a non-rev and non-plain version do/can we display - // anything wrt the version's updateability. - if bs.Version != nil && bs.Version.Type() != gps.IsVersion { - c, has := p.Manifest.Constraints[proj.Ident().ProjectRoot] - if !has { - c.Constraint = gps.Any() + // Check if the manifest has an override for this project. If so, + // set that as the constraint. + if pp, has := p.Manifest.Ovr[proj.Ident().ProjectRoot]; has && pp.Constraint != nil { + bs.hasOverride = true + bs.Constraint = pp.Constraint + } else { + bs.Constraint = gps.Any() + for _, c := range cm[bs.ProjectRoot] { + bs.Constraint = c.Intersect(bs.Constraint) + } } - // TODO: This constraint is only the constraint imposed by the - // current project, not by any transitive deps. As a result, - // transitive project deps will always show "any" here. - bs.Constraint = c.Constraint - - vl, err := sm.ListVersions(proj.Ident()) - if err == nil { - gps.SortPairedForUpgrade(vl) - - for _, v := range vl { - // Because we've sorted the version list for - // upgrade, the first version we encounter that - // matches our constraint will be what we want. - if c.Constraint.Matches(v) { - bs.Latest = v.Revision() - break + + // Only if we have a non-rev and non-plain version do/can we display + // anything wrt the version's updateability. + if bs.Version != nil && bs.Version.Type() != gps.IsVersion { + c, has := p.Manifest.Constraints[proj.Ident().ProjectRoot] + if !has { + c.Constraint = gps.Any() + } + // TODO: This constraint is only the constraint imposed by the + // current project, not by any transitive deps. As a result, + // transitive project deps will always show "any" here. + bs.Constraint = c.Constraint + + vl, err := sm.ListVersions(proj.Ident()) + if err == nil { + gps.SortPairedForUpgrade(vl) + + for _, v := range vl { + // Because we've sorted the version list for + // upgrade, the first version we encounter that + // matches our constraint will be what we want. + if c.Constraint.Matches(v) { + bs.Latest = v.Revision() + break + } } } } - } - out.BasicLine(&bs) + bsCh <- &bs + + }(proj) } + + wg.Wait() + close(bsCh) + close(errorCh) logger.Println() + + if len(errorCh) > 0 { + for err := range errorCh { + ctx.Err.Println(err.Error()) + } + ctx.Err.Println() + } + + // A map of ProjectRoot and *BasicStatus. This is used in maintain the + // order of BasicStatus in output by collecting all the BasicStatus and + // then using them in order. + bsMap := make(map[string]*BasicStatus) + for bs := range bsCh { + bsMap[bs.ProjectRoot] = bs + } + + // Use the collected BasicStatus in outputter + for _, proj := range slp { + out.BasicLine(bsMap[string(proj.Ident().ProjectRoot)]) + } + out.BasicFooter() return digestMismatch, hasMissingPkgs, nil From 9de9063a0f22b583ed85a7590366d640b9af6fdb Mon Sep 17 00:00:00 2001 From: Sunny Date: Sat, 9 Sep 2017 20:17:33 +0530 Subject: [PATCH 2/8] status: log errors and failure feedback --- cmd/dep/status.go | 54 +++++++++++++++++++++++++++++++++++------- cmd/dep/status_test.go | 10 ++++++++ 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/cmd/dep/status.go b/cmd/dep/status.go index f209e28f4b..1210201aee 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -43,6 +43,8 @@ print an extended status output for each dependency of the project. Status returns exit code zero if all dependencies are in a "good state". ` +var errFailedUpdate = errors.New("failed to fetch updates") + func (cmd *statusCommand) Name() string { return "status" } func (cmd *statusCommand) Args() string { return "[package...]" } func (cmd *statusCommand) ShortHelp() string { return statusShortHelp } @@ -98,7 +100,7 @@ func (out *tableOutput) BasicLine(bs *BasicStatus) { bs.getConsolidatedConstraint(), formatVersion(bs.Version), formatVersion(bs.Revision), - formatVersion(bs.Latest), + bs.getConsolidatedLatest(), bs.PackageCount, ) } @@ -226,6 +228,15 @@ func (cmd *statusCommand) Run(ctx *dep.Ctx, args []string) error { digestMismatch, hasMissingPkgs, err := runStatusAll(ctx, out, p, sm) if err != nil { + // Print the outdated results + if err == errFailedUpdate { + ctx.Out.Println(buf.String()) + + // Print the help when in non-verbose mode + if !ctx.Verbose { + ctx.Out.Println("Failed to get status of some projects. Run `dep status -v` to see the error messages.") + } + } return err } @@ -249,8 +260,8 @@ type rawStatus struct { ProjectRoot string Constraint string Version string - Revision gps.Revision - Latest gps.Version + Revision string + Latest string PackageCount int } @@ -265,6 +276,7 @@ type BasicStatus struct { Latest gps.Version PackageCount int hasOverride bool + hasError bool } func (bs *BasicStatus) getConsolidatedConstraint() string { @@ -292,13 +304,26 @@ func (bs *BasicStatus) getConsolidatedVersion() string { return version } +func (bs *BasicStatus) getConsolidatedLatest() string { + latest := "" + if bs.Latest != nil { + latest = formatVersion(bs.Latest) + } + + if bs.hasError { + latest += "unknown" + } + + return latest +} + func (bs *BasicStatus) marshalJSON() *rawStatus { return &rawStatus{ ProjectRoot: bs.ProjectRoot, Constraint: bs.getConsolidatedConstraint(), Version: formatVersion(bs.Version), - Revision: bs.Revision, - Latest: bs.Latest, + Revision: formatVersion(bs.Revision), + Latest: bs.getConsolidatedLatest(), PackageCount: bs.PackageCount, } } @@ -394,6 +419,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana ptr, err := sm.ListPackages(proj.Ident(), proj.Version()) if err != nil { + bs.hasError = true errorCh <- err } @@ -449,6 +475,11 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana break } } + } else { + // Failed to fetch version list (could happen due to + // network issue) + bs.hasError = true + errorCh <- err } } @@ -462,11 +493,16 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana close(errorCh) logger.Println() + var updateError error + if len(errorCh) > 0 { - for err := range errorCh { - ctx.Err.Println(err.Error()) + updateError = errFailedUpdate + if ctx.Verbose { + for err := range errorCh { + ctx.Err.Println(err.Error()) + } + ctx.Err.Println() } - ctx.Err.Println() } // A map of ProjectRoot and *BasicStatus. This is used in maintain the @@ -484,7 +520,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana out.BasicFooter() - return digestMismatch, hasMissingPkgs, nil + return digestMismatch, hasMissingPkgs, updateError } // Hash digest mismatch may indicate that some deps are no longer diff --git a/cmd/dep/status_test.go b/cmd/dep/status_test.go index 35ef53e007..e0fe4ed615 100644 --- a/cmd/dep/status_test.go +++ b/cmd/dep/status_test.go @@ -85,6 +85,16 @@ func TestBasicLine(t *testing.T) { wantJSONStatus: []string{`"Revision":"revxyz"`, `"Constraint":"1.2.3"`, `"Version":"1.0.0"`}, wantTableStatus: []string{`github.com/foo/bar 1.2.3 1.0.0 revxyz 0`}, }, + { + name: "BasicStatus with update error", + status: BasicStatus{ + ProjectRoot: "github.com/foo/bar", + hasError: true, + }, + wantDotStatus: []string{`[label="github.com/foo/bar"];`}, + wantJSONStatus: []string{`"Version":""`, `"Revision":""`, `"Latest":"unknown"`}, + wantTableStatus: []string{`github.com/foo/bar unknown 0`}, + }, } for _, test := range tests { From 152a82a2b0ca27efe0aa06714d27557a1b07f666 Mon Sep 17 00:00:00 2001 From: Sunny Date: Sun, 10 Sep 2017 20:52:07 +0530 Subject: [PATCH 3/8] status: add failure count for update failures --- cmd/dep/status.go | 85 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 24 deletions(-) diff --git a/cmd/dep/status.go b/cmd/dep/status.go index 1210201aee..b3863fc485 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -43,7 +43,11 @@ print an extended status output for each dependency of the project. Status returns exit code zero if all dependencies are in a "good state". ` -var errFailedUpdate = errors.New("failed to fetch updates") +var ( + errFailedUpdate = errors.New("failed to fetch updates") + errFailedListPkg = errors.New("failed to list packages") + errMultipleFailures = errors.New("multiple sources of failure") +) func (cmd *statusCommand) Name() string { return "status" } func (cmd *statusCommand) Args() string { return "[package...]" } @@ -226,18 +230,22 @@ func (cmd *statusCommand) Run(ctx *dep.Ctx, args []string) error { } } - digestMismatch, hasMissingPkgs, err := runStatusAll(ctx, out, p, sm) - if err != nil { - // Print the outdated results - if err == errFailedUpdate { + digestMismatch, hasMissingPkgs, errStatus := runStatusAll(ctx, out, p, sm) + if errStatus.err != nil { + // If it's only update errors + if errStatus.err == errFailedUpdate { + // Print the results with unknown data ctx.Out.Println(buf.String()) // Print the help when in non-verbose mode if !ctx.Verbose { - ctx.Out.Println("Failed to get status of some projects. Run `dep status -v` to see the error messages.") + ctx.Out.Printf("The status of %d projects are unknown due to errors. Rerun with `-v` flag to see details.\n", errStatus.count) } + } else { + // List package failure or multiple failures + ctx.Out.Println("Failed to get status. Rerun with `-v` flag to see details.") } - return err + return errStatus.err } if digestMismatch { @@ -334,18 +342,24 @@ type MissingStatus struct { MissingPackages []string } -func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (bool, bool, error) { +// errorStatus contains information about error and number of status failures. +type errorStatus struct { + err error + count int +} + +func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (bool, bool, errorStatus) { var digestMismatch, hasMissingPkgs bool if p.Lock == nil { - return digestMismatch, hasMissingPkgs, errors.Errorf("no Gopkg.lock found. Run `dep ensure` to generate lock file") + return digestMismatch, hasMissingPkgs, errorStatus{err: errors.Errorf("no Gopkg.lock found. Run `dep ensure` to generate lock file")} } // While the network churns on ListVersions() requests, statically analyze // code from the current project. ptree, err := pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot)) if err != nil { - return digestMismatch, hasMissingPkgs, errors.Wrapf(err, "analysis of local packages failed") + return digestMismatch, hasMissingPkgs, errorStatus{err: errors.Wrapf(err, "analysis of local packages failed")} } // Set up a solver in order to check the InputHash. @@ -365,12 +379,12 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana } if err := ctx.ValidateParams(sm, params); err != nil { - return digestMismatch, hasMissingPkgs, err + return digestMismatch, hasMissingPkgs, errorStatus{err: err} } s, err := gps.Prepare(params, sm) if err != nil { - return digestMismatch, hasMissingPkgs, errors.Wrapf(err, "could not set up solver for input hashing") + return digestMismatch, hasMissingPkgs, errorStatus{err: errors.Wrapf(err, "could not set up solver for input hashing")} } cm := collectConstraints(ptree, p, sm) @@ -395,8 +409,9 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana // BasicStatus channel to collect all the BasicStatus bsCh := make(chan *BasicStatus, len(slp)) - // Error channel to collect all the errors - errorCh := make(chan error, len(slp)) + // Error channels to collect different errors + errListPkgCh := make(chan error, len(slp)) + errListVerCh := make(chan error, len(slp)) var wg sync.WaitGroup @@ -420,7 +435,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana if err != nil { bs.hasError = true - errorCh <- err + errListPkgCh <- err } prm, _ := ptr.ToReachMap(true, false, false, nil) @@ -479,7 +494,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana // Failed to fetch version list (could happen due to // network issue) bs.hasError = true - errorCh <- err + errListVerCh <- err } } @@ -490,15 +505,37 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana wg.Wait() close(bsCh) - close(errorCh) + close(errListPkgCh) + close(errListVerCh) + + // Newline after printing the status progress output logger.Println() - var updateError error + var statusError error + var errorCount int + + // List Packages errors. This would happen only for dot output. + if len(errListPkgCh) > 0 { + statusError = errFailedListPkg + if ctx.Verbose { + for err := range errListPkgCh { + ctx.Err.Println(err.Error()) + } + ctx.Err.Println() + } + } + + // List Version errors + if len(errListVerCh) > 0 { + if statusError == errFailedListPkg { + statusError = errMultipleFailures + } else { + statusError = errFailedUpdate + } - if len(errorCh) > 0 { - updateError = errFailedUpdate + errorCount = len(errListVerCh) if ctx.Verbose { - for err := range errorCh { + for err := range errListVerCh { ctx.Err.Println(err.Error()) } ctx.Err.Println() @@ -520,7 +557,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana out.BasicFooter() - return digestMismatch, hasMissingPkgs, updateError + return digestMismatch, hasMissingPkgs, errorStatus{err: statusError, count: errorCount} } // Hash digest mismatch may indicate that some deps are no longer @@ -562,7 +599,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana ctx.Err.Printf("\t%s: %s\n", fail.ex, fail.err.Error()) } - return digestMismatch, hasMissingPkgs, errors.New("address issues with undeducible import paths to get more status information") + return digestMismatch, hasMissingPkgs, errorStatus{err: errors.New("address issues with undeducible import paths to get more status information")} } out.MissingHeader() @@ -582,7 +619,7 @@ outer: } out.MissingFooter() - return digestMismatch, hasMissingPkgs, nil + return digestMismatch, hasMissingPkgs, errorStatus{} } func formatVersion(v gps.Version) string { From cf9a971b81a8a0cabed54f6086cebccb79bc0bbf Mon Sep 17 00:00:00 2001 From: Sunny Date: Sun, 10 Sep 2017 21:10:17 +0530 Subject: [PATCH 4/8] fix(status): add shortRev and longRev for latest Adds options to specify shortRev or longRev to getConsolidatedLatest(). --- cmd/dep/status.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/cmd/dep/status.go b/cmd/dep/status.go index b3863fc485..3917c61b3e 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -43,6 +43,11 @@ print an extended status output for each dependency of the project. Status returns exit code zero if all dependencies are in a "good state". ` +const ( + shortRev = iota + longRev = iota +) + var ( errFailedUpdate = errors.New("failed to fetch updates") errFailedListPkg = errors.New("failed to list packages") @@ -104,7 +109,7 @@ func (out *tableOutput) BasicLine(bs *BasicStatus) { bs.getConsolidatedConstraint(), formatVersion(bs.Version), formatVersion(bs.Revision), - bs.getConsolidatedLatest(), + bs.getConsolidatedLatest(shortRev), bs.PackageCount, ) } @@ -312,10 +317,15 @@ func (bs *BasicStatus) getConsolidatedVersion() string { return version } -func (bs *BasicStatus) getConsolidatedLatest() string { +func (bs *BasicStatus) getConsolidatedLatest(revSize int) string { latest := "" if bs.Latest != nil { - latest = formatVersion(bs.Latest) + switch revSize { + case shortRev: + latest = formatVersion(bs.Latest) + case longRev: + latest = bs.Latest.String() + } } if bs.hasError { @@ -330,8 +340,8 @@ func (bs *BasicStatus) marshalJSON() *rawStatus { ProjectRoot: bs.ProjectRoot, Constraint: bs.getConsolidatedConstraint(), Version: formatVersion(bs.Version), - Revision: formatVersion(bs.Revision), - Latest: bs.getConsolidatedLatest(), + Revision: string(bs.Revision), + Latest: bs.getConsolidatedLatest(longRev), PackageCount: bs.PackageCount, } } From d1f2cdc0df2134fa9df84710b090047e5bbc2859 Mon Sep 17 00:00:00 2001 From: Sunny Date: Sun, 10 Sep 2017 22:03:37 +0530 Subject: [PATCH 5/8] status: add test for getConsolidatedLatest() --- cmd/dep/status_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/cmd/dep/status_test.go b/cmd/dep/status_test.go index e0fe4ed615..214d27390c 100644 --- a/cmd/dep/status_test.go +++ b/cmd/dep/status_test.go @@ -230,3 +230,60 @@ func TestBasicStatusGetConsolidatedVersion(t *testing.T) { }) } } + +func TestBasicStatusGetConsolidatedLatest(t *testing.T) { + testCases := []struct { + name string + basicStatus BasicStatus + revSize int + wantLatest string + }{ + { + name: "empty BasicStatus", + basicStatus: BasicStatus{}, + revSize: shortRev, + wantLatest: "", + }, + { + name: "nil latest", + basicStatus: BasicStatus{ + Latest: nil, + }, + revSize: shortRev, + wantLatest: "", + }, + { + name: "with error", + basicStatus: BasicStatus{ + hasError: true, + }, + revSize: shortRev, + wantLatest: "unknown", + }, + { + name: "short latest", + basicStatus: BasicStatus{ + Latest: gps.Revision("adummylonglongrevision"), + }, + revSize: shortRev, + wantLatest: "adummyl", + }, + { + name: "long latest", + basicStatus: BasicStatus{ + Latest: gps.Revision("adummylonglongrevision"), + }, + revSize: longRev, + wantLatest: "adummylonglongrevision", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gotRev := tc.basicStatus.getConsolidatedLatest(tc.revSize) + if gotRev != tc.wantLatest { + t.Errorf("unexpected consolidated latest: \n\t(GOT) %v \n\t(WNT) %v", gotRev, tc.wantLatest) + } + }) + } +} From bce09f4cbf6b1b84a102484936691905a3107f49 Mon Sep 17 00:00:00 2001 From: Sunny Date: Mon, 11 Sep 2017 15:31:48 +0530 Subject: [PATCH 6/8] status: document runStatusAll() result --- cmd/dep/status.go | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/cmd/dep/status.go b/cmd/dep/status.go index 3917c61b3e..412626dacf 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -354,22 +354,24 @@ type MissingStatus struct { // errorStatus contains information about error and number of status failures. type errorStatus struct { - err error + err error + // count is for counting errors due to which we don't fail completely, but + // return partial results with missing/unknown data. count int } -func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (bool, bool, errorStatus) { - var digestMismatch, hasMissingPkgs bool - +func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (digestMismatch bool, hasMissingPkgs bool, errStatus errorStatus) { if p.Lock == nil { - return digestMismatch, hasMissingPkgs, errorStatus{err: errors.Errorf("no Gopkg.lock found. Run `dep ensure` to generate lock file")} + errStatus.err = errors.Errorf("no Gopkg.lock found. Run `dep ensure` to generate lock file") + return digestMismatch, hasMissingPkgs, errStatus } // While the network churns on ListVersions() requests, statically analyze // code from the current project. ptree, err := pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot)) if err != nil { - return digestMismatch, hasMissingPkgs, errorStatus{err: errors.Wrapf(err, "analysis of local packages failed")} + errStatus.err = errors.Wrapf(err, "analysis of local packages failed") + return digestMismatch, hasMissingPkgs, errStatus } // Set up a solver in order to check the InputHash. @@ -394,7 +396,8 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana s, err := gps.Prepare(params, sm) if err != nil { - return digestMismatch, hasMissingPkgs, errorStatus{err: errors.Wrapf(err, "could not set up solver for input hashing")} + errStatus.err = errors.Wrapf(err, "could not set up solver for input hashing") + return digestMismatch, hasMissingPkgs, errStatus } cm := collectConstraints(ptree, p, sm) @@ -521,12 +524,9 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana // Newline after printing the status progress output logger.Println() - var statusError error - var errorCount int - // List Packages errors. This would happen only for dot output. if len(errListPkgCh) > 0 { - statusError = errFailedListPkg + errStatus.err = errFailedListPkg if ctx.Verbose { for err := range errListPkgCh { ctx.Err.Println(err.Error()) @@ -537,13 +537,15 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana // List Version errors if len(errListVerCh) > 0 { - if statusError == errFailedListPkg { - statusError = errMultipleFailures + if errStatus.err == nil { + errStatus.err = errFailedUpdate } else { - statusError = errFailedUpdate + errStatus.err = errMultipleFailures } - errorCount = len(errListVerCh) + // Count ListVersions error because we get partial results when + // this happens. + errStatus.count = len(errListVerCh) if ctx.Verbose { for err := range errListVerCh { ctx.Err.Println(err.Error()) @@ -567,7 +569,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana out.BasicFooter() - return digestMismatch, hasMissingPkgs, errorStatus{err: statusError, count: errorCount} + return digestMismatch, hasMissingPkgs, errStatus } // Hash digest mismatch may indicate that some deps are no longer From 88230aadc478d4bbe80dd6ac0df30490d061a50e Mon Sep 17 00:00:00 2001 From: Sunny Date: Mon, 11 Sep 2017 21:36:32 +0530 Subject: [PATCH 7/8] status: refactor return statements Return explicit values instead of variables in runStatusAll(). --- cmd/dep/status.go | 48 ++++++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 30 deletions(-) diff --git a/cmd/dep/status.go b/cmd/dep/status.go index 412626dacf..d3d7b7c1a4 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -235,22 +235,22 @@ func (cmd *statusCommand) Run(ctx *dep.Ctx, args []string) error { } } - digestMismatch, hasMissingPkgs, errStatus := runStatusAll(ctx, out, p, sm) - if errStatus.err != nil { + digestMismatch, hasMissingPkgs, errCount, err := runStatusAll(ctx, out, p, sm) + if err != nil { // If it's only update errors - if errStatus.err == errFailedUpdate { + if err == errFailedUpdate { // Print the results with unknown data ctx.Out.Println(buf.String()) // Print the help when in non-verbose mode if !ctx.Verbose { - ctx.Out.Printf("The status of %d projects are unknown due to errors. Rerun with `-v` flag to see details.\n", errStatus.count) + ctx.Out.Printf("The status of %d projects are unknown due to errors. Rerun with `-v` flag to see details.\n", errCount) } } else { // List package failure or multiple failures ctx.Out.Println("Failed to get status. Rerun with `-v` flag to see details.") } - return errStatus.err + return err } if digestMismatch { @@ -352,26 +352,16 @@ type MissingStatus struct { MissingPackages []string } -// errorStatus contains information about error and number of status failures. -type errorStatus struct { - err error - // count is for counting errors due to which we don't fail completely, but - // return partial results with missing/unknown data. - count int -} - -func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (digestMismatch bool, hasMissingPkgs bool, errStatus errorStatus) { +func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (digestMismatch bool, hasMissingPkgs bool, errCount int, err error) { if p.Lock == nil { - errStatus.err = errors.Errorf("no Gopkg.lock found. Run `dep ensure` to generate lock file") - return digestMismatch, hasMissingPkgs, errStatus + return false, false, 0, errors.Errorf("no Gopkg.lock found. Run `dep ensure` to generate lock file") } // While the network churns on ListVersions() requests, statically analyze // code from the current project. ptree, err := pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot)) if err != nil { - errStatus.err = errors.Wrapf(err, "analysis of local packages failed") - return digestMismatch, hasMissingPkgs, errStatus + return false, false, 0, errors.Wrapf(err, "analysis of local packages failed") } // Set up a solver in order to check the InputHash. @@ -391,13 +381,12 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana } if err := ctx.ValidateParams(sm, params); err != nil { - return digestMismatch, hasMissingPkgs, errorStatus{err: err} + return false, false, 0, err } s, err := gps.Prepare(params, sm) if err != nil { - errStatus.err = errors.Wrapf(err, "could not set up solver for input hashing") - return digestMismatch, hasMissingPkgs, errStatus + return false, false, 0, errors.Wrapf(err, "could not set up solver for input hashing") } cm := collectConstraints(ptree, p, sm) @@ -526,7 +515,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana // List Packages errors. This would happen only for dot output. if len(errListPkgCh) > 0 { - errStatus.err = errFailedListPkg + err = errFailedListPkg if ctx.Verbose { for err := range errListPkgCh { ctx.Err.Println(err.Error()) @@ -537,15 +526,15 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana // List Version errors if len(errListVerCh) > 0 { - if errStatus.err == nil { - errStatus.err = errFailedUpdate + if err == nil { + err = errFailedUpdate } else { - errStatus.err = errMultipleFailures + err = errMultipleFailures } // Count ListVersions error because we get partial results when // this happens. - errStatus.count = len(errListVerCh) + errCount = len(errListVerCh) if ctx.Verbose { for err := range errListVerCh { ctx.Err.Println(err.Error()) @@ -569,7 +558,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana out.BasicFooter() - return digestMismatch, hasMissingPkgs, errStatus + return false, false, errCount, err } // Hash digest mismatch may indicate that some deps are no longer @@ -578,7 +567,6 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana // // It's possible for digests to not match, but still have a correct // lock. - digestMismatch = true rm, _ := ptree.ToReachMap(true, true, false, nil) external := rm.FlattenFn(paths.IsStandardImportPath) @@ -611,7 +599,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana ctx.Err.Printf("\t%s: %s\n", fail.ex, fail.err.Error()) } - return digestMismatch, hasMissingPkgs, errorStatus{err: errors.New("address issues with undeducible import paths to get more status information")} + return true, false, 0, errors.New("address issues with undeducible import paths to get more status information") } out.MissingHeader() @@ -631,7 +619,7 @@ outer: } out.MissingFooter() - return digestMismatch, hasMissingPkgs, errorStatus{} + return true, hasMissingPkgs, 0, nil } func formatVersion(v gps.Version) string { From 17bb4d6e140994d9bba48e853df6dbeb4fd4c14a Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 21 Sep 2017 23:42:41 +0530 Subject: [PATCH 8/8] status: cleanup --- cmd/dep/status.go | 25 ++++++++++++------------- cmd/dep/status_test.go | 2 +- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/cmd/dep/status.go b/cmd/dep/status.go index d3d7b7c1a4..fa93507978 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -44,8 +44,8 @@ Status returns exit code zero if all dependencies are in a "good state". ` const ( - shortRev = iota - longRev = iota + shortRev uint8 = iota + longRev ) var ( @@ -317,7 +317,7 @@ func (bs *BasicStatus) getConsolidatedVersion() string { return version } -func (bs *BasicStatus) getConsolidatedLatest(revSize int) string { +func (bs *BasicStatus) getConsolidatedLatest(revSize uint8) string { latest := "" if bs.Latest != nil { switch revSize { @@ -408,10 +408,10 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana logger.Println("Checking upstream projects:") - // BasicStatus channel to collect all the BasicStatus + // BasicStatus channel to collect all the BasicStatus. bsCh := make(chan *BasicStatus, len(slp)) - // Error channels to collect different errors + // Error channels to collect different errors. errListPkgCh := make(chan error, len(slp)) errListVerCh := make(chan error, len(slp)) @@ -422,15 +422,13 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana logger.Printf("(%d/%d) %s\n", i+1, len(slp), proj.Ident().ProjectRoot) go func(proj gps.LockedProject) { - defer wg.Done() - bs := BasicStatus{ ProjectRoot: string(proj.Ident().ProjectRoot), PackageCount: len(proj.Packages()), } // Get children only for specific outputers - // in order to avoid slower status process + // in order to avoid slower status process. switch out.(type) { case *dotOutput: ptr, err := sm.ListPackages(proj.Ident(), proj.Version()) @@ -444,7 +442,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana bs.Children = prm.FlattenFn(paths.IsStandardImportPath) } - // Split apart the version from the lock into its constituent parts + // Split apart the version from the lock into its constituent parts. switch tv := proj.Version().(type) { case gps.UnpairedVersion: bs.Version = tv @@ -494,7 +492,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana } } else { // Failed to fetch version list (could happen due to - // network issue) + // network issue). bs.hasError = true errListVerCh <- err } @@ -502,6 +500,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana bsCh <- &bs + wg.Done() }(proj) } @@ -510,7 +509,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana close(errListPkgCh) close(errListVerCh) - // Newline after printing the status progress output + // Newline after printing the status progress output. logger.Println() // List Packages errors. This would happen only for dot output. @@ -524,7 +523,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana } } - // List Version errors + // List Version errors. if len(errListVerCh) > 0 { if err == nil { err = errFailedUpdate @@ -551,7 +550,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana bsMap[bs.ProjectRoot] = bs } - // Use the collected BasicStatus in outputter + // Use the collected BasicStatus in outputter. for _, proj := range slp { out.BasicLine(bsMap[string(proj.Ident().ProjectRoot)]) } diff --git a/cmd/dep/status_test.go b/cmd/dep/status_test.go index 214d27390c..151912a18c 100644 --- a/cmd/dep/status_test.go +++ b/cmd/dep/status_test.go @@ -235,7 +235,7 @@ func TestBasicStatusGetConsolidatedLatest(t *testing.T) { testCases := []struct { name string basicStatus BasicStatus - revSize int + revSize uint8 wantLatest string }{ {