From cdff6e6df91272d647e698a58b46764735885158 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Fri, 21 Jul 2017 15:52:59 -0500 Subject: [PATCH 1/4] Make projectExistsInLock usable for all importers --- cmd/dep/godep_importer.go | 14 +----------- cmd/dep/godep_importer_test.go | 30 ------------------------- cmd/dep/root_analyzer.go | 12 ++++++++++ cmd/dep/root_analyzer_test.go | 40 ++++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 43 deletions(-) diff --git a/cmd/dep/godep_importer.go b/cmd/dep/godep_importer.go index 3deb2da1b5..937fd2c0da 100644 --- a/cmd/dep/godep_importer.go +++ b/cmd/dep/godep_importer.go @@ -108,7 +108,7 @@ func (g *godepImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e pkg.ImportPath = string(ip) // Check if it already existing in locked projects - if projectExistsInLock(lock, pkg.ImportPath) { + if projectExistsInLock(lock, ip) { continue } @@ -187,15 +187,3 @@ func (g *godepImporter) buildLockedProject(pkg godepPackage, manifest *dep.Manif return lp } - -// projectExistsInLock checks if the given import path already existing in -// locked projects. -func projectExistsInLock(l *dep.Lock, ip string) bool { - for _, lp := range l.P { - if ip == string(lp.Ident().ProjectRoot) { - return true - } - } - - return false -} diff --git a/cmd/dep/godep_importer_test.go b/cmd/dep/godep_importer_test.go index 5b83b05325..874455f22f 100644 --- a/cmd/dep/godep_importer_test.go +++ b/cmd/dep/godep_importer_test.go @@ -10,7 +10,6 @@ import ( "path/filepath" "testing" - "github.com/golang/dep" "github.com/golang/dep/internal/gps" "github.com/golang/dep/internal/test" "github.com/pkg/errors" @@ -277,35 +276,6 @@ func TestGodepConfig_JsonLoad(t *testing.T) { } } -func TestGodepConfig_ProjectExistsInLock(t *testing.T) { - lock := &dep.Lock{} - pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/sdboyer/deptest")} - ver := gps.NewVersion("v1.0.0") - lock.P = append(lock.P, gps.NewLockedProject(pi, ver, nil)) - - cases := []struct { - importPath string - want bool - }{ - { - importPath: "github.com/sdboyer/deptest", - want: true, - }, - { - importPath: "github.com/golang/notexist", - want: false, - }, - } - - for _, c := range cases { - result := projectExistsInLock(lock, c.importPath) - - if result != c.want { - t.Fatalf("projectExistsInLock result is not as want: \n\t(GOT) %v \n\t(WNT) %v", result, c.want) - } - } -} - // equalImports compares two slices of godepPackage and checks if they are // equal. func equalImports(a, b []godepPackage) bool { diff --git a/cmd/dep/root_analyzer.go b/cmd/dep/root_analyzer.go index 9f93b7eb22..2ef6f94cb5 100644 --- a/cmd/dep/root_analyzer.go +++ b/cmd/dep/root_analyzer.go @@ -211,3 +211,15 @@ func lookupVersionForLockedProject(pi gps.ProjectIdentifier, c gps.Constraint, r // Give up and lock only to a revision return rev, nil } + +// projectExistsInLock checks if the given project already exists in +// a lockfile. +func projectExistsInLock(l *dep.Lock, pr gps.ProjectRoot) bool { + for _, lp := range l.P { + if pr == lp.Ident().ProjectRoot { + return true + } + } + + return false +} diff --git a/cmd/dep/root_analyzer_test.go b/cmd/dep/root_analyzer_test.go index a2c5a85b84..e06be921ca 100644 --- a/cmd/dep/root_analyzer_test.go +++ b/cmd/dep/root_analyzer_test.go @@ -7,6 +7,7 @@ package main import ( "testing" + "github.com/golang/dep" "github.com/golang/dep/internal/gps" "github.com/golang/dep/internal/test" ) @@ -111,3 +112,42 @@ func TestLookupVersionForLockedProject_FallbackToRevision(t *testing.T) { t.Fatalf("Expected the locked version to be the manifest's pinned revision: wanted '%s', got '%s'", wantV, gotV) } } + +func TestProjectExistsInLock(t *testing.T) { + lock := &dep.Lock{} + pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/sdboyer/deptest")} + ver := gps.NewVersion("v1.0.0") + lock.P = append(lock.P, gps.NewLockedProject(pi, ver, nil)) + + cases := []struct { + name string + importPath string + want bool + }{ + { + name: "root project", + importPath: "github.com/sdboyer/deptest", + want: true, + }, + { + name: "sub package", + importPath: "github.com/sdboyer/deptest/foo", + want: false, + }, + { + name: "nonexisting project", + importPath: "github.com/golang/notexist", + want: false, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + result := projectExistsInLock(lock, gps.ProjectRoot(c.importPath)) + + if result != c.want { + t.Fatalf("projectExistsInLock result is not as want: \n\t(GOT) %v \n\t(WNT) %v", result, c.want) + } + }) + } +} From 65bfe46aaf2971f6657cb6b11fbdc4f592dbdd33 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Thu, 20 Jul 2017 14:43:02 -0500 Subject: [PATCH 2/4] Combine logic for regular and test deps in glide --- cmd/dep/glide_importer.go | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/cmd/dep/glide_importer.go b/cmd/dep/glide_importer.go index 6bb34eb8fe..3bccd2781a 100644 --- a/cmd/dep/glide_importer.go +++ b/cmd/dep/glide_importer.go @@ -145,14 +145,7 @@ func (g *glideImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e Constraints: make(gps.ProjectConstraints), } - for _, pkg := range g.yaml.Imports { - pc, err := g.buildProjectConstraint(pkg) - if err != nil { - return nil, nil, err - } - manifest.Constraints[pc.Ident.ProjectRoot] = gps.ProjectProperties{Source: pc.Ident.Source, Constraint: pc.Constraint} - } - for _, pkg := range g.yaml.TestImports { + for _, pkg := range append(g.yaml.Imports, g.yaml.TestImports...) { pc, err := g.buildProjectConstraint(pkg) if err != nil { return nil, nil, err @@ -177,11 +170,7 @@ func (g *glideImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e if g.lock != nil { lock = &dep.Lock{} - for _, pkg := range g.lock.Imports { - lp := g.buildLockedProject(pkg, manifest) - lock.P = append(lock.P, lp) - } - for _, pkg := range g.lock.TestImports { + for _, pkg := range append(g.lock.Imports, g.lock.TestImports...) { lp := g.buildLockedProject(pkg, manifest) lock.P = append(lock.P, lp) } From a61cc1b49b70652c8f80377ba7d75a28f73110a9 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Tue, 25 Jul 2017 10:58:35 -0500 Subject: [PATCH 3/4] Skip duplicate project roots during glide import --- cmd/dep/glide_importer.go | 47 ++++++++++++++++++++++++---------- cmd/dep/glide_importer_test.go | 47 ++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 13 deletions(-) diff --git a/cmd/dep/glide_importer.go b/cmd/dep/glide_importer.go index 3bccd2781a..b343e0c794 100644 --- a/cmd/dep/glide_importer.go +++ b/cmd/dep/glide_importer.go @@ -150,6 +150,11 @@ func (g *glideImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e if err != nil { return nil, nil, err } + + if _, isDup := manifest.Constraints[pc.Ident.ProjectRoot]; isDup { + continue + } + manifest.Constraints[pc.Ident.ProjectRoot] = gps.ProjectProperties{Source: pc.Ident.Source, Constraint: pc.Constraint} } @@ -171,7 +176,15 @@ func (g *glideImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e lock = &dep.Lock{} for _, pkg := range append(g.lock.Imports, g.lock.TestImports...) { - lp := g.buildLockedProject(pkg, manifest) + lp, err := g.buildLockedProject(pkg, manifest) + if err != nil { + return nil, nil, err + } + + if projectExistsInLock(lock, lp.Ident().ProjectRoot) { + continue + } + lock.P = append(lock.P, lp) } } @@ -180,8 +193,17 @@ func (g *glideImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e } func (g *glideImporter) buildProjectConstraint(pkg glidePackage) (pc gps.ProjectConstraint, err error) { - if pkg.Name == "" { - err = errors.New("Invalid glide configuration, package name is required") + pr, err := g.sm.DeduceProjectRoot(pkg.Name) + if err != nil { + return + } + + pc.Ident = gps.ProjectIdentifier{ + ProjectRoot: pr, + Source: pkg.Repository, + } + pc.Constraint, err = g.sm.InferConstraint(pkg.Reference, pc.Ident) + if err != nil { return } @@ -194,21 +216,20 @@ func (g *glideImporter) buildProjectConstraint(pkg glidePackage) (pc gps.Project } } - pc.Ident = gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot(pkg.Name), Source: pkg.Repository} - pc.Constraint, err = g.sm.InferConstraint(pkg.Reference, pc.Ident) - if err != nil { - return - } - f := fb.NewConstraintFeedback(pc, fb.DepTypeImported) f.LogFeedback(g.logger) return } -func (g *glideImporter) buildLockedProject(pkg glideLockedPackage, manifest *dep.Manifest) gps.LockedProject { +func (g *glideImporter) buildLockedProject(pkg glideLockedPackage, manifest *dep.Manifest) (lp gps.LockedProject, err error) { + pr, err := g.sm.DeduceProjectRoot(pkg.Name) + if err != nil { + return + } + pi := gps.ProjectIdentifier{ - ProjectRoot: gps.ProjectRoot(pkg.Name), + ProjectRoot: pr, Source: pkg.Repository, } revision := gps.Revision(pkg.Reference) @@ -220,10 +241,10 @@ func (g *glideImporter) buildLockedProject(pkg glideLockedPackage, manifest *dep g.logger.Println(err.Error()) } - lp := gps.NewLockedProject(pi, version, nil) + lp = gps.NewLockedProject(pi, version, nil) f := fb.NewLockedProjectFeedback(lp, fb.DepTypeImported) f.LogFeedback(g.logger) - return lp + return } diff --git a/cmd/dep/glide_importer_test.go b/cmd/dep/glide_importer_test.go index 630b7814b7..ce624234bf 100644 --- a/cmd/dep/glide_importer_test.go +++ b/cmd/dep/glide_importer_test.go @@ -375,3 +375,50 @@ func equalSlice(a, b []string) bool { return true } + +func TestGlideConfig_Convert_ConsolidateRootPackages(t *testing.T) { + h := test.NewHelper(t) + defer h.Cleanup() + + ctx := newTestContext(h) + sm, err := ctx.SourceManager() + h.Must(err) + defer sm.Release() + + g := newGlideImporter(ctx.Err, true, sm) + g.yaml = glideYaml{ + Imports: []glidePackage{ + {Name: "github.com/carolynvs/deptest-subpkg/subby"}, + {Name: "github.com/carolynvs/deptest-subpkg"}, + }, + } + g.lock = &glideLock{ + Imports: []glideLockedPackage{ + {Name: "github.com/carolynvs/deptest-subpkg/subby"}, + {Name: "github.com/carolynvs/deptest-subpkg"}, + }, + } + + manifest, lock, err := g.convert(testGlideProjectRoot) + h.Must(err) + + gotMLen := len(manifest.Constraints) + if gotMLen != 1 { + t.Fatalf("Expected manifest to contain 1 constraint, got %d", gotMLen) + } + + wantRoot := gps.ProjectRoot("github.com/carolynvs/deptest-subpkg") + if _, has := manifest.Constraints[wantRoot]; !has { + t.Fatalf("Expected manifest to contain a constraint for %s, got %v", wantRoot, manifest.Constraints) + } + + gotLLen := len(lock.P) + if gotLLen != 1 { + t.Fatalf("Expected lock to contain 1 project, got %d", gotLLen) + } + + gotRoot := lock.P[0].Ident().ProjectRoot + if gotRoot != wantRoot { + t.Fatalf("Expected lock to contain a project for %s, got %v", wantRoot, gotRoot) + } +} From 3dbbfbd3bdd9e0e8a6fe0b4f909a605017e63346 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Tue, 25 Jul 2017 19:21:44 -0500 Subject: [PATCH 4/4] Add GlideImporter testdata for empty constraints --- .../harness_tests/init/glide/case1/final/Gopkg.lock | 7 ++++++- .../harness_tests/init/glide/case1/final/Gopkg.toml | 3 +++ .../harness_tests/init/glide/case1/initial/glide.lock | 2 ++ .../harness_tests/init/glide/case1/initial/glide.yaml | 1 + .../harness_tests/init/glide/case1/initial/main.go | 1 + .../testdata/harness_tests/init/glide/case1/testcase.json | 1 + 6 files changed, 14 insertions(+), 1 deletion(-) diff --git a/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.lock b/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.lock index 56f9bb2b7a..74aad6763b 100644 --- a/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.lock +++ b/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.lock @@ -1,6 +1,11 @@ # This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'. +[[projects]] + name = "github.com/carolynvs/deptest-subpkg" + packages = ["subby"] + revision = "6c41d90f78bb1015696a2ad591debfa8971512d5" + [[projects]] name = "github.com/sdboyer/deptest" packages = ["."] @@ -16,6 +21,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "c53803413bd0160505cce903e1cba743e0b964088f8cc42a6123f6fe1a0ae9d3" + inputs-digest = "7efcfca7f138c3579d22b4ef788294649c734ea630124fb8fbb47acf8770b086" solver-name = "gps-cdcl" solver-version = 1 diff --git a/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.toml b/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.toml index ab7893d76e..f9d63b74dc 100644 --- a/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.toml +++ b/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.toml @@ -1,5 +1,8 @@ ignored = ["github.com/sdboyer/dep-test","github.com/golang/notexist/samples"] +[[constraint]] + name = "github.com/carolynvs/deptest-subpkg" + [[constraint]] name = "github.com/sdboyer/deptestdos" version = "2.0.0" diff --git a/cmd/dep/testdata/harness_tests/init/glide/case1/initial/glide.lock b/cmd/dep/testdata/harness_tests/init/glide/case1/initial/glide.lock index 1d29509927..7a36a4a039 100644 --- a/cmd/dep/testdata/harness_tests/init/glide/case1/initial/glide.lock +++ b/cmd/dep/testdata/harness_tests/init/glide/case1/initial/glide.lock @@ -7,6 +7,8 @@ imports: version: ff2948a2ac8f538c4ecd55962e919d1e13e74baf - name: github.com/sdboyer/deptestdos version: 5c607206be5decd28e6263ffffdcee067266015e +- name: github.com/carolynvs/deptest-subpkg/subby + version: 6c41d90f78bb1015696a2ad591debfa8971512d5 testImports: - name: github.com/golang/lint version: cb00e5669539f047b2f4c53a421a01b0c8e172c6 diff --git a/cmd/dep/testdata/harness_tests/init/glide/case1/initial/glide.yaml b/cmd/dep/testdata/harness_tests/init/glide/case1/initial/glide.yaml index ee269a398d..64b8b6b620 100644 --- a/cmd/dep/testdata/harness_tests/init/glide/case1/initial/glide.yaml +++ b/cmd/dep/testdata/harness_tests/init/glide/case1/initial/glide.yaml @@ -16,5 +16,6 @@ import: version: v1.0.0 - package: github.com/sdboyer/deptestdos version: v2.0.0 +- package: github.com/carolynvs/deptest-subpkg/subby testImport: - package: github.com/golang/lint diff --git a/cmd/dep/testdata/harness_tests/init/glide/case1/initial/main.go b/cmd/dep/testdata/harness_tests/init/glide/case1/initial/main.go index 2b2c7c396e..5238b24923 100644 --- a/cmd/dep/testdata/harness_tests/init/glide/case1/initial/main.go +++ b/cmd/dep/testdata/harness_tests/init/glide/case1/initial/main.go @@ -7,6 +7,7 @@ package main import ( "fmt" + _ "github.com/carolynvs/deptest-subpkg/subby" "github.com/sdboyer/deptestdos" ) diff --git a/cmd/dep/testdata/harness_tests/init/glide/case1/testcase.json b/cmd/dep/testdata/harness_tests/init/glide/case1/testcase.json index e119765489..cf0d79080e 100644 --- a/cmd/dep/testdata/harness_tests/init/glide/case1/testcase.json +++ b/cmd/dep/testdata/harness_tests/init/glide/case1/testcase.json @@ -8,6 +8,7 @@ "github.com/sdboyer/deptestdos": "5c607206be5decd28e6263ffffdcee067266015e" }, "vendor-final": [ + "github.com/carolynvs/deptest-subpkg", "github.com/sdboyer/deptest", "github.com/sdboyer/deptestdos" ]