From 7dc5eadaa6fce0d4895346b1c6ba65c4e724b976 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 22 Dec 2016 23:35:31 -0500 Subject: [PATCH 1/3] 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 970f5837fb3b1166a146b26f6d4fee44c8b670c4 Mon Sep 17 00:00:00 2001 From: Jess Frazelle Date: Wed, 4 Jan 2017 16:39:21 -0800 Subject: [PATCH 2/3] change rm to remove in tests Signed-off-by: Jess Frazelle --- remove_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/remove_test.go b/remove_test.go index 8da55c9189..72142eb426 100644 --- a/remove_test.go +++ b/remove_test.go @@ -75,7 +75,7 @@ func main() { tg.tempFile("src/"+root+"/manifest.json", origm) tg.cd(tg.path("src/" + root)) - tg.run("rm", "-unused") + tg.run("remove", "-unused") manifest := tg.readManifest() if manifest != expectedManifest { @@ -83,31 +83,31 @@ func main() { } tg.tempFile("src/"+root+"/manifest.json", origm) - tg.run("rm", "github.com/not/used") + tg.run("remove", "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 { + if err := tg.doRun([]string{"remove", "-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 { + if err := tg.doRun([]string{"remove", "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 { + if err := tg.doRun([]string{"remove", "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 { + if err := tg.doRun([]string{"remove", "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") + tg.run("remove", "-force", "github.com/pkg/errors", "github.com/not/used") manifest = tg.readManifest() if manifest != `{ From af7fcaf85646bea6053c81c17eac046145c77dda Mon Sep 17 00:00:00 2001 From: Jess Frazelle Date: Wed, 4 Jan 2017 16:53:45 -0800 Subject: [PATCH 3/3] apply @sdboyer changes on peters Signed-off-by: Jess Frazelle --- dep_test.go | 3 +- ensure.go | 6 +-- main.go | 18 ++++++++ remove.go | 124 ++++++++++++++++++++++++++++++++++++++++++---------- 4 files changed, 121 insertions(+), 30 deletions(-) diff --git a/dep_test.go b/dep_test.go index d3b5bf7363..5b3275e296 100644 --- a/dep_test.go +++ b/dep_test.go @@ -171,7 +171,8 @@ func (tg *testgoData) doRun(args []string) error { } else { prog = filepath.Join(tg.wd, "testdep"+exeSuffix) } - cmd := exec.Command(prog, append(args, "-v")...) + args = append(args[:1], append([]string{"-v"}, args[1:]...)...) + cmd := exec.Command(prog, args...) tg.stdout.Reset() tg.stderr.Reset() cmd.Stdout = &tg.stdout diff --git a/ensure.go b/ensure.go index c3010e11bc..5fda801d54 100644 --- a/ensure.go +++ b/ensure.go @@ -176,11 +176,7 @@ func (cmd *ensureCommand) Run(args []string) error { return errors.New(buf.String()) } - params := gps.SolveParameters{ - RootDir: p.absroot, - Manifest: p.m, - Lock: p.l, - } + params := p.makeParams() if *verbose { params.Trace = true params.TraceLogger = log.New(os.Stderr, "", 0) diff --git a/main.go b/main.go index f04b3cf723..36de58b2df 100644 --- a/main.go +++ b/main.go @@ -181,6 +181,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 b006e80004..b5e2dc0e6d 100644 --- a/remove.go +++ b/remove.go @@ -9,6 +9,7 @@ import ( "fmt" "log" "os" + "strings" "github.com/pkg/errors" "github.com/sdboyer/gps" @@ -62,35 +63,110 @@ func (cmd *removeCommand) Run(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, nil) - for _, arg := range args { - /* - * - 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: ? - */ - - 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 cmd.unused { + if len(args) > 0 { + return fmt.Errorf("remove takes no arguments when running with -unused") + } + + 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) + } + + 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 + } + + 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(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)) - } - params := gps.SolveParameters{ - RootDir: p.absroot, - RootPackageTree: pkgT, - Manifest: p.m, - Lock: p.l, + 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 && !cmd.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 := p.makeParams() + params.RootPackageTree = pkgT + if *verbose { params.Trace = true params.TraceLogger = log.New(os.Stderr, "", 0)