From 5583b686ff021addbcce62b60befe98638a57ade Mon Sep 17 00:00:00 2001 From: sam boyer Date: Mon, 15 Jan 2018 23:41:36 -0800 Subject: [PATCH 1/7] init: Break out huge Run func into methods a bit --- cmd/dep/failures.go | 23 ++++++++++ cmd/dep/init.go | 101 ++++++++++++++++++++++--------------------- cmd/dep/init_test.go | 40 ----------------- 3 files changed, 74 insertions(+), 90 deletions(-) create mode 100644 cmd/dep/failures.go delete mode 100644 cmd/dep/init_test.go diff --git a/cmd/dep/failures.go b/cmd/dep/failures.go new file mode 100644 index 0000000000..c40ac8c785 --- /dev/null +++ b/cmd/dep/failures.go @@ -0,0 +1,23 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "context" + + "github.com/golang/dep/gps" + "github.com/pkg/errors" +) + +// TODO solve failures can be really creative - we need to be similarly creative +// in handling them and informing the user appropriately +func handleAllTheFailuresOfTheWorld(err error) error { + switch errors.Cause(err) { + case context.Canceled, context.DeadlineExceeded, gps.ErrSourceManagerIsReleased: + return nil + } + + return errors.Wrap(err, "Solving failure") +} diff --git a/cmd/dep/init.go b/cmd/dep/init.go index e09c1f20b0..376f920f1b 100644 --- a/cmd/dep/init.go +++ b/cmd/dep/init.go @@ -89,43 +89,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { } } - var err error - p := new(dep.Project) - if err = p.SetRoot(root); err != nil { - return errors.Wrapf(err, "init failed: unable to set the root project to %s", root) - } - - ctx.GOPATH, err = ctx.DetectProjectGOPATH(p) - if err != nil { - return errors.Wrapf(err, "init failed: unable to detect the containing GOPATH") - } - - mf := filepath.Join(root, dep.ManifestName) - lf := filepath.Join(root, dep.LockName) - vpath := filepath.Join(root, "vendor") - - mok, err := fs.IsRegular(mf) - if err != nil { - return errors.Wrapf(err, "init failed: unable to check for an existing manifest at %s", mf) - } - if mok { - return errors.Errorf("init aborted: manifest already exists at %s", mf) - } - // Manifest file does not exist. - - lok, err := fs.IsRegular(lf) - if err != nil { - return errors.Wrapf(err, "init failed: unable to check for an existing lock at %s", lf) - } - if lok { - return errors.Errorf("invalid aborted: lock already exists at %s", lf) - } - - ip, err := ctx.ImportForAbs(root) - if err != nil { - return errors.Wrapf(err, "init failed: unable to determine the import path for the root project %s", root) - } - p.ImportRoot = gps.ProjectRoot(ip) + p, err := cmd.establishProjectAt(root, ctx) sm, err := ctx.SourceManager() if err != nil { @@ -137,7 +101,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { if ctx.Verbose { ctx.Out.Println("Getting direct dependencies...") } - pkgT, directDeps, err := getDirectDependencies(sm, p) + pkgT, directDeps, err := cmd.getDirectDependencies(sm, p) if err != nil { return errors.Wrap(err, "init failed: unable to determine direct dependencies") } @@ -203,7 +167,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { p.Lock.SolveMeta.InputsDigest = s.HashInputs() // Pass timestamp (yyyyMMddHHmmss format) as suffix to backup name. - vendorbak, err := dep.BackupVendor(vpath, time.Now().Format("20060102150405")) + vendorbak, err := dep.BackupVendor(filepath.Join(root, "vendor"), time.Now().Format("20060102150405")) if err != nil { return errors.Wrap(err, "init failed: first backup vendor/, delete it, and then retry the previous command: failed to backup existing vendor directory") } @@ -227,6 +191,54 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { return nil } +// establishProjectAt attempts to set up the provided path as the root for the +// project to be created. +// +// It checks for being within a GOPATH, that there is no pre-existing manifest +// and lock, and that we can successfully infer the root import path from +// GOPATH. +// +// If successful, it returns a dep.Project, ready for further use. +func (cmd *initCommand) establishProjectAt(root string, ctx *dep.Ctx) (*dep.Project, error) { + var err error + p := new(dep.Project) + if err = p.SetRoot(root); err != nil { + return nil, errors.Wrapf(err, "init failed: unable to set the root project to %s", root) + } + + ctx.GOPATH, err = ctx.DetectProjectGOPATH(p) + if err != nil { + return nil, errors.Wrapf(err, "init failed: unable to detect the containing GOPATH") + } + + mf := filepath.Join(root, dep.ManifestName) + lf := filepath.Join(root, dep.LockName) + + mok, err := fs.IsRegular(mf) + if err != nil { + return nil, errors.Wrapf(err, "init failed: unable to check for an existing manifest at %s", mf) + } + if mok { + return nil, errors.Errorf("init aborted: manifest already exists at %s", mf) + } + + lok, err := fs.IsRegular(lf) + if err != nil { + return nil, errors.Wrapf(err, "init failed: unable to check for an existing lock at %s", lf) + } + if lok { + return nil, errors.Errorf("invalid aborted: lock already exists at %s", lf) + } + + ip, err := ctx.ImportForAbs(root) + if err != nil { + return nil, errors.Wrapf(err, "init failed: unable to determine the import path for the root project %s", root) + } + p.ImportRoot = gps.ProjectRoot(ip) + + return p, nil +} + func getDirectDependencies(sm gps.SourceManager, p *dep.Project) (pkgtree.PackageTree, map[string]bool, error) { pkgT, err := p.ParseRootPackageTree() if err != nil { @@ -245,14 +257,3 @@ func getDirectDependencies(sm gps.SourceManager, p *dep.Project) (pkgtree.Packag return pkgT, directDeps, nil } - -// TODO solve failures can be really creative - we need to be similarly creative -// in handling them and informing the user appropriately -func handleAllTheFailuresOfTheWorld(err error) error { - switch errors.Cause(err) { - case context.Canceled, context.DeadlineExceeded, gps.ErrSourceManagerIsReleased: - return nil - } - - return errors.Wrap(err, "Solving failure") -} diff --git a/cmd/dep/init_test.go b/cmd/dep/init_test.go deleted file mode 100644 index 0d0aff23ba..0000000000 --- a/cmd/dep/init_test.go +++ /dev/null @@ -1,40 +0,0 @@ -// Copyright 2016 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package main - -import ( - "path/filepath" - "testing" - - "github.com/golang/dep" - "github.com/golang/dep/gps" - "github.com/golang/dep/internal/test" -) - -func TestGetDirectDependencies_ConsolidatesRootProjects(t *testing.T) { - h := test.NewHelper(t) - defer h.Cleanup() - - ctx := NewTestContext(h) - sm, err := ctx.SourceManager() - h.Must(err) - defer sm.Release() - - testprj := "directdepstest" - testdir := filepath.Join("src", testprj) - h.TempDir(testdir) - h.TempCopy(filepath.Join(testdir, "main.go"), "init/directdeps/main.go") - - testpath := h.Path(testdir) - prj := &dep.Project{AbsRoot: testpath, ResolvedAbsRoot: testpath, ImportRoot: gps.ProjectRoot(testprj)} - - _, dd, err := getDirectDependencies(sm, prj) - h.Must(err) - - wantpr := "github.com/carolynvs/deptest-subpkg" - if _, has := dd[wantpr]; !has { - t.Fatalf("Expected direct dependencies to contain %s, got %v", wantpr, dd) - } -} From 97b8be8a1baea94accfafb38521576a01683b018 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Mon, 15 Jan 2018 23:42:15 -0800 Subject: [PATCH 2/7] dep: Add Project.GetDirectDependencyNames() This method is a a complete and correct implementation for retrieving the list of direct dependency names. It should be able to be used by all dep commands, if needed. --- cmd/dep/init.go | 2 +- project.go | 83 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 76 insertions(+), 9 deletions(-) diff --git a/cmd/dep/init.go b/cmd/dep/init.go index 376f920f1b..4372fa21ce 100644 --- a/cmd/dep/init.go +++ b/cmd/dep/init.go @@ -101,7 +101,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { if ctx.Verbose { ctx.Out.Println("Getting direct dependencies...") } - pkgT, directDeps, err := cmd.getDirectDependencies(sm, p) + pkgT, directDeps, err := getDirectDependencies(sm, p) if err != nil { return errors.Wrap(err, "init failed: unable to determine direct dependencies") } diff --git a/project.go b/project.go index 2da46dc497..a722eae83d 100644 --- a/project.go +++ b/project.go @@ -10,6 +10,7 @@ import ( "path/filepath" "github.com/golang/dep/gps" + "github.com/golang/dep/gps/paths" "github.com/golang/dep/gps/pkgtree" "github.com/golang/dep/internal/fs" "github.com/pkg/errors" @@ -99,9 +100,10 @@ type Project struct { // If AbsRoot is not a symlink, then ResolvedAbsRoot should equal AbsRoot. ResolvedAbsRoot string // ImportRoot is the import path of the project's root directory. - ImportRoot gps.ProjectRoot - Manifest *Manifest - Lock *Lock // Optional + ImportRoot gps.ProjectRoot + Manifest *Manifest + Lock *Lock // Optional + RootPackageTree pkgtree.PackageTree } // SetRoot sets the project AbsRoot and ResolvedAbsRoot. If root is not a symlink, ResolvedAbsRoot will be set to root. @@ -137,18 +139,83 @@ func (p *Project) MakeParams() gps.SolveParameters { // ParseRootPackageTree analyzes the root project's disk contents to create a // PackageTree, trimming out packages that are not relevant for root projects // along the way. +// +// The resulting tree is cached internally at p.RootPackageTree. func (p *Project) ParseRootPackageTree() (pkgtree.PackageTree, error) { - ptree, err := pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot)) + if len(p.RootPackageTree.Packages) == 0 { + ptree, err := pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot)) + if err != nil { + return pkgtree.PackageTree{}, errors.Wrap(err, "analysis of current project's packages failed") + } + // We don't care about (unreachable) hidden packages for the root project, + // so drop all of those. + var ig *pkgtree.IgnoredRuleset + if p.Manifest != nil { + ig = p.Manifest.IgnoredPackages() + } + p.RootPackageTree = ptree.TrimHiddenPackages(true, true, ig) + } + return p.RootPackageTree, nil +} + +// GetDirectDependencyNames returns the set of unique Project Roots that are the +// direct dependencies of this Project. +// +// A project is considered a direct dependency if at least one of packages in it +// is named in either this Project's required list, or if there is at least one +// non-ignored import statement from a non-ignored package in the current +// project's package tree. +// +// The returned map of Project Roots contains only boolean true values; this +// makes a "false" value always indicate an absent key, which makes if checks +// against the map more ergonomic. +// +// This function will correctly utilize ignores and requireds from an existing +// manifest, if one is present, but will also do the right thing without a +// manifest. +func (p *Project) GetDirectDependencyNames(sm gps.SourceManager) (map[gps.ProjectRoot]bool, error) { + ptree, err := p.ParseRootPackageTree() if err != nil { - return pkgtree.PackageTree{}, errors.Wrap(err, "analysis of current project's packages failed") + return nil, err } - // We don't care about (unreachable) hidden packages for the root project, - // so drop all of those. + var ig *pkgtree.IgnoredRuleset + var req map[string]bool if p.Manifest != nil { ig = p.Manifest.IgnoredPackages() + req = p.Manifest.RequiredPackages() + } + + rm, _ := ptree.ToReachMap(true, true, false, ig) + reach := rm.FlattenFn(paths.IsStandardImportPath) + + if len(req) > 0 { + // Make a map of imports that are both in the import path list and the + // required list to avoid duplication. + skip := make(map[string]bool, len(req)) + for _, r := range reach { + if req[r] { + skip[r] = true + } + } + + for r := range req { + if !skip[r] { + reach = append(reach, r) + } + } } - return ptree.TrimHiddenPackages(true, true, ig), nil + + directDeps := map[gps.ProjectRoot]bool{} + for _, ip := range reach { + pr, err := sm.DeduceProjectRoot(ip) + if err != nil { + return nil, err + } + directDeps[pr] = true + } + + return directDeps, nil } // BackupVendor looks for existing vendor directory and if it's not empty, From 6fc8e052d2464cad7465d849fad1481c554e7498 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 16 Jan 2018 01:18:05 -0800 Subject: [PATCH 3/7] dep: Convert to using GetDirectDependencyNames() Specifically, we need to switch map[string]bool for map[gps.ProjectRoot]bool. Trivial refactor, but it's preferable to do this anyway as this is a situation where gps.ProjectRoot should be used to denote that strings in this map are assumed to be project roots. Case in point, one of the importers was making the assumption that it had packages, not root paths, but it wasn't obvious without the type hinting. --- cmd/dep/gopath_scanner.go | 11 +++++++---- cmd/dep/init.go | 32 +++++++------------------------- cmd/dep/root_analyzer.go | 10 +++++----- cmd/dep/status.go | 7 ++++--- project.go | 4 ++-- 5 files changed, 25 insertions(+), 39 deletions(-) diff --git a/cmd/dep/gopath_scanner.go b/cmd/dep/gopath_scanner.go index 5bf82ccddd..ed94d8d49f 100644 --- a/cmd/dep/gopath_scanner.go +++ b/cmd/dep/gopath_scanner.go @@ -24,7 +24,7 @@ import ( // It uses its results to fill-in any missing details left by the rootAnalyzer. type gopathScanner struct { ctx *dep.Ctx - directDeps map[string]bool + directDeps map[gps.ProjectRoot]bool sm gps.SourceManager pd projectData @@ -32,7 +32,7 @@ type gopathScanner struct { origL *dep.Lock } -func newGopathScanner(ctx *dep.Ctx, directDeps map[string]bool, sm gps.SourceManager) *gopathScanner { +func newGopathScanner(ctx *dep.Ctx, directDeps map[gps.ProjectRoot]bool, sm gps.SourceManager) *gopathScanner { return &gopathScanner{ ctx: ctx, directDeps: directDeps, @@ -113,7 +113,7 @@ func (g *gopathScanner) overlay(rootM *dep.Manifest, rootL *dep.Lock) { rootL.P = append(rootL.P, lp) lockedProjects[pkg] = true - if _, isDirect := g.directDeps[string(pkg)]; !isDirect { + if _, isDirect := g.directDeps[pkg]; !isDirect { f := fb.NewLockedProjectFeedback(lp, fb.DepTypeTransitive) f.LogFeedback(g.ctx.Err) } @@ -220,7 +220,10 @@ func (g *gopathScanner) scanGopathForDependencies() (projectData, error) { return projectData{}, nil } - for ip := range g.directDeps { + for ippr := range g.directDeps { + // TODO(sdboyer) these are not import paths by this point, they've + // already been worked down to project roots. + ip := string(ippr) pr, err := g.sm.DeduceProjectRoot(ip) if err != nil { return projectData{}, errors.Wrap(err, "sm.DeduceProjectRoot") diff --git a/cmd/dep/init.go b/cmd/dep/init.go index 4372fa21ce..180f6d5d45 100644 --- a/cmd/dep/init.go +++ b/cmd/dep/init.go @@ -15,13 +15,11 @@ import ( "github.com/golang/dep" "github.com/golang/dep/gps" - "github.com/golang/dep/gps/paths" - "github.com/golang/dep/gps/pkgtree" "github.com/golang/dep/internal/fs" "github.com/pkg/errors" ) -const initShortHelp = `Initialize a new project with manifest and lock files` +const initShortHelp = `Set up a new Go project, or migrate an existing one` const initLongHelp = ` Initialize the project at filepath root by parsing its dependencies, writing manifest and lock files, and vendoring the dependencies. If root isn't @@ -101,12 +99,15 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { if ctx.Verbose { ctx.Out.Println("Getting direct dependencies...") } - pkgT, directDeps, err := getDirectDependencies(sm, p) + + // If this errors, the next call will too; don't bother handling it twice. + ptree, _ := p.ParseRootPackageTree() + directDeps, err := p.GetDirectDependencyNames(sm) if err != nil { return errors.Wrap(err, "init failed: unable to determine direct dependencies") } if ctx.Verbose { - ctx.Out.Printf("Checked %d directories for packages.\nFound %d direct dependencies.\n", len(pkgT.Packages), len(directDeps)) + ctx.Out.Printf("Checked %d directories for packages.\nFound %d direct dependencies.\n", len(ptree.Packages), len(directDeps)) } // Initialize with imported data, then fill in the gaps using the GOPATH @@ -129,7 +130,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { params := gps.SolveParameters{ RootDir: root, - RootPackageTree: pkgT, + RootPackageTree: ptree, Manifest: p.Manifest, Lock: p.Lock, ProjectAnalyzer: rootAnalyzer, @@ -238,22 +239,3 @@ func (cmd *initCommand) establishProjectAt(root string, ctx *dep.Ctx) (*dep.Proj return p, nil } - -func getDirectDependencies(sm gps.SourceManager, p *dep.Project) (pkgtree.PackageTree, map[string]bool, error) { - pkgT, err := p.ParseRootPackageTree() - if err != nil { - return pkgtree.PackageTree{}, nil, err - } - - directDeps := map[string]bool{} - rm, _ := pkgT.ToReachMap(true, true, false, nil) - for _, ip := range rm.FlattenFn(paths.IsStandardImportPath) { - pr, err := sm.DeduceProjectRoot(ip) - if err != nil { - return pkgtree.PackageTree{}, nil, err - } - directDeps[string(pr)] = true - } - - return pkgT, directDeps, nil -} diff --git a/cmd/dep/root_analyzer.go b/cmd/dep/root_analyzer.go index 0dcf8683b3..76d410fe6e 100644 --- a/cmd/dep/root_analyzer.go +++ b/cmd/dep/root_analyzer.go @@ -26,10 +26,10 @@ type rootAnalyzer struct { skipTools bool ctx *dep.Ctx sm gps.SourceManager - directDeps map[string]bool + directDeps map[gps.ProjectRoot]bool } -func newRootAnalyzer(skipTools bool, ctx *dep.Ctx, directDeps map[string]bool, sm gps.SourceManager) *rootAnalyzer { +func newRootAnalyzer(skipTools bool, ctx *dep.Ctx, directDeps map[gps.ProjectRoot]bool, sm gps.SourceManager) *rootAnalyzer { return &rootAnalyzer{ skipTools: skipTools, ctx: ctx, @@ -73,7 +73,7 @@ func (a *rootAnalyzer) cacheDeps(pr gps.ProjectRoot) error { return nil } - deps := make(chan string) + deps := make(chan gps.ProjectRoot) for i := 0; i < concurrency; i++ { g.Go(func() error { @@ -132,7 +132,7 @@ func (a *rootAnalyzer) importManifestAndLock(dir string, pr gps.ProjectRoot, sup func (a *rootAnalyzer) removeTransitiveDependencies(m *dep.Manifest) { for pr := range m.Constraints { - if _, isDirect := a.directDeps[string(pr)]; !isDirect { + if _, isDirect := a.directDeps[pr]; !isDirect { delete(m.Constraints, pr) } } @@ -172,7 +172,7 @@ func (a *rootAnalyzer) FinalizeRootManifestAndLock(m *dep.Manifest, l *dep.Lock, var f *fb.ConstraintFeedback pr := y.Ident().ProjectRoot // New constraints: in new lock and dir dep but not in manifest - if _, ok := a.directDeps[string(pr)]; ok { + if _, ok := a.directDeps[pr]; ok { if _, ok := m.Constraints[pr]; !ok { pp := getProjectPropertiesFromVersion(y.Version()) if pp.Constraint != nil { diff --git a/cmd/dep/status.go b/cmd/dep/status.go index 1a24696cc6..de09497c26 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -764,8 +764,9 @@ func collectConstraints(ctx *dep.Ctx, p *dep.Project, sm gps.SourceManager) (con var mutex sync.Mutex constraintCollection := make(constraintsCollection) - // Get direct deps of the root project. - _, directDeps, err := getDirectDependencies(sm, p) + // Collect the complete set of direct project dependencies, incorporating + // requireds and ignores appropriately. + directDeps, err := p.GetDirectDependencyNames(sm) if err != nil { // Return empty collection, not nil, if we fail here. return constraintCollection, []error{errors.Wrap(err, "failed to get direct dependencies")} @@ -805,7 +806,7 @@ func collectConstraints(ctx *dep.Ctx, p *dep.Project, sm gps.SourceManager) (con // project and constraint values. for pr, pp := range pc { // Check if the project constraint is imported in the root project - if _, ok := directDeps[string(pr)]; !ok { + if _, ok := directDeps[pr]; !ok { continue } diff --git a/project.go b/project.go index a722eae83d..6686dd7f8a 100644 --- a/project.go +++ b/project.go @@ -167,8 +167,8 @@ func (p *Project) ParseRootPackageTree() (pkgtree.PackageTree, error) { // project's package tree. // // The returned map of Project Roots contains only boolean true values; this -// makes a "false" value always indicate an absent key, which makes if checks -// against the map more ergonomic. +// makes a "false" value always indicate an absent key, which makes conditional +// checks against the map more ergonomic. // // This function will correctly utilize ignores and requireds from an existing // manifest, if one is present, but will also do the right thing without a From 6a5fcbd8e8ebdbdb9b68acd355b2da307edcd45e Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 16 Jan 2018 01:20:48 -0800 Subject: [PATCH 4/7] dep: Add ineffectual constraints finder and warn Finally fixes #302. --- cmd/dep/ensure.go | 14 ++++++++++++++ project.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/cmd/dep/ensure.go b/cmd/dep/ensure.go index 3b0b1041ee..be02379426 100644 --- a/cmd/dep/ensure.go +++ b/cmd/dep/ensure.go @@ -197,6 +197,20 @@ func (cmd *ensureCommand) Run(ctx *dep.Ctx, args []string) error { ctx.Out.Println(err) } } + if ineffs := p.FindIneffectualConstraints(sm); len(ineffs) > 0 { + ctx.Err.Printf("Warning: the following project(s) have [[constraint]] stanzas in %s:\n\n", dep.ManifestName) + for _, ineff := range ineffs { + ctx.Err.Println(" ✗ ", ineff) + } + // TODO(sdboyer) lazy wording, it does not mention ignores at all + ctx.Err.Printf("\nHowever, these projects are not direct dependencies of the current project:\n") + ctx.Err.Printf("they are not imported in any .go files, nor are they in the 'required' list in\n") + ctx.Err.Printf("%s. Dep only applies [[constraint]] rules to direct dependencies, so\n", dep.ManifestName) + ctx.Err.Printf("these rules will have no effect.\n\n") + ctx.Err.Printf("Either or import/require packages from these projects to make them into direct\n") + ctx.Err.Printf("dependencies, or convert the [[constraint]] to an [[override]] to enforce rules\n") + ctx.Err.Printf("on these projects if they are transitive dependencies,\n\n") + } if cmd.add { return cmd.runAdd(ctx, args, p, sm, params) diff --git a/project.go b/project.go index 6686dd7f8a..5f25c48e66 100644 --- a/project.go +++ b/project.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "github.com/golang/dep/gps" "github.com/golang/dep/gps/paths" @@ -218,6 +219,35 @@ func (p *Project) GetDirectDependencyNames(sm gps.SourceManager) (map[gps.Projec return directDeps, nil } +// FindIneffectualConstraints looks for constraint rules expressed in the +// manifest that will have no effect during solving, as they are specified for +// projects that are not direct dependencies of the Project. +// +// "Direct dependency" here is as implemented by GetDirectDependencyNames() - +// after all "ignored" and "required" rules have been considered. +func (p *Project) FindIneffectualConstraints(sm gps.SourceManager) []gps.ProjectRoot { + if p.Manifest == nil { + return nil + } + + dd, err := p.GetDirectDependencyNames(sm) + if err != nil { + return nil + } + + var ineff []gps.ProjectRoot + for pr := range p.Manifest.DependencyConstraints() { + if !dd[pr] { + ineff = append(ineff, pr) + } + } + + sort.Slice(ineff, func(i, j int) bool { + return ineff[i] < ineff[j] + }) + return ineff +} + // BackupVendor looks for existing vendor directory and if it's not empty, // creates a backup of it to a new directory with the provided suffix. func BackupVendor(vpath, suffix string) (string, error) { From 689880a0a3d2fff6ab06595b715dce358050195c Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 16 Jan 2018 01:28:06 -0800 Subject: [PATCH 5/7] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b84258f1d5..dac5a2f56e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ IMPROVEMENTS: * Add constraint for locked projects in `dep status`. ([#962](https://github.com/golang/dep/pull/962) * Make external config importers error tolerant. ([#1315](https://github.com/golang/dep/pull/1315)) * Show LATEST and VERSION as the same type in status. ([#1515](https://github.com/golang/dep/pull/1515) +* Warn when [[constraint]] rules that will have no effect. ([#1534](https://github.com/golang/dep/pull/1534)) # v0.3.2 From 2eeefbd1d7cb1a7361de6e692f82f2f11f6d2741 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 16 Jan 2018 08:28:36 -0800 Subject: [PATCH 6/7] dep: Emit ptree from GetDirectDependencyNames() A convenience, as callers often need both the RootPackageTree and the direct dependencies map. Though this does further suggest that we ought to be able to stitch this up neatly in gps, at some point. Also, handle an error that was previously dropped. --- cmd/dep/init.go | 7 ++++--- cmd/dep/status.go | 2 +- project.go | 16 ++++++++-------- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/cmd/dep/init.go b/cmd/dep/init.go index 180f6d5d45..e8c255c87d 100644 --- a/cmd/dep/init.go +++ b/cmd/dep/init.go @@ -88,6 +88,9 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { } p, err := cmd.establishProjectAt(root, ctx) + if err != nil { + return err + } sm, err := ctx.SourceManager() if err != nil { @@ -100,9 +103,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { ctx.Out.Println("Getting direct dependencies...") } - // If this errors, the next call will too; don't bother handling it twice. - ptree, _ := p.ParseRootPackageTree() - directDeps, err := p.GetDirectDependencyNames(sm) + ptree, directDeps, err := p.GetDirectDependencyNames(sm) if err != nil { return errors.Wrap(err, "init failed: unable to determine direct dependencies") } diff --git a/cmd/dep/status.go b/cmd/dep/status.go index de09497c26..c342b20f29 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -766,7 +766,7 @@ func collectConstraints(ctx *dep.Ctx, p *dep.Project, sm gps.SourceManager) (con // Collect the complete set of direct project dependencies, incorporating // requireds and ignores appropriately. - directDeps, err := p.GetDirectDependencyNames(sm) + _, directDeps, err := p.GetDirectDependencyNames(sm) if err != nil { // Return empty collection, not nil, if we fail here. return constraintCollection, []error{errors.Wrap(err, "failed to get direct dependencies")} diff --git a/project.go b/project.go index 5f25c48e66..d2677e8866 100644 --- a/project.go +++ b/project.go @@ -143,7 +143,7 @@ func (p *Project) MakeParams() gps.SolveParameters { // // The resulting tree is cached internally at p.RootPackageTree. func (p *Project) ParseRootPackageTree() (pkgtree.PackageTree, error) { - if len(p.RootPackageTree.Packages) == 0 { + if p.RootPackageTree.Packages == nil { ptree, err := pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot)) if err != nil { return pkgtree.PackageTree{}, errors.Wrap(err, "analysis of current project's packages failed") @@ -174,10 +174,10 @@ func (p *Project) ParseRootPackageTree() (pkgtree.PackageTree, error) { // This function will correctly utilize ignores and requireds from an existing // manifest, if one is present, but will also do the right thing without a // manifest. -func (p *Project) GetDirectDependencyNames(sm gps.SourceManager) (map[gps.ProjectRoot]bool, error) { +func (p *Project) GetDirectDependencyNames(sm gps.SourceManager) (pkgtree.PackageTree, map[gps.ProjectRoot]bool, error) { ptree, err := p.ParseRootPackageTree() if err != nil { - return nil, err + return pkgtree.PackageTree{}, nil, err } var ig *pkgtree.IgnoredRuleset @@ -211,26 +211,26 @@ func (p *Project) GetDirectDependencyNames(sm gps.SourceManager) (map[gps.Projec for _, ip := range reach { pr, err := sm.DeduceProjectRoot(ip) if err != nil { - return nil, err + return pkgtree.PackageTree{}, nil, err } directDeps[pr] = true } - return directDeps, nil + return ptree, directDeps, nil } // FindIneffectualConstraints looks for constraint rules expressed in the // manifest that will have no effect during solving, as they are specified for // projects that are not direct dependencies of the Project. // -// "Direct dependency" here is as implemented by GetDirectDependencyNames() - -// after all "ignored" and "required" rules have been considered. +// "Direct dependency" here is as implemented by GetDirectDependencyNames(); +// it correctly incorporates all "ignored" and "required" rules. func (p *Project) FindIneffectualConstraints(sm gps.SourceManager) []gps.ProjectRoot { if p.Manifest == nil { return nil } - dd, err := p.GetDirectDependencyNames(sm) + _, dd, err := p.GetDirectDependencyNames(sm) if err != nil { return nil } From b89aa7e97679b16b85109eadef02423960f7d814 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 16 Jan 2018 20:19:28 -0800 Subject: [PATCH 7/7] dep: Wording tweaks on ineffectuals error message --- cmd/dep/ensure.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/dep/ensure.go b/cmd/dep/ensure.go index be02379426..202ec35505 100644 --- a/cmd/dep/ensure.go +++ b/cmd/dep/ensure.go @@ -207,9 +207,9 @@ func (cmd *ensureCommand) Run(ctx *dep.Ctx, args []string) error { ctx.Err.Printf("they are not imported in any .go files, nor are they in the 'required' list in\n") ctx.Err.Printf("%s. Dep only applies [[constraint]] rules to direct dependencies, so\n", dep.ManifestName) ctx.Err.Printf("these rules will have no effect.\n\n") - ctx.Err.Printf("Either or import/require packages from these projects to make them into direct\n") - ctx.Err.Printf("dependencies, or convert the [[constraint]] to an [[override]] to enforce rules\n") - ctx.Err.Printf("on these projects if they are transitive dependencies,\n\n") + ctx.Err.Printf("Either import/require packages from these projects so that they become direct\n") + ctx.Err.Printf("dependencies, or convert each [[constraint]] to an [[override]] to enforce rules\n") + ctx.Err.Printf("on these projects, if they happen to be transitive dependencies,\n\n") } if cmd.add { @@ -529,7 +529,7 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm } inManifest := p.Manifest.HasConstraintsOn(pc.Ident.ProjectRoot) - inImports := exrmap[pc.Ident.ProjectRoot] + inImports := exmap[string(pc.Ident.ProjectRoot)] if inManifest && inImports { errCh <- errors.Errorf("nothing to -add, %s is already in %s and the project's direct imports or required list", pc.Ident.ProjectRoot, dep.ManifestName) return