ref(ensure): make ensure -add concurrent

This change makes `ensure -add` concurrently add the dependencies and
uses golang.org/x/sync/syncmap for a concurrent map to replace
addInstructions map.

Also, logs a message when the sources are being fetched.

Author: darkowlzz

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 IMPROVEMENTS:
 
+* `dep ensure -add` now concurrently fetches the source and adds the projects.
+(#1218)
 * File name case check is now performed on `Gopkg.toml` and `Gopkg.lock`.
 (#1114)
 * gps: gps now supports pruning. (#1020)

cmd/dep/ensure.go

@@ -22,6 +22,7 @@ import (
 	"github.com/golang/dep/internal/gps/paths"
 	"github.com/golang/dep/internal/gps/pkgtree"
 	"github.com/pkg/errors"
+	"golang.org/x/sync/syncmap"
 )
 
 const ensureShortHelp = `Ensure a dependency is safely vendored in the project`
@@ -112,7 +113,10 @@ dep ensure -update -no-vendor
 
 `
 
-var errUpdateArgsValidation = errors.New("update arguments validation failed")
+var (
+	errUpdateArgsValidation = errors.New("update arguments validation failed")
+	errAddDepsFailed        = errors.New("adding dependencies failed")
+)
 
 func (cmd *ensureCommand) Name() string { return "ensure" }
 func (cmd *ensureCommand) Args() string {
@@ -466,93 +470,136 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm
 		constraint gps.Constraint
 		typ        addType
 	}
-	addInstructions := make(map[gps.ProjectRoot]addInstruction)
 
-	for _, arg := range args {
-		// TODO(sdboyer) return all errors, not just the first one we encounter
-		// TODO(sdboyer) do these concurrently?
-		pc, path, err := getProjectConstraint(arg, sm)
-		if err != nil {
-			// TODO(sdboyer) ensure these errors are contextualized in a sensible way for -add
-			return err
-		}
+	// Create a syncmap to store all the addInstruction(s) concurrently.
+	addInstructions := new(syncmap.Map)
 
-		// check if the the parsed path is the current root path
-		if strings.EqualFold(string(p.ImportRoot), string(pc.Ident.ProjectRoot)) {
-			return errors.New("cannot add current project to itself")
-		}
+	// Channel for receiving all the errors.
+	errCh := make(chan error, len(args))
 
-		inManifest := p.Manifest.HasConstraintsOn(pc.Ident.ProjectRoot)
-		inImports := exrmap[pc.Ident.ProjectRoot]
-		if inManifest && inImports {
-			return errors.Errorf("nothing to -add, %s is already in %s and the project's direct imports or required list", pc.Ident.ProjectRoot, dep.ManifestName)
-		}
+	var wg sync.WaitGroup
 
-		err = sm.SyncSourceFor(pc.Ident)
-		if err != nil {
-			return errors.Wrapf(err, "failed to fetch source for %s", pc.Ident.ProjectRoot)
+	fmt.Println("Fetching sources...")
+
+	for i, arg := range args {
+		wg.Add(1)
+
+		if ctx.Verbose {
+			ctx.Err.Printf("(%d/%d) %s\n", i+1, len(args), arg)
 		}
 
-		someConstraint := !gps.IsAny(pc.Constraint) || pc.Ident.Source != ""
+		go func(arg string) {
+			defer wg.Done()
+
+			pc, path, err := getProjectConstraint(arg, sm)
+			if err != nil {
+				// TODO(sdboyer) ensure these errors are contextualized in a sensible way for -add
+				errCh <- err
+				return
+			}
+
+			// check if the the parsed path is the current root path
+			if strings.EqualFold(string(p.ImportRoot), string(pc.Ident.ProjectRoot)) {
+				errCh <- errors.New("cannot add current project to itself")
+				return
+			}
+
+			inManifest := p.Manifest.HasConstraintsOn(pc.Ident.ProjectRoot)
+			inImports := exrmap[pc.Ident.ProjectRoot]
+			if inManifest && inImports {
+				errCh <- errors.Errorf("nothing to -add, %s is already in %s and the project's direct imports or required list", pc.Ident.ProjectRoot, dep.ManifestName)
+				return
+			}
+
+			err = sm.SyncSourceFor(pc.Ident)
+			if err != nil {
+				errCh <- errors.Wrapf(err, "failed to fetch source for %s", pc.Ident.ProjectRoot)
+				return
+			}
 
-		instr, has := addInstructions[pc.Ident.ProjectRoot]
-		if has {
-			// Multiple packages from the same project were specified as
-			// arguments; make sure they agree on declared constraints.
-			// TODO(sdboyer) until we have a general method for checking constraint equality, only allow one to declare
-			if someConstraint {
-				if !gps.IsAny(instr.constraint) || instr.id.Source != "" {
-					return errors.Errorf("can only specify rules once per project being added; rules were given at least twice for %s", pc.Ident.ProjectRoot)
+			someConstraint := !gps.IsAny(pc.Constraint) || pc.Ident.Source != ""
+
+			instrInterface, has := addInstructions.LoadOrStore(pc.Ident.ProjectRoot, addInstruction{})
+			instr := instrInterface.(addInstruction)
+			if has {
+				// Multiple packages from the same project were specified as
+				// arguments; make sure they agree on declared constraints.
+				// TODO(sdboyer) until we have a general method for checking constraint equality, only allow one to declare
+				if someConstraint {
+					if !gps.IsAny(instr.constraint) || instr.id.Source != "" {
+						errCh <- errors.Errorf("can only specify rules once per project being added; rules were given at least twice for %s", pc.Ident.ProjectRoot)
+						return
+					}
+					instr.constraint = pc.Constraint
+					instr.id = pc.Ident
 				}
+			} else {
+				instr.ephReq = make(map[string]bool)
 				instr.constraint = pc.Constraint
 				instr.id = pc.Ident
 			}
-		} else {
-			instr.ephReq = make(map[string]bool)
-			instr.constraint = pc.Constraint
-			instr.id = pc.Ident
-		}
-
-		if inManifest {
-			if someConstraint {
-				return errors.Errorf("%s already contains rules for %s, cannot specify a version constraint or alternate source", dep.ManifestName, path)
-			}
 
-			instr.ephReq[path] = true
-			instr.typ |= isInManifest
-		} else if inImports {
-			if !someConstraint {
-				if exmap[path] {
-					return errors.Errorf("%s is already imported or required, so -add is only valid with a constraint", path)
+			if inManifest {
+				if someConstraint {
+					errCh <- errors.Errorf("%s already contains rules for %s, cannot specify a version constraint or alternate source", dep.ManifestName, path)
+					return
 				}
 
-				// No constraints, but the package isn't imported; require it.
-				// TODO(sdboyer) this case seems like it's getting overly specific and risks muddying the water more than it helps
 				instr.ephReq[path] = true
-				instr.typ |= isInImportsNoConstraint
-			} else {
-				// Don't require on this branch if the path was a ProjectRoot;
-				// most common here will be the user adding constraints to
-				// something they already imported, and if they specify the
-				// root, there's a good chance they don't actually want to
-				// require the project's root package, but are just trying to
-				// indicate which project should receive the constraints.
-				if !exmap[path] && string(pc.Ident.ProjectRoot) != path {
+				instr.typ |= isInManifest
+			} else if inImports {
+				if !someConstraint {
+					if exmap[path] {
+						errCh <- errors.Errorf("%s is already imported or required, so -add is only valid with a constraint", path)
+						return
+					}
+
+					// No constraints, but the package isn't imported; require it.
+					// TODO(sdboyer) this case seems like it's getting overly specific and risks muddying the water more than it helps
 					instr.ephReq[path] = true
+					instr.typ |= isInImportsNoConstraint
+				} else {
+					// Don't require on this branch if the path was a ProjectRoot;
+					// most common here will be the user adding constraints to
+					// something they already imported, and if they specify the
+					// root, there's a good chance they don't actually want to
+					// require the project's root package, but are just trying to
+					// indicate which project should receive the constraints.
+					if !exmap[path] && string(pc.Ident.ProjectRoot) != path {
+						instr.ephReq[path] = true
+					}
+					instr.typ |= isInImportsWithConstraint
 				}
-				instr.typ |= isInImportsWithConstraint
+			} else {
+				instr.typ |= isInNeither
+				instr.ephReq[path] = true
 			}
-		} else {
-			instr.typ |= isInNeither
-			instr.ephReq[path] = true
-		}
 
-		addInstructions[pc.Ident.ProjectRoot] = instr
+			addInstructions.Store(pc.Ident.ProjectRoot, instr)
+		}(arg)
+	}
+
+	wg.Wait()
+	close(errCh)
+
+	// Newline after printing the fetching source output.
+	ctx.Err.Println()
+
+	// Log all the errors.
+	if len(errCh) > 0 {
+		ctx.Err.Printf("Failed to add the dependencies:\n\n")
+		for err := range errCh {
+			ctx.Err.Println("  ✗", err.Error())
+		}
+		ctx.Err.Println()
+		return errAddDepsFailed
 	}
 
 	// We're now sure all of our add instructions are individually and mutually
 	// valid, so it's safe to begin modifying the input parameters.
-	for pr, instr := range addInstructions {
+	addInstructions.Range(func(ki, vi interface{}) bool {
+		pr := ki.(gps.ProjectRoot)
+		instr := vi.(addInstruction)
 		// The arg processing logic above only adds to the ephReq list if
 		// that package definitely needs to be on that list, so we don't
 		// need to check instr.typ here - if it's in instr.ephReq, it
@@ -569,7 +616,9 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm
 				Constraint: instr.constraint,
 			}
 		}
-	}
+
+		return true
+	})
 
 	// Re-prepare a solver now that our params are complete.
 	solver, err = gps.Prepare(params, sm)
@@ -587,7 +636,10 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm
 	var reqlist []string
 	appender := dep.NewManifest()
 
-	for pr, instr := range addInstructions {
+	addInstructions.Range(func(ki, vi interface{}) bool {
+		pr := ki.(gps.ProjectRoot)
+		instr := vi.(addInstruction)
+
 		for path := range instr.ephReq {
 			reqlist = append(reqlist, path)
 		}
@@ -615,7 +667,9 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm
 			}
 			appender.Constraints[pr] = pp
 		}
-	}
+
+		return true
+	})
 
 	extra, err := appender.MarshalTOML()
 	if err != nil {