Merge pull request #898 from carolynvs/root-imported-constraints

Remove duplicate root projects when importing from glide

cmd/dep/glide_importer.go

@@ -145,18 +145,16 @@ func (g *glideImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e
 		Constraints: make(gps.ProjectConstraints),
 	}
 
-	for _, pkg := range g.yaml.Imports {
+	for _, pkg := range append(g.yaml.Imports, g.yaml.TestImports...) {
 		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 {
-		pc, err := g.buildProjectConstraint(pkg)
-		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}
 	}
 
@@ -177,12 +175,16 @@ 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 {
-			lp := g.buildLockedProject(pkg, manifest)
+		for _, pkg := range append(g.lock.Imports, g.lock.TestImports...) {
+			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)
 		}
 	}
@@ -191,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
 	}
 
@@ -205,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)
@@ -231,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
 }

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

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

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 {

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

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

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"

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

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

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"
   ]