Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Warn on ineffectual constraints #1534

Merged
merged 7 commits into from
Jan 17, 2018
Merged

Warn on ineffectual constraints #1534

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5583b68
init: Break out huge Run func into methods a bit
sdboyer Jan 16, 2018
97b8be8
dep: Add Project.GetDirectDependencyNames()
sdboyer Jan 16, 2018
6fc8e05
dep: Convert to using GetDirectDependencyNames()
sdboyer Jan 16, 2018
6a5fcbd
dep: Add ineffectual constraints finder and warn
sdboyer Jan 16, 2018
689880a
Update CHANGELOG
sdboyer Jan 16, 2018
2eeefbd
dep: Emit ptree from GetDirectDependencyNames()
sdboyer Jan 16, 2018
b89aa7e
dep: Wording tweaks on ineffectuals error message
sdboyer Jan 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
14 changes: 14 additions & 0 deletions cmd/dep/ensure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 23 additions & 0 deletions cmd/dep/failures.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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")
}
11 changes: 7 additions & 4 deletions cmd/dep/gopath_scanner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ 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
origM *dep.Manifest
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,
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carolynvs i noticed this while working on this. do you have any immediate thoughts about bugs we might have as a result of this pseudotype mismatch?

// 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")
Expand Down
111 changes: 47 additions & 64 deletions cmd/dep/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -89,43 +87,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 {
Expand All @@ -137,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.
Copy link
Collaborator

@jmank88 jmank88 Jan 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems brittle.

Since GetDirectDependencyNames calls ParseRootPackageTree internally, what about an additional ptree return from GetDirectDependencyNames?

GetDirectDependencyNames(sm gps.SourceManager) (pkgtree.PackageTree, map[gps.ProjectRoot]bool, error) {

Edit: If this was the only reason for caching the result internally, then that could be simplified out too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that nil pointer is from the cache field too: if len(p.RootPackageTree.Packages)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i waffled over this a bit. it was between that signature, and what i did here - because we absolutely can't create a circumstance where casual use of the API results in hitting the disk twice for data that, at least in dep's model, hasn't changed.

however, i didn't particularly like having the extra return value - it felt like exposing a bit too much detail of the underlying behavior. the ptree isn't what we asked for here. but, now that i've finished writing it and looking back, i'd agree that the cost is less than the complexity arising from adding this new statefulness dimension to *dep.Project.

so yeah, i'll switch it around.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually looking at the code again, i've flipped back. not caching it internally also makes the FindIneffectualConstraints() method much more complicated to implement.

there are a lot of different possible paths through this code, and orders in which different use cases might call it. we can't reasonably take out the caching. but i suppose that also adding the additional return value doesn't hurt - at minimum, it's one less instance of error handling (which was your original concern).

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
Expand All @@ -165,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,
Expand Down Expand Up @@ -203,7 +168,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")
}
Expand All @@ -227,32 +192,50 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error {
return nil
}

func getDirectDependencies(sm gps.SourceManager, p *dep.Project) (pkgtree.PackageTree, map[string]bool, error) {
pkgT, err := p.ParseRootPackageTree()
// 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 pkgtree.PackageTree{}, nil, err
return nil, errors.Wrapf(err, "init failed: unable to detect the containing GOPATH")
}

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
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)
}

return pkgT, directDeps, nil
}
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)
}

// 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
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 errors.Wrap(err, "Solving failure")
return p, nil
}
40 changes: 0 additions & 40 deletions cmd/dep/init_test.go
View file
Open in desktop

This file was deleted.

10 changes: 5 additions & 5 deletions cmd/dep/root_analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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 {
Expand Down
7 changes: 4 additions & 3 deletions cmd/dep/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")}
Expand Down Expand Up @@ -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
}

Expand Down
Loading