Skip to content

Commit

Permalink
dev: hoist generated code out of sandbox into workspace
Browse files Browse the repository at this point in the history
One might be expected that this command be implemented as
`dev generate protobuf` or `dev generate execgen` or similar, but that's
not really practical for a couple reasons:

1. Building all the protobuf in-tree isn't really possible on its own:
   see #58018 (comment).
2. For non-proto generated code, one can imagine that we do a bunch of
   `bazel query`s to list all the generated files in tree and to find
   which files are generated. Given that there are hundreds of generated
   files in tree, and a single `bazel query` can take ~1s, this is not
   tractable.

The implementation here is ad-hoc and a bit hacky. Basically we search
`bazel-bin` for files ending in `.go` and apply some heuristics to
figure out whether they should be copied and where those files should
go.

This is gated behind a feature flag for now because actually copying
these files into the workspace right now dirties a ton of files. (The
diffs are benign -- mostly comments.) We can flip this option to be on
by default when it's safe (probably after the `Makefile` is deleted).

Closes #68565.

Release note: None
  • Loading branch information
rickystewart committed Aug 12, 2021
1 parent b9691c4 commit e44df5d
Show file tree
Hide file tree
Showing 6 changed files with 172 additions and 26 deletions.
74 changes: 56 additions & 18 deletions pkg/cmd/dev/build.go
Expand Up @@ -14,6 +14,7 @@ import (
"context"
"fmt"
"log"
"os"
"path"
"path/filepath"
"strings"
Expand All @@ -24,7 +25,10 @@ import (
"github.com/spf13/cobra"
)

const crossFlag = "cross"
const (
crossFlag = "cross"
hoistGeneratedCodeFlag = "hoist-generated-code"
)

// makeBuildCmd constructs the subcommand used to build the specified binaries.
func makeBuildCmd(runE func(cmd *cobra.Command, args []string) error) *cobra.Command {
Expand All @@ -46,6 +50,7 @@ func makeBuildCmd(runE func(cmd *cobra.Command, args []string) error) *cobra.Com
You can optionally set a config, as in --cross=windows.
Defaults to linux if not specified. The config should be the name of a
build configuration specified in .bazelrc, minus the "cross" prefix.`)
buildCmd.Flags().Bool(hoistGeneratedCodeFlag, false, "hoist generated code out of the Bazel sandbox into the workspace")
buildCmd.Flags().Lookup(crossFlag).NoOptDefVal = "linux"
return buildCmd
}
Expand Down Expand Up @@ -74,6 +79,7 @@ var buildTargetMapping = map[string]string{
func (d *dev) build(cmd *cobra.Command, targets []string) error {
ctx := cmd.Context()
cross := mustGetFlagString(cmd, crossFlag)
hoistGeneratedCode := mustGetFlagBool(cmd, hoistGeneratedCodeFlag)

args, fullTargets, err := getBasicBuildArgs(targets)
if err != nil {
Expand All @@ -85,7 +91,7 @@ func (d *dev) build(cmd *cobra.Command, targets []string) error {
if err := d.exec.CommandContextInheritingStdStreams(ctx, "bazel", args...); err != nil {
return err
}
return d.symlinkBinaries(ctx, fullTargets)
return d.stageArtifacts(ctx, fullTargets, hoistGeneratedCode)
}
// Cross-compilation case.
cross = "cross" + cross
Expand Down Expand Up @@ -117,7 +123,7 @@ func (d *dev) build(cmd *cobra.Command, targets []string) error {
return nil
}

func (d *dev) symlinkBinaries(ctx context.Context, targets []string) error {
func (d *dev) stageArtifacts(ctx context.Context, targets []string, hoistGeneratedCode bool) error {
workspace, err := d.getWorkspace(ctx)
if err != nil {
return err
Expand All @@ -126,20 +132,21 @@ func (d *dev) symlinkBinaries(ctx context.Context, targets []string) error {
if err = d.os.MkdirAll(path.Join(workspace, "bin")); err != nil {
return err
}
bazelBin, err := d.getBazelBin(ctx)
if err != nil {
return err
}

for _, target := range targets {
binaryPath, err := d.getPathToBin(ctx, target)
if err != nil {
return err
}
binaryPath := filepath.Join(bazelBin, bazelutil.OutputOfBinaryRule(target))
base := targetToBinBasename(target)
var symlinkPath string
// Binaries beginning with the string "cockroach" go right at
// the top of the workspace; others go in the `bin` directory.
if strings.HasPrefix(base, "cockroach") {
symlinkPath = path.Join(workspace, base)
symlinkPath = filepath.Join(workspace, base)
} else {
symlinkPath = path.Join(workspace, "bin", base)
symlinkPath = filepath.Join(workspace, "bin", base)
}

// Symlink from binaryPath -> symlinkPath
Expand All @@ -156,6 +163,46 @@ func (d *dev) symlinkBinaries(ctx context.Context, targets []string) error {
logSuccessfulBuild(target, rel)
}

if hoistGeneratedCode {
goFiles, err := d.os.ListFilesWithSuffix(filepath.Join(bazelBin, "pkg"), ".go")
if err != nil {
return err
}
for _, file := range goFiles {
const cockroachURL = "github.com/cockroachdb/cockroach/"
ind := strings.LastIndex(file, cockroachURL)
if ind > 0 {
// If the cockroach URL was found in the filepath, then we should
// trim everything up to and including the URL to find the path
// where the file should be staged.
loc := file[ind+len(cockroachURL):]
err := d.os.CopyFile(file, filepath.Join(workspace, loc))
if err != nil {
return err
}
continue
}
pathComponents := strings.Split(file, string(os.PathSeparator))
var skip bool
for _, component := range pathComponents[:len(pathComponents)-1] {
// Pretty decent heuristic for whether a file needs to be staged.
// When path components contain ., they normally are generated files
// from third-party packages, as in google.golang.org. Meanwhile,
// when path components end in _, that usually represents internal
// stuff that doesn't need to be staged, like
// pkg/cmd/dev/dev_test_/testmain.go. Note that generated proto code
// is handled by the cockroach URL case above.
if len(component) > 0 && (strings.ContainsRune(component, '.') || component[len(component)-1] == '_') {
skip = true
}
}
if !skip {
// Failures here don't mean much. Just ignore them.
_ = d.os.CopyFile(file, filepath.Join(workspace, strings.TrimPrefix(file, bazelBin+"/")))
}
}
}

return nil
}

Expand All @@ -170,15 +217,6 @@ func targetToBinBasename(target string) string {
return base
}

func (d *dev) getPathToBin(ctx context.Context, target string) (string, error) {
bazelBin, err := d.getBazelBin(ctx)
if err != nil {
return "", err
}
rel := bazelutil.OutputOfBinaryRule(target)
return filepath.Join(bazelBin, rel), nil
}

// getBasicBuildArgs is for enumerating the arguments to pass to `bazel` in
// order to build the given high-level targets.
// The first string slice returned is the list of arguments (i.e. to pass to
Expand Down
20 changes: 13 additions & 7 deletions pkg/cmd/dev/generate.go
Expand Up @@ -28,10 +28,10 @@ func makeGenerateCmd(runE func(cmd *cobra.Command, args []string) error) *cobra.
Short: `Generate the specified files`,
Long: `Generate the specified files.`,
Example: `
dev generate
dev generate bazel
dev generate protobuf
dev generate {exec,opt}gen`,
dev generate
dev generate bazel
dev generate docs
`,
Args: cobra.MinimumNArgs(0),
// TODO(irfansharif): Errors but default just eaten up. Let's wrap these
// invocations in something that prints out the appropriate error log
Expand All @@ -41,10 +41,12 @@ func makeGenerateCmd(runE func(cmd *cobra.Command, args []string) error) *cobra.
}

func (d *dev) generate(cmd *cobra.Command, targets []string) error {
// TODO(irfansharif): Flesh out the remaining targets.
var generatorTargetMapping = map[string]func(cmd *cobra.Command) error{
"bazel": d.generateBazel,
"docs": d.generateDocs,
"bazel": d.generateBazel,
"docs": d.generateDocs,
"execgen": d.generateUnimplemented,
"optgen": d.generateUnimplemented,
"proto": d.generateUnimplemented,
}

if len(targets) == 0 {
Expand Down Expand Up @@ -129,3 +131,7 @@ func (d *dev) generateDocs(cmd *cobra.Command) error {
}
return nil
}

func (*dev) generateUnimplemented(*cobra.Command) error {
return errors.New("To generate Go code, run `dev build` with the flag `--hoist-generated-code`")
}
37 changes: 37 additions & 0 deletions pkg/cmd/dev/io/os/os.go
Expand Up @@ -13,9 +13,12 @@ package os
import (
"fmt"
"io"
"io/fs"
"io/ioutil"
"log"
"os"
"path/filepath"
"strings"

"github.com/cockroachdb/cockroach/pkg/cmd/dev/recording"
"github.com/cockroachdb/errors/oserror"
Expand Down Expand Up @@ -206,6 +209,40 @@ func (o *OS) CopyFile(src, dst string) error {
return err
}

// ListFilesWithSuffix lists all the files under a directory recursively that
// end in the given suffix.
func (o *OS) ListFilesWithSuffix(root, suffix string) ([]string, error) {
command := fmt.Sprintf("find %s -name *%s", root, suffix)
o.logger.Print(command)

var ret []string
if o.Recording == nil {
// Do the real thing.
err := filepath.Walk(root, func(path string, info fs.FileInfo, err error) error {
// If there's an error walking the tree, throw it away -- there's nothing
// interesting we can do with it.
if err != nil || info.IsDir() {
//nolint:returnerrcheck
return nil
}
if strings.HasSuffix(path, suffix) {
ret = append(ret, path)
}
return nil
})
if err != nil {
return nil, err
}
return ret, nil
}

lines, err := o.replay(command)
if err != nil {
return nil, err
}
return strings.Split(strings.TrimSpace(lines), "\n"), nil
}

// replay replays the specified command, erroring out if it's mismatched with
// what the recording plays back next. It returns the recorded output.
func (o *OS) replay(command string) (output string, err error) {
Expand Down
5 changes: 4 additions & 1 deletion pkg/cmd/dev/test.go
Expand Up @@ -12,8 +12,10 @@ package main

import (
"fmt"
"path/filepath"
"strings"

bazelutil "github.com/cockroachdb/cockroach/pkg/build/util"
"github.com/cockroachdb/errors"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -99,10 +101,11 @@ func (d *dev) runUnitTest(cmd *cobra.Command, pkgs []string) error {
if err != nil {
return err
}
stressBin, err = d.getPathToBin(ctx, stressTarget)
bazelBin, err := d.getBazelBin(ctx)
if err != nil {
return err
}
stressBin = filepath.Join(bazelBin, bazelutil.OutputOfBinaryRule(stressTarget))
}

var args []string
Expand Down
15 changes: 15 additions & 0 deletions pkg/cmd/dev/testdata/build.txt
Expand Up @@ -62,3 +62,18 @@ mkdir go/src/github.com/cockroachdb/cockroach/bin
bazel info bazel-bin --color=no
rm go/src/github.com/cockroachdb/cockroach/cockroach-short
ln -s /private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/cmd/cockroach-short/cockroach-short_/cockroach-short go/src/github.com/cockroachdb/cockroach/cockroach-short

dev build cockroach-short --hoist-generated-code
----
getenv PATH
which cc
readlink /usr/local/opt/ccache/libexec/cc
export PATH=/usr/local/opt/make/libexec/gnubin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/go/bin:/Library/Apple/usr/bin
bazel build --color=yes --experimental_convenience_symlinks=ignore //pkg/cmd/cockroach-short --config=dev
bazel info workspace --color=no --config=dev
mkdir go/src/github.com/cockroachdb/cockroach/bin
bazel info bazel-bin --color=no --config=dev
rm go/src/github.com/cockroachdb/cockroach/cockroach-short
ln -s /private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/cmd/cockroach-short/cockroach-short_/cockroach-short go/src/github.com/cockroachdb/cockroach/cockroach-short
find /private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg -name *.go
cp /private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/kv/kvserver/kvserver_go_proto_/github.com/cockroachdb/cockroach/pkg/kv/kvserver/storage_services.pb.go go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/storage_services.pb.go
47 changes: 47 additions & 0 deletions pkg/cmd/dev/testdata/recording/build.txt
Expand Up @@ -172,3 +172,50 @@ rm go/src/github.com/cockroachdb/cockroach/cockroach-short

ln -s /private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/cmd/cockroach-short/cockroach-short_/cockroach-short go/src/github.com/cockroachdb/cockroach/cockroach-short
----

getenv PATH
----
/usr/local/opt/ccache/libexec:/usr/local/opt/make/libexec/gnubin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/go/bin:/Library/Apple/usr/bin

which cc
----
/usr/local/opt/ccache/libexec/cc

readlink /usr/local/opt/ccache/libexec/cc
----
../bin/ccache

export PATH=/usr/local/opt/make/libexec/gnubin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/go/bin:/Library/Apple/usr/bin
----

bazel build --color=yes --experimental_convenience_symlinks=ignore //pkg/cmd/cockroach-short --config=dev
----

bazel info workspace --color=no --config=dev
----
go/src/github.com/cockroachdb/cockroach

mkdir go/src/github.com/cockroachdb/cockroach/bin
----

bazel info bazel-bin --color=no --config=dev
----
/private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin

rm go/src/github.com/cockroachdb/cockroach/cockroach-short
----

ln -s /private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/cmd/cockroach-short/cockroach-short_/cockroach-short go/src/github.com/cockroachdb/cockroach/cockroach-short
----

find /private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg -name *.go
----
----
/private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/_bazel/bin/pkg/cmd/dev/dev_test_/testmain.go
/private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/kv/kvclient/rangefeed/mock_rangefeed_gomock_gopath/src/google.golang.org/grpc/preloader.go
/private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/kv/kvserver/kvserver_go_proto_/github.com/cockroachdb/cockroach/pkg/kv/kvserver/storage_services.pb.go
----
----

cp /private/var/tmp/_bazel/99e666e4e674209ecdb66b46371278df/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/kv/kvserver/kvserver_go_proto_/github.com/cockroachdb/cockroach/pkg/kv/kvserver/storage_services.pb.go go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/storage_services.pb.go
----

0 comments on commit e44df5d

Please sign in to comment.