gen/genbzl: general improvements

pkg/gen/BUILD.bazel

@@ -1,15 +1,4 @@
-load(":gen.bzl", "EXPLICIT_SRCS", "docs", "execgen", "gen", "go_proto", "gomock", "misc", "optgen", "stringer")
-load(":excluded.bzl", "EXCLUDED_SRCS")
-
-filegroup(
-    name = "explicitly_generated",
-    srcs = EXPLICIT_SRCS,
-)
-
-filegroup(
-    name = "excluded",
-    srcs = EXCLUDED_SRCS,
-)
+load(":gen.bzl", "docs", "execgen", "gen", "go_proto", "gomock", "misc", "optgen", "stringer")
 
 execgen()
 

pkg/gen/gen.bzl

@@ -27,13 +27,6 @@ load(":optgen.bzl", "OPTGEN_SRCS")
 load(":misc.bzl", "MISC_SRCS")
 load(":docs.bzl", "DOCS_SRCS")
 
-
-# TODO(ajwerner): Use the all variable combined with genquery to construct
-# a test to show that all of the generated files in the repo are represented
-# here. Of course, this will rely on actually representing all of the generated
-# file here.
-EXPLICIT_SRCS = PROTOBUF_SRCS + GOMOCK_SRCS + STRINGER_SRCS + EXECGEN_SRCS + OPTGEN_SRCS + DOCS_SRCS
-
 # GeneratedFileInfo provides two pieces of information to the _hoist_files
 # rule. It provides the set of files to be hoisted via the generated_files
 # field and it provides a list of commands to run to clean up potentially
@@ -147,7 +140,7 @@ chmod 0644 {dst}\
 set -euo pipefail
 
 {cleanup_tasks}
-{cmds}\
+{cmds}
 """
   cleanup_tasks = []
   cmds = []

pkg/gen/genbzl/BUILD.bazel

@@ -2,7 +2,11 @@ load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")
 
 go_library(
     name = "genbzl_lib",
-    srcs = ["main.go"],
+    srcs = [
+        "main.go",
+        "target.go",
+        "targets.go",
+    ],
     importpath = "github.com/cockroachdb/cockroach/pkg/gen/genbzl",
     visibility = ["//visibility:private"],
     deps = [

pkg/gen/genbzl/main.go

@@ -12,30 +12,29 @@
 // the gen package's BUILD.bazel to facilitate hoisting these generated files
 // back into the source tree.
 //
-// It's all a bit meta. The flow is that we invoke this binary inside the
-// bazelutil/bazel-generate.sh script which writes out some bzl files with
-// lists of targets which are then depended on in gen.bzl.
+// The flow is that we invoke this binary inside the
+// bazelutil/bazel-generate.sh script which writes out some bzl files
+// defining lists of targets for generation and hoisting.
+//
+// The program assumes it will be run from the root of the cockroach workspace.
+// If it's not, errors will occur.
 package main
 
 import (
-	"bufio"
-	"bytes"
 	"flag"
 	"fmt"
-	"io"
 	"os"
-	"os/exec"
 	"path/filepath"
-	"sort"
 	"strings"
-	"text/template"
 
 	"github.com/cockroachdb/cockroach/pkg/cli/exit"
-	"github.com/cockroachdb/errors"
 )
 
 var (
-	outDir = flag.String("out-dir", "", "directory in which to place the generated files")
+	outDir = flag.String(
+		"out-dir", "",
+		"directory in which to place the generated files",
+	)
 )
 
 func main() {
@@ -46,147 +45,57 @@ func main() {
 	}
 }
 
-var tmpl = template.Must(template.New("file").Parse(
-	`# Generated by genbzl
-
-{{ .Variable }} = [{{ range .Targets }}
-  "{{ . }}",{{end}}
-]
-`))
-
-type templateData struct {
-	Variable string
-	Targets  []string
-}
-
-var targets = []*target{
-	{
-		filename: "protobuf.bzl", variable: "PROTOBUF_SRCS",
-		query: "kind(go_proto_library, //pkg/...)",
-	},
-	{
-		filename: "gomock.bzl", variable: "GOMOCK_SRCS",
-		query: `labels("out", kind("_gomock_prog_exec rule",  //pkg/...:*))`,
-	},
-	{
-		filename: "stringer.bzl",
-		variable: "STRINGER_SRCS",
-		query:    `labels("outs",  filter("-stringer$", kind("genrule rule",  //pkg/...:*)))`,
-	},
-	{
-		filename: "execgen.bzl", variable: "EXECGEN_SRCS",
-		query: `
-let genrules =
-  kind("genrule rule",  //pkg/...:*)
-in labels("outs",  attr("tools", "execgen", $genrules)
-  + attr("exec_tools", "execgen", $genrules))`,
-	},
-	{
-		filename: "optgen.bzl", variable: "OPTGEN_SRCS",
-		query: `
-let targets = attr("exec_tools", "(opt|lang)gen",  kind("genrule rule",  //pkg/...:*))
-in let og = labels("outs",  $targets)
-in $og - filter(".*:.*(-gen|gen-).*", $og)`,
-	},
-	{
-		filename: "docs.bzl", variable: "DOCS_SRCS",
-		query: `
-kind("generated file", //docs/...:*)
-  - labels("outs", //docs/generated/sql/bnf:svg)`,
-	},
-	{
-		filename: "excluded.bzl", variable: "EXCLUDED_SRCS",
-		query: `
-let all = kind("generated file", //...:*)
-in ($all ^ //pkg/ui/...:*)
-  + ($all ^ labels("out", kind("_gomock_prog_gen rule",  //pkg/...:*)))
-  + filter(".*:.*(-gen|gen-).*", $all)
-  + //pkg/testutils/lint/passes/errcheck:errcheck_excludes.txt
-  + //build/bazelutil:test_stamping.txt
-  + labels("outs", //docs/generated/sql/bnf:svg)`,
-	},
-	// MISC_SRCS is a special beast to capture everything which is not excluded
-	// or included explicitly in one of the other targets. By defining this
-	// target in this way, we ensure that we don't forget to generate anything.
-	// One note is that this needs to be last. The definition relies on the
-	// existence of the above definitions (indirectly via the fact that these
-	// referenced gen labels refer to variables whose data we're populating
-	// here).
-	{
-		filename: "misc.bzl", variable: "MISC_SRCS",
-		query: `
-kind("generated file", //...:*)
-  - labels("srcs", //pkg/gen:explicitly_generated)
-  - labels("srcs", //pkg/gen:excluded)
-`,
-	},
-}
-
-type target struct {
-	filename string
-	variable string
-	query    string
-}
-
-func execQuery(q string) (results []string, _ error) {
-	cmd := exec.Command("bazel", "query", q)
-	var stdout bytes.Buffer
-	var stderr strings.Builder
-	cmd.Stdout = &stdout
-	cmd.Stderr = &stderr
-	if err := cmd.Run(); err != nil {
-		return nil, errors.Wrapf(err,
-			"failed to run %s: (stderr)\n%s", cmd, &stderr)
-	}
-	for sc := bufio.NewScanner(&stdout); sc.Scan(); {
-		results = append(results, sc.Text())
-	}
-	sort.Strings(results)
-	return results, nil
-}
-
 func generate(outDir string) error {
-	// Go through once and rewrite all the generated variables to be empty sets.
-	// The reason to do this is that the values stored in these variables are
-	// part of the build graph and can affect `bazel query`. If any of the
-	// targets were to have rotted and contain values which don't exist in the
-	// graph, we'd see errors. Clearing out the lists and then building them up
-	// again from empty helps avoid such issues.
-	for _, t := range targets {
-		if err := t.write(outDir, nil); err != nil {
-			return err
-		}
+	qd, err := getQueryData()
+	if err != nil {
+		return err
 	}
-	// Now go and populate the lists based on the results of the queries.
-	for _, t := range targets {
-		out, err := execQuery(t.query)
+	for _, q := range targets {
+		if q.doNotGenerate {
+			continue
+		}
+		out, err := q.target.execQuery(qd)
 		if err != nil {
 			return err
 		}
-		if err := t.write(outDir, out); err != nil {
+		if err := q.target.write(outDir, out); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-func (t *target) write(outDir string, out []string) error {
-	var buf bytes.Buffer
-	if err := tmpl.Execute(&buf, templateData{
-		Variable: t.variable,
-		Targets:  out,
-	}); err != nil {
-		return errors.Wrapf(err, "failed to execute template for %s", t.filename)
-	}
-	f, err := os.Create(filepath.Join(outDir, t.filename))
+// getQueryData gets a bazel query expression that attempt to capture all of
+// the files and targets we're interested in. Importantly, it excludes certain
+// directories such as this here gen directory and pkg/ui. It excludes the
+// current directory, because the currently generated files may have errors.
+// It excludes the pkg/ui directory because including it will lead to an
+// invocation of npm, which can be slow and painful on platforms without good
+// npm support.
+//
+// The targets should be thought of as the following expression, constructed
+// additively in code.
+//
+//  build/...:* + //docs/...:* + //pkg/...:* - //pkg//ui/...:* - //pkg/gen/...:*
+//
+func getQueryData() (*queryData, error) {
+	dirs := []string{"build", "docs"}
+	ents, err := os.ReadDir("pkg")
 	if err != nil {
-		return errors.Wrapf(err, "failed to open file for %s", t.filename)
+		return nil, err
 	}
-	if _, err := io.Copy(f, &buf); err != nil {
-		return errors.Wrapf(err, "failed to write file for %s", t.filename)
+	toSkip := map[string]struct{}{"ui": {}, "gen": {}}
+	for _, e := range ents {
+		if _, shouldSkip := toSkip[e.Name()]; shouldSkip || !e.IsDir() {
+			continue
+		}
+		dirs = append(dirs, filepath.Join("pkg", e.Name()))
 	}
-	if err := f.Close(); err != nil {
-		return errors.Wrapf(err, "failed to write file for %s", t.filename)
+	exprs := make([]string, 0, len(dirs))
+	for _, dir := range dirs {
+		exprs = append(exprs, "//"+dir+"/...:*")
 	}
-	return nil
+	return &queryData{
+		All: "(" + strings.Join(exprs, " + ") + ")",
+	}, nil
 }

pkg/gen/genbzl/target.go

@@ -0,0 +1,102 @@
+// Copyright 2022 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package main
+
+import (
+	"bufio"
+	"bytes"
+	"io"
+	"io/ioutil"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"sort"
+	"strings"
+	"text/template"
+
+	"github.com/cockroachdb/errors"
+)
+
+type target string
+
+func (t target) query() *template.Template {
+	return queryTemplates.Lookup(string(t))
+}
+
+func (t target) execQuery(qd *queryData) (results []string, _ error) {
+	f, err := ioutil.TempFile("", "")
+	if err != nil {
+		return nil, err
+	}
+	defer func() { _ = os.Remove(f.Name()) }()
+	if err := t.query().Execute(f, qd); err != nil {
+		return nil, err
+	}
+	if err := f.Close(); err != nil {
+		return nil, err
+	}
+	cmd := exec.Command("bazel", "query", "--query_file", f.Name())
+	var stdout bytes.Buffer
+	var stderr strings.Builder
+	cmd.Stdout = &stdout
+	cmd.Stderr = &stderr
+	if err := cmd.Run(); err != nil {
+		return nil, errors.Wrapf(err,
+			"failed to run %s: (stderr)\n%s", cmd, &stderr)
+	}
+	for sc := bufio.NewScanner(&stdout); sc.Scan(); {
+		results = append(results, sc.Text())
+	}
+	sort.Strings(results)
+	return results, nil
+}
+
+func (t target) filename() string {
+	return string(t) + ".bzl"
+}
+
+func (t target) variable() string {
+	return strings.ToUpper(string(t)) + "_SRCS"
+}
+
+func (t target) write(outDir string, out []string) error {
+	var buf bytes.Buffer
+	if err := outputVariableTemplate.Execute(&buf, variableData{
+		Variable: t.variable(),
+		Targets:  out,
+	}); err != nil {
+		return errors.Wrapf(err, "failed to execute template for %s", t)
+	}
+	f, err := os.Create(filepath.Join(outDir, t.filename()))
+	if err != nil {
+		return errors.Wrapf(err, "failed to open file for %s", t)
+	}
+	if _, err := io.Copy(f, &buf); err != nil {
+		return errors.Wrapf(err, "failed to write file for %s", t)
+	}
+	if err := f.Close(); err != nil {
+		return errors.Wrapf(err, "failed to write file for %s", t)
+	}
+	return nil
+}
+
+var outputVariableTemplate = template.Must(template.New("file").Parse(
+	`# Generated by genbzl
+
+{{ .Variable }} = [{{ range .Targets }}
+  "{{ . }}",{{end}}
+]
+`))
+
+type variableData struct {
+	Variable string
+	Targets  []string
+}