From 0687413431fb959625463c00ef6265ce4223c977 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 22 Dec 2016 15:24:58 -0500 Subject: [PATCH 1/4] rm: add -force, split disk and manifest concepts --- remove.go | 59 ++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/remove.go b/remove.go index 3a01c723dd..ee7d494673 100644 --- a/remove.go +++ b/remove.go @@ -42,6 +42,12 @@ Flags: `, } +var force bool + +func init() { + removeCmd.flag.Var(&force, "force", "Remove dependencies from manifest even if present in the current project's import") +} + func runRemove(args []string) error { p, err := depContext.loadProject("") if err != nil { @@ -64,13 +70,35 @@ func runRemove(args []string) error { return errors.Wrap(err, "gps.ListPackages") } - // get the list of packages - pd, err := getProjectData(pkgT, cpr, sm) - if err != nil { - return err + // TODO this will end up ignoring internal pkgs with errs (and any other + // internal pkgs that import them), which is not what we want for this mode. + // A new callback, or a new param on this one, will be introduced to gps + // soon, and we'll want to use that when it arrives. + //reachlist := pkgT.ExternalReach(true, true).ListExternalImports() + reachmap := pkgT.ExternalReach(true, true) + //var imap := make(map[string]bool, len(reachlist)) + //for _, im := range reachlist { + //imap[im] = true + //} + + // warm the cache, in case any paths require go get metadata discovery + for _, arg := range args { + go sm.DeduceProjectRoot(arg) } for _, arg := range args { + pr, err := sm.DeduceProjectRoot(arg) + if err != nil { + // couldn't detect the project root for this string - + // a non-valid project root was provided + return errors.Wrap(err, "gps.DeduceProjectRoot") + } + if string(pr) != arg { + // don't be magical with subpaths, otherwise we muddy the waters + // between project roots and import paths + return fmt.Errorf("%q is not a project root, but %q is - is that what you want to remove?", arg, pr) + } + /* * - Remove package from manifest * - if the package IS NOT being used, solving should do what we want @@ -78,11 +106,28 @@ func runRemove(args []string) error { * - Desired behavior: stop and tell the user, unless --force * - Actual solver behavior: ? */ + var pkgimport []string + for pkg, imports := range reachmap { + for _, im := range imports { + if hasImportPathPrefix(im, arg) { + pkgimport = append(pkgimport, pkg) + break + } + } + } - if _, found := pd.dependencies[gps.ProjectRoot(arg)]; found { - //TODO: Tell the user where it is in use? - return fmt.Errorf("not removing '%s' because it is in use", arg) + if _, indeps := p.m.Dependencies[gps.ProjectRoot(arg)]; !indeps { + return fmt.Errorf("%q is not present in the manifest, cannot remove it", arg) } + + if len(pkgimport) > 0 && !force { + if len(pkgimport) == 1 { + return fmt.Errorf("not removing %q because it is imported by %q (pass -force to override)", arg, pkgimport[0]) + } else { + return fmt.Errorf("not removing %q because it is imported by:\n\t%s (pass -force to override)", arg, strings.Join(pkgimport, "\n\t")) + } + } + delete(p.m.Dependencies, gps.ProjectRoot(arg)) } From 53ae39458150d9a406a0699f9114d5644512e8f1 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 22 Dec 2016 17:00:31 -0500 Subject: [PATCH 2/4] rm: add -unused, fix errs --- remove.go | 134 ++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 89 insertions(+), 45 deletions(-) diff --git a/remove.go b/remove.go index ee7d494673..8ec8430cfa 100644 --- a/remove.go +++ b/remove.go @@ -5,10 +5,12 @@ package main import ( + "flag" "fmt" "log" "os" "path/filepath" + "strings" "github.com/pkg/errors" "github.com/sdboyer/gps" @@ -17,6 +19,7 @@ import ( var removeCmd = &command{ fn: runRemove, name: "rm", + flag: flag.NewFlagSet("", flag.ExitOnError), short: `[flags] [packages] Remove a package or a set of packages. `, @@ -42,10 +45,11 @@ Flags: `, } -var force bool +var unused, force bool func init() { - removeCmd.flag.Var(&force, "force", "Remove dependencies from manifest even if present in the current project's import") + removeCmd.flag.BoolVar(&force, "force", false, "Remove dependencies from manifest even if present in the current project's import") + removeCmd.flag.BoolVar(&unused, "unused", false, "Purge all dependencies from manifest that are no longer imported by the project") } func runRemove(args []string) error { @@ -75,60 +79,100 @@ func runRemove(args []string) error { // A new callback, or a new param on this one, will be introduced to gps // soon, and we'll want to use that when it arrives. //reachlist := pkgT.ExternalReach(true, true).ListExternalImports() - reachmap := pkgT.ExternalReach(true, true) - //var imap := make(map[string]bool, len(reachlist)) - //for _, im := range reachlist { - //imap[im] = true - //} - - // warm the cache, in case any paths require go get metadata discovery - for _, arg := range args { - go sm.DeduceProjectRoot(arg) - } + reachmap := pkgT.ExternalReach(true, true, nil) - for _, arg := range args { - pr, err := sm.DeduceProjectRoot(arg) - if err != nil { - // couldn't detect the project root for this string - - // a non-valid project root was provided - return errors.Wrap(err, "gps.DeduceProjectRoot") + if unused { + if len(args) > 0 { + return fmt.Errorf("rm takes no arguments when running with -unused") } - if string(pr) != arg { - // don't be magical with subpaths, otherwise we muddy the waters - // between project roots and import paths - return fmt.Errorf("%q is not a project root, but %q is - is that what you want to remove?", arg, pr) + + reachlist := reachmap.ListExternalImports() + + // warm the cache in parallel, in case any paths require go get metadata + // discovery + for _, im := range reachlist { + go sm.DeduceProjectRoot(im) } - /* - * - Remove package from manifest - * - if the package IS NOT being used, solving should do what we want - * - if the package IS being used: - * - Desired behavior: stop and tell the user, unless --force - * - Actual solver behavior: ? - */ - var pkgimport []string - for pkg, imports := range reachmap { - for _, im := range imports { - if hasImportPathPrefix(im, arg) { - pkgimport = append(pkgimport, pkg) - break - } + otherroots := make(map[gps.ProjectRoot]bool) + for _, im := range reachlist { + if isStdLib(im) { + continue + } + pr, err := sm.DeduceProjectRoot(im) + if err != nil { + // not being able to detect the root for an import path that's + // actually in the import list is a deeper problem. However, + // it's not our direct concern here, so we just warn. + logf("could not infer root for %q", pr) + continue } + otherroots[pr] = true } - if _, indeps := p.m.Dependencies[gps.ProjectRoot(arg)]; !indeps { - return fmt.Errorf("%q is not present in the manifest, cannot remove it", arg) + var rm []gps.ProjectRoot + for pr, _ := range p.m.Dependencies { + if _, has := otherroots[pr]; !has { + delete(p.m.Dependencies, pr) + rm = append(rm, pr) + } } - if len(pkgimport) > 0 && !force { - if len(pkgimport) == 1 { - return fmt.Errorf("not removing %q because it is imported by %q (pass -force to override)", arg, pkgimport[0]) - } else { - return fmt.Errorf("not removing %q because it is imported by:\n\t%s (pass -force to override)", arg, strings.Join(pkgimport, "\n\t")) - } + if len(rm) == 0 { + logf("nothing to do") + return nil + } + } else { + // warm the cache in parallel, in case any paths require go get metadata + // discovery + for _, arg := range args { + go sm.DeduceProjectRoot(arg) } - delete(p.m.Dependencies, gps.ProjectRoot(arg)) + for _, arg := range args { + pr, err := sm.DeduceProjectRoot(arg) + if err != nil { + // couldn't detect the project root for this string - + // a non-valid project root was provided + return errors.Wrap(err, "gps.DeduceProjectRoot") + } + if string(pr) != arg { + // don't be magical with subpaths, otherwise we muddy the waters + // between project roots and import paths + return fmt.Errorf("%q is not a project root, but %q is - is that what you want to remove?", arg, pr) + } + + /* + * - Remove package from manifest + * - if the package IS NOT being used, solving should do what we want + * - if the package IS being used: + * - Desired behavior: stop and tell the user, unless --force + * - Actual solver behavior: ? + */ + var pkgimport []string + for pkg, imports := range reachmap { + for _, im := range imports { + if hasImportPathPrefix(im, arg) { + pkgimport = append(pkgimport, pkg) + break + } + } + } + + if _, indeps := p.m.Dependencies[gps.ProjectRoot(arg)]; !indeps { + return fmt.Errorf("%q is not present in the manifest, cannot remove it", arg) + } + + if len(pkgimport) > 0 && !force { + if len(pkgimport) == 1 { + return fmt.Errorf("not removing %q because it is imported by %q (pass -force to override)", arg, pkgimport[0]) + } else { + return fmt.Errorf("not removing %q because it is imported by:\n\t%s (pass -force to override)", arg, strings.Join(pkgimport, "\n\t")) + } + } + + delete(p.m.Dependencies, gps.ProjectRoot(arg)) + } } params := gps.SolveParameters{ From d5cc65aebabc20a6307b5007b9bd4da0051fd8aa Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 22 Dec 2016 23:35:31 -0500 Subject: [PATCH 3/4] rm: add basic tests --- remove_test.go | 158 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 remove_test.go diff --git a/remove_test.go b/remove_test.go new file mode 100644 index 0000000000..8da55c9189 --- /dev/null +++ b/remove_test.go @@ -0,0 +1,158 @@ +// 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 "testing" + +func TestRemove(t *testing.T) { + needsExternalNetwork(t) + needsGit(t) + + tg := testgo(t) + defer tg.cleanup() + + tg.tempDir("src") + tg.setenv("GOPATH", tg.path(".")) + + importPaths := map[string]string{ + "github.com/pkg/errors": "v0.8.0", // semver + "github.com/Sirupsen/logrus": "42b84f9ec624953ecbf81a94feccb3f5935c5edf", // random sha + } + + // checkout the specified revisions + for ip, rev := range importPaths { + tg.runGo("get", ip) + repoDir := tg.path("src/" + ip) + tg.runGit(repoDir, "checkout", rev) + } + + // Build a fake consumer of these packages. + const root = "github.com/golang/notexist" + m := `package main + +import ( + "github.com/Sirupsen/logrus" + "github.com/pkg/errors" +) + +func main() { + err := nil + if err != nil { + errors.Wrap(err, "thing") + } + logrus.Info("whatev") +}` + + tg.tempFile("src/"+root+"/thing.go", m) + origm := `{ + "dependencies": { + "github.com/not/used": { + "version": "2.0.0" + }, + "github.com/Sirupsen/logrus": { + "revision": "42b84f9ec624953ecbf81a94feccb3f5935c5edf" + }, + "github.com/pkg/errors": { + "version": ">=0.8.0, <1.0.0" + } + } +} +` + expectedManifest := `{ + "dependencies": { + "github.com/Sirupsen/logrus": { + "revision": "42b84f9ec624953ecbf81a94feccb3f5935c5edf" + }, + "github.com/pkg/errors": { + "version": ">=0.8.0, <1.0.0" + } + } +} +` + + tg.tempFile("src/"+root+"/manifest.json", origm) + + tg.cd(tg.path("src/" + root)) + tg.run("rm", "-unused") + + manifest := tg.readManifest() + if manifest != expectedManifest { + t.Fatalf("expected %s, got %s", expectedManifest, manifest) + } + + tg.tempFile("src/"+root+"/manifest.json", origm) + tg.run("rm", "github.com/not/used") + + manifest = tg.readManifest() + if manifest != expectedManifest { + t.Fatalf("expected %s, got %s", expectedManifest, manifest) + } + + if err := tg.doRun([]string{"rm", "-unused", "github.com/not/used"}); err == nil { + t.Fatal("rm with both -unused and arg should have failed") + } + + if err := tg.doRun([]string{"rm", "github.com/not/present"}); err == nil { + t.Fatal("rm with arg not in manifest should have failed") + } + + if err := tg.doRun([]string{"rm", "github.com/not/used", "github.com/not/present"}); err == nil { + t.Fatal("rm with one arg not in manifest should have failed") + } + + if err := tg.doRun([]string{"rm", "github.com/pkg/errors"}); err == nil { + t.Fatal("rm of arg in manifest and imports should have failed without -force") + } + + tg.tempFile("src/"+root+"/manifest.json", origm) + tg.run("rm", "-force", "github.com/pkg/errors", "github.com/not/used") + + manifest = tg.readManifest() + if manifest != `{ + "dependencies": { + "github.com/Sirupsen/logrus": { + "revision": "42b84f9ec624953ecbf81a94feccb3f5935c5edf" + } + } +} +` { + t.Fatalf("expected %s, got %s", expectedManifest, manifest) + } + + sysCommit, err := getRepoLatestCommit("golang/sys") + tg.must(err) + expectedLock := `{ + "projects": [ + { + "name": "github.com/Sirupsen/logrus", + "revision": "42b84f9ec624953ecbf81a94feccb3f5935c5edf", + "packages": [ + "." + ] + }, + { + "name": "github.com/pkg/errors", + "version": "v0.8.0", + "revision": "645ef00459ed84a119197bfb8d8205042c6df63d", + "packages": [ + "." + ] + }, + { + "name": "golang.org/x/sys", + "branch": "master", + "revision": "` + sysCommit + `", + "packages": [ + "unix" + ] + } + ] +} +` + lock := wipeMemo(tg.readLock()) + if lock != expectedLock { + t.Fatalf("expected %s, got %s", expectedLock, lock) + } +} From e73be2243e3d0c3907adcd100d8a067b1b1ffd82 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 22 Dec 2016 23:49:30 -0500 Subject: [PATCH 4/4] dep: construct SolveParameters safely SolveParameters' manifest and lock have to be untyped nil if they're empty - otherwise gps will end up calling methods on them anyway. --- ensure.go | 11 +++++------ main.go | 18 ++++++++++++++++++ remove.go | 8 ++------ 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/ensure.go b/ensure.go index 894b107466..96323d3811 100644 --- a/ensure.go +++ b/ensure.go @@ -163,12 +163,11 @@ func runEnsure(args []string) error { return errors.New(buf.String()) } - params := gps.SolveParameters{ - RootDir: p.absroot, - Manifest: p.m, - Lock: p.l, - Trace: true, - TraceLogger: log.New(os.Stdout, "", 0), + params := p.makeParams() + + if *verbose { + params.Trace = true + params.TraceLogger = log.New(os.Stderr, "", 0) } params.RootPackageTree, err = gps.ListPackages(p.absroot, string(p.importroot)) diff --git a/main.go b/main.go index 252aea243d..c3efced804 100644 --- a/main.go +++ b/main.go @@ -185,6 +185,24 @@ type project struct { l *lock } +// makeParams is a simple helper to create a gps.SolveParameters without setting +// any nils incorrectly. +func (p *project) makeParams() gps.SolveParameters { + params := gps.SolveParameters{ + RootDir: p.absroot, + } + + if p.m != nil { + params.Manifest = p.m + } + + if p.l != nil { + params.Lock = p.l + } + + return params +} + func logf(format string, args ...interface{}) { // TODO: something else? fmt.Fprintf(os.Stderr, "dep: "+format+"\n", args...) diff --git a/remove.go b/remove.go index 8ec8430cfa..b2fa4feb7f 100644 --- a/remove.go +++ b/remove.go @@ -175,12 +175,8 @@ func runRemove(args []string) error { } } - params := gps.SolveParameters{ - RootDir: p.absroot, - RootPackageTree: pkgT, - Manifest: p.m, - Lock: p.l, - } + params := p.makeParams() + params.RootPackageTree = pkgT if *verbose { params.Trace = true