Implement ... expansion for dev test

build.go

@@ -12,7 +12,6 @@ package main
 
 import (
 	"context"
-	"fmt"
 	"os"
 	"os/exec"
 	"path"
@@ -52,19 +51,8 @@ var buildTargetMapping = map[string]string{
 	"roachtest":        "//pkg/cmd/roachtest",
 }
 
-func init() {
-	// Shared flags.
-
-	// Wherever `buchr/bazel-remote` is running. We're somewhat tying ourselves
-	// to the one implementation, but that seems fine for now. It has support
-	// (very experimental) support for the  Remote Asset API, which helps speed
-	// things up when the cache sits across the network boundary.
-	buildCmd.Flags().String(remoteCacheFlag, "", "remote caching grpc endpoint to use")
-}
-
 func runBuild(cmd *cobra.Command, targets []string) error {
 	ctx := context.Background()
-
 	if len(targets) == 0 {
 		// Default to building the cockroach binary.
 		targets = append(targets, "cockroach")
@@ -80,11 +68,7 @@ func runBuild(cmd *cobra.Command, targets []string) error {
 	// Don't let bazel generate any convenience symlinks, we'll create them
 	// ourself.
 	args = append(args, "--experimental_convenience_symlinks=ignore")
-	if cAddr := mustGetFlagString(cmd, remoteCacheFlag); cAddr != "" {
-		args = append(args, "--remote_local_fallback")
-		args = append(args, fmt.Sprintf("--remote_cache=grpc://%s", cAddr))
-		args = append(args, fmt.Sprintf("--experimental_remote_downloader=grpc://%s", cAddr))
-	}
+	args = append(args, mustGetRemoteCacheArgs(remoteCacheAddr)...)
 
 	for _, target := range targets {
 		buildTarget, ok := buildTargetMapping[target]

main.go

@@ -11,6 +11,7 @@
 package main
 
 import (
+	"io/ioutil"
 	"log"
 	"os"
 	"os/exec"
@@ -50,21 +51,42 @@ lets engineers do a few things:
 }
 
 var (
-	bazel           = "bazel"
-	remoteCacheFlag = "remote-cache"
+	remoteCacheAddr string
+	debugLogger     = log.New(ioutil.Discard, "DEBUG: ", 0)
 )
 
 func init() {
 	log.SetFlags(0)
 	log.SetPrefix("")
 
-	devCmd.AddCommand(
+	cmds := []*cobra.Command{
 		benchCmd,
 		buildCmd,
 		generateCmd,
 		lintCmd,
 		testCmd,
-	)
+	}
+
+	// Add all the shared flags.
+	var debugVar bool
+	for _, cmd := range cmds {
+		cmd.Flags().BoolVar(&debugVar, "debug", false, "enable debug logging for dev itself")
+		// This points to the grpc endpoint of a running `buchr/bazel-remote`
+		// instance. We're tying ourselves to the one implementation, but that
+		// seems fine for now. It seems mature, and has (very experimental)
+		// support for the  Remote Asset API, which helps speed things up when
+		// the cache sits across the network boundary.
+		cmd.Flags().StringVar(&remoteCacheAddr, "remote-cache", "", "remote caching grpc endpoint to use")
+	}
+	for _, cmd := range cmds {
+		cmd.PreRun = func(cmd *cobra.Command, args []string) {
+			if debugVar {
+				debugLogger.SetOutput(os.Stderr)
+			}
+		}
+	}
+
+	devCmd.AddCommand(cmds...)
 
 	// Hide the `help` sub-command.
 	devCmd.SetHelpCommand(&cobra.Command{
@@ -74,7 +96,7 @@ func init() {
 }
 
 func runDev() error {
-	_, err := exec.LookPath(bazel)
+	_, err := exec.LookPath("bazel")
 	if err != nil {
 		return errors.New("bazel not found in $PATH")
 	}
@@ -87,7 +109,7 @@ func runDev() error {
 
 func main() {
 	if err := runDev(); err != nil {
-		log.Printf("error: %v", err)
+		log.Printf("ERROR: %v", err)
 		os.Exit(1)
 	}
 }

test.go

@@ -13,7 +13,7 @@ package main
 import (
 	"context"
 	"fmt"
-	"log"
+	"os/exec"
 	"strings"
 	"time"
 
@@ -33,10 +33,7 @@ var testCmd = &cobra.Command{
 	dev test --logic --files=prepare|fk --subtests=20042 --config=local
 	dev test --fuzz sql/sem/tree --filter=Decimal`,
 	Args: cobra.MinimumNArgs(0),
-	RunE: func(cmd *cobra.Command, args []string) error {
-		ctx := context.Background()
-		return runTest(ctx, cmd, args)
-	},
+	RunE: runTest,
 }
 
 var (
@@ -77,12 +74,13 @@ func init() {
 	testCmd.Flags().String(filesFlag, "", "run logic tests for files matching this regex")
 	testCmd.Flags().String(subtestsFlag, "", "run logic test subtests matching this regex")
 	testCmd.Flags().String(configFlag, "", "run logic tests under the specified config")
-
-	// Shared flags.
-	testCmd.Flags().String(remoteCacheFlag, "", "remote caching endpoint to use")
 }
 
-func runTest(ctx context.Context, cmd *cobra.Command, pkgs []string) error {
+// TODO(irfansharif): Add tests for the various bazel commands that get
+// generated from the set of provided user flags.
+
+func runTest(cmd *cobra.Command, pkgs []string) error {
+	ctx := context.Background()
 	if logicTest := mustGetFlagBool(cmd, logicFlag); logicTest {
 		return runLogicTest(cmd)
 	}
@@ -102,34 +100,67 @@ func runUnitTest(ctx context.Context, cmd *cobra.Command, pkgs []string) error {
 	ignoreCache := mustGetFlagBool(cmd, ignoreCacheFlag)
 	verbose := mustGetFlagBool(cmd, vFlag)
 	showLogs := mustGetFlagBool(cmd, showLogsFlag)
-	cAddr := mustGetFlagString(cmd, remoteCacheFlag)
 
 	if showLogs {
 		return errors.New("-show-logs unimplemented")
 	}
 
-	log.Printf("unit test args: stress=%t  race=%t  filter=%s  timeout=%s  ignore-cache=%t  pkgs=%s",
+	debugLogger.Printf("unit test args: stress=%t  race=%t  filter=%s  timeout=%s  ignore-cache=%t  pkgs=%s",
 		stress, race, filter, timeout, ignoreCache, pkgs)
 
 	var args []string
 	args = append(args, "test")
+	args = append(args, "--color=yes")
+	args = append(args, "--experimental_convenience_symlinks=ignore") // don't generate any convenience symlinks
+	args = append(args, mustGetRemoteCacheArgs(remoteCacheAddr)...)
 	if race {
 		args = append(args, "--features", "race")
 	}
-	args = append(args, "--color=yes")
-	if cAddr != "" {
-		args = append(args, "--remote_local_fallback")
-		args = append(args, fmt.Sprintf("--remote_cache=grpc://%s", cAddr))
-		args = append(args, fmt.Sprintf("--experimental_remote_downloader=grpc://%s", cAddr))
-	}
 
 	for _, pkg := range pkgs {
+		if !strings.HasPrefix(pkg, "pkg/") {
+			return errors.Newf("malformed package %q, expecting %q", pkg, "pkg/{...}")
+		}
+
 		if strings.HasSuffix(pkg, "...") {
-			args = append(args, fmt.Sprintf("@cockroach//%s", pkg))
+			// Similar to `go test`, we implement `...` expansion to allow
+			// callers to use the following pattern to test all packages under a
+			// named one:
+			//
+			//     dev test pkg/util/... -v
+			//
+			// NB: We'll want to filter for just the go_test targets here. Not
+			// doing so prompts bazel to try and build all named targets. This
+			// is undesirable for the various `*_proto` targets seeing as how
+			// they're not buildable in isolation. This is because we often
+			// attach methods to proto types in hand-written files, files that
+			// are not picked up by the proto bazel targets[1]. Regular bazel
+			// compilation is still fine seeing as how the top-level go_library
+			// targets both embeds the proto target, and sources the
+			// hand-written file. But the proto target in isolation may not be
+			// buildable because without those additional methods, those types
+			// may fail to satisfy required interfaces.
+			//
+			// So, blinding selecting for all targets won't work, and we'll want
+			// to filter things out first.
+			//
+			// [1]: pkg/rpc/heartbeat.proto is one example of this pattern,
+			// where we define `Stringer` separately for the `RemoteOffset`
+			// type.
+			{
+				out, err := exec.Command("bazel", "query", fmt.Sprintf("kind(go_test,  //%s)", pkg)).Output()
+				if err != nil {
+					return err
+				}
+				targets := strings.TrimSpace(string(out))
+				for _, target := range strings.Split(targets, "\n") {
+					args = append(args, target)
+				}
+			}
 		} else {
 			components := strings.Split(pkg, "/")
 			pkgName := components[len(components)-1]
-			args = append(args, fmt.Sprintf("@cockroach//%s:%s_test", pkg, pkgName))
+			args = append(args, fmt.Sprintf("//%s:%s_test", pkg, pkgName))
 		}
 	}
 
@@ -156,14 +187,14 @@ func runLogicTest(cmd *cobra.Command) error {
 	subtests := mustGetFlagString(cmd, subtestsFlag)
 	config := mustGetFlagString(cmd, configFlag)
 
-	log.Printf("logic test args: files=%s  subtests=%s  config=%s",
+	debugLogger.Printf("logic test args: files=%s  subtests=%s  config=%s",
 		files, subtests, config)
 	return errors.New("--logic unimplemented")
 }
 
 func runFuzzTest(cmd *cobra.Command, pkgs []string) error {
 	filter := mustGetFlagString(cmd, filterFlag)
 
-	log.Printf("fuzz test args: filter=%s  pkgs=%s", filter, pkgs)
+	debugLogger.Printf("fuzz test args: filter=%s  pkgs=%s", filter, pkgs)
 	return errors.New("--fuzz unimplemented")
 }

util.go

@@ -15,6 +15,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"log"
+	"net"
 	"os"
 	"os/exec"
 	"strings"
@@ -48,9 +49,38 @@ func mustGetFlagDuration(cmd *cobra.Command, name string) time.Duration {
 	return val
 }
 
+func parseAddr(addr string) (string, error) {
+	host, port, err := net.SplitHostPort(addr)
+	if err != nil {
+		return "", err
+	}
+
+	ip := net.ParseIP(host)
+	if ip == nil {
+		return "", errors.Newf("invalid address %s", addr)
+	}
+
+	return fmt.Sprintf("%s:%s", ip, port), nil
+}
+
+func mustGetRemoteCacheArgs(cacheAddr string) []string {
+	if cacheAddr == "" {
+		return nil
+	}
+	cAddr, err := parseAddr(cacheAddr)
+	if err != nil {
+		log.Fatalf("unexpected error: %v", err)
+	}
+	var args []string
+	args = append(args, "--remote_local_fallback")
+	args = append(args, fmt.Sprintf("--remote_cache=grpc://%s", cAddr))
+	args = append(args, fmt.Sprintf("--experimental_remote_downloader=grpc://%s", cAddr))
+	return args
+}
+
 func execute(ctx context.Context, name string, args ...string) error {
 	cmd := exec.CommandContext(ctx, name, args...)
-	log.Printf("executing: %s", cmd.String())
+	debugLogger.Printf("executing: %s", cmd.String())
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 	if err := cmd.Start(); err != nil {