From 9b3eb9c637f8cc47d5543830d1aae4b09705bbf7 Mon Sep 17 00:00:00 2001 From: Scott Minor Date: Fri, 15 Oct 2021 13:12:31 -0700 Subject: [PATCH 1/3] bazel: Parse workspace_log during query This change modifies Query to return a struct of query results. These results include targets that match the query, as well as other optional information. The first useful such piece of information is the workspace log, which details when: * WORKSPACE caused a download to happen, including the SHA256 of the download * WORKSPACE caused a command execution to happen among other events. These events will be useful in reducing external dependencies down to the SHA256s of their corresponding downloads, rather than needing to resort to hashing individual files of the external dependencies. --- lib/bazel/BUILD.bazel | 4 +++- lib/bazel/affected_targets.go | 19 ++++++++++++--- lib/bazel/commands/commands.go | 2 +- lib/bazel/options.go | 37 +++++++++++++++++++++++++++-- lib/bazel/options_test.go | 26 ++++++++++++++++++++ lib/bazel/proto/BUILD.bazel | 10 ++++++++ lib/bazel/proto/workspace_log.proto | 2 +- lib/bazel/query.go | 29 +++++++++++++++++++--- lib/bazel/query_test.go | 2 +- lib/git/worktree.go | 5 ++-- lib/git/worktree_test.go | 4 ++-- machinist/testing/BUILD.bazel | 5 +--- 12 files changed, 125 insertions(+), 20 deletions(-) create mode 100644 lib/bazel/options_test.go diff --git a/lib/bazel/BUILD.bazel b/lib/bazel/BUILD.bazel index 842fd716e..d0ec1cc55 100644 --- a/lib/bazel/BUILD.bazel +++ b/lib/bazel/BUILD.bazel @@ -11,7 +11,8 @@ go_library( importpath = "github.com/enfabrica/enkit/lib/bazel", visibility = ["//visibility:public"], deps = [ - "//lib/bazel/proto:build_go_proto", + "//lib/bazel/proto:go_protos", + "//lib/multierror:go_default_library", "//lib/proto/delimited:go_default_library", "@com_github_bazelbuild_buildtools//wspace:go_default_library", "@org_golang_google_protobuf//proto:go_default_library", @@ -21,6 +22,7 @@ go_library( go_test( name = "go_default_test", srcs = [ + "options_test.go", "query_test.go", "workspace_test.go", ], diff --git a/lib/bazel/affected_targets.go b/lib/bazel/affected_targets.go index bd4dc3a75..f73e97fc9 100644 --- a/lib/bazel/affected_targets.go +++ b/lib/bazel/affected_targets.go @@ -53,19 +53,32 @@ func GetAffectedTargets(start string, end string) ([]string, error) { return nil, fmt.Errorf("failed to open bazel workspace: %w", err) } + workspaceLogStart, err := WithTempWorkspaceRulesLog() + if err != nil { + return nil, fmt.Errorf("start workspace: %w", err) + } + workspaceLogEnd, err := WithTempWorkspaceRulesLog() + if err != nil { + return nil, fmt.Errorf("end workspace: %w", err) + } + // Get all target info for both VCS time points. - targets, err := startWorkspace.Query("deps(//...)", WithKeepGoing(), WithUnorderedOutput()) + startResults, err := startWorkspace.Query("deps(//...)", WithKeepGoing(), WithUnorderedOutput(), workspaceLogStart) if err != nil { return nil, fmt.Errorf("failed to query deps for start point: %w", err) } - targets, err = endWorkspace.Query("deps(//...)", WithKeepGoing(), WithUnorderedOutput()) + endResults, err := endWorkspace.Query("deps(//...)", WithKeepGoing(), WithUnorderedOutput(), workspaceLogEnd) if err != nil { return nil, fmt.Errorf("failed to query deps for end point: %w", err) } + fmt.Fprintf(os.Stderr, "Before results:\tTargets: %d\tWorkspace events: %d\n", len(startResults.Targets), len(startResults.WorkspaceEvents)) + fmt.Fprintf(os.Stderr, "After results:\tTargets: %d\tWorkspace events: %d\n", len(endResults.Targets), len(endResults.WorkspaceEvents)) + // TODO(scott): Implement diffing of returned targets - targets = targets + startResults = startResults + endResults = endResults return nil, fmt.Errorf("not implemented") } diff --git a/lib/bazel/commands/commands.go b/lib/bazel/commands/commands.go index 79b7aaffa..203c091fe 100644 --- a/lib/bazel/commands/commands.go +++ b/lib/bazel/commands/commands.go @@ -114,7 +114,7 @@ func (c *AffectedTargetsList) Run(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("can't generate worktree for committish %q: %w", c.parent.End, err) } - defer endTree.Close() + //defer endTree.Close() endTreePath = endTree.Root() } endWS := filepath.Clean(filepath.Join(endTreePath, gitToBazelPath)) diff --git a/lib/bazel/options.go b/lib/bazel/options.go index b8ae79fdb..f939a431e 100644 --- a/lib/bazel/options.go +++ b/lib/bazel/options.go @@ -1,7 +1,13 @@ package bazel import ( + "errors" + "fmt" + "io/ioutil" + "os" "os/exec" + + "github.com/enfabrica/enkit/lib/multierror" ) // subcommand is implemented by an arguments struct for each bazel subcommand @@ -51,6 +57,7 @@ type queryOptions struct { keepGoing bool unorderedOutput bool + workspaceLog *os.File } // Args returns the `query` and relevant subcommand arguments as passed to bazel. @@ -62,6 +69,11 @@ func (o *queryOptions) Args() []string { if o.unorderedOutput { f = append(f, "--order_output=no") } + if o.workspaceLog != nil { + // See https://github.com/bazelbuild/bazel/issues/6807 for tracking issue + // making this flag non-experimental + f = append(f, "--experimental_workspace_rules_log_file", o.workspaceLog.Name()) + } f = append(f, "--", o.query) return f } @@ -73,10 +85,11 @@ func (o *queryOptions) filterError(err error) error { return nil } - if err, ok := err.(*exec.ExitError); ok { + var execErr *exec.ExitError + if errors.As(err, &execErr) { // PARTIAL_ANALYSIS_FAILURE is expected when --keep_going is passed // https://github.com/bazelbuild/bazel/blob/86409b7a248d1cb966268451f9aa4db0763c3eb2/src/main/java/com/google/devtools/build/lib/util/ExitCode.java#L38 - if err.ExitCode() == 3 { + if execErr.ExitCode() == 3 { return nil } } @@ -84,6 +97,15 @@ func (o *queryOptions) filterError(err error) error { return err } +func (o *queryOptions) Close() error { + var errs []error + if o.workspaceLog != nil { + errs = append(errs, o.workspaceLog.Close()) + errs = append(errs, os.RemoveAll(o.workspaceLog.Name())) + } + return multierror.New(errs) +} + // QueryOption modifies bazel query subcommand flags. type QueryOption func(*queryOptions) @@ -103,6 +125,17 @@ func WithUnorderedOutput() QueryOption { } } +func WithTempWorkspaceRulesLog() (QueryOption, error) { + f, err := ioutil.TempFile("", "bazel_workspace_log_*.pb") + if err != nil { + return nil, fmt.Errorf("failed to create temp workspace log file: %w", err) + } + + return func(o *queryOptions) { + o.workspaceLog = f + }, nil +} + // apply applies all the options to this option struct. func (opts QueryOptions) apply(o *queryOptions) { for _, opt := range opts { diff --git a/lib/bazel/options_test.go b/lib/bazel/options_test.go new file mode 100644 index 000000000..402685514 --- /dev/null +++ b/lib/bazel/options_test.go @@ -0,0 +1,26 @@ +package bazel + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestQueryOptionsArgs(t *testing.T) { + tempLog, err := WithTempWorkspaceRulesLog() + assert.NoError(t, err) + opts := QueryOptions{ + WithKeepGoing(), + WithUnorderedOutput(), + tempLog, + } + opt := &queryOptions{} + opts.apply(opt) + got := opt.Args() + + assert.Contains(t, got, "query") + assert.Contains(t, got, "--output=streamed_proto") + assert.Contains(t, got, "--order_output=no") + assert.Contains(t, got, "--keep_going") + assert.Contains(t, got, "--experimental_workspace_rules_log_file") +} \ No newline at end of file diff --git a/lib/bazel/proto/BUILD.bazel b/lib/bazel/proto/BUILD.bazel index 4d0cf020c..476d6bdb2 100644 --- a/lib/bazel/proto/BUILD.bazel +++ b/lib/bazel/proto/BUILD.bazel @@ -35,3 +35,13 @@ go_proto_library( proto = ":workspace_log_proto", visibility = ["//lib/bazel:__subpackages__"], ) + +go_library( + name = "go_protos", + embed = [ + ":build_go_proto", + ":workspace_log_go_proto", + ], + importpath = "github.com/enfabrica/enkit/lib/bazel/proto", + visibility = ["//lib/bazel:__subpackages__"], +) diff --git a/lib/bazel/proto/workspace_log.proto b/lib/bazel/proto/workspace_log.proto index 6d7d00367..84dd6c492 100644 --- a/lib/bazel/proto/workspace_log.proto +++ b/lib/bazel/proto/workspace_log.proto @@ -18,7 +18,7 @@ // limitations under the License. syntax = "proto3"; -package lib.bazel.proto; +package blaze_query; // Information on "Execute" event in repository_ctx. message ExecuteEvent { diff --git a/lib/bazel/query.go b/lib/bazel/query.go index 8cb4268ae..55a29e350 100644 --- a/lib/bazel/query.go +++ b/lib/bazel/query.go @@ -3,6 +3,7 @@ package bazel import ( "fmt" "io" + "strings" "os/exec" bpb "github.com/enfabrica/enkit/lib/bazel/proto" @@ -40,19 +41,25 @@ var streamedBazelCommand = func(cmd *exec.Cmd) (io.Reader, chan error, error) { } err = cmd.Wait() if err != nil { - errChan <- err // Don't wrap, so raw ExitError can be picked up by the caller + errChan <- fmt.Errorf("command failed: `%s`: %w", strings.Join(cmd.Args, " "), err) } }() return pipeReader, errChan, nil } +type QueryResult struct { + Targets map[string]*bpb.Target + WorkspaceEvents []*bpb.WorkspaceEvent +} + // Query performs a `bazel query` using the provided query string. If // `keep_going` is set, then `--keep_going` is set on the bazel commandline, and // errors from the bazel process are ignored. -func (w *Workspace) Query(query string, options ...QueryOption) (map[string]*bpb.Target, error) { +func (w *Workspace) Query(query string, options ...QueryOption) (*QueryResult, error) { queryOpts := &queryOptions{query: query} QueryOptions(options).apply(queryOpts) + defer queryOpts.Close() cmd := w.bazelCommand(queryOpts) resultStream, errChan, err := streamedBazelCommand(cmd) @@ -79,7 +86,23 @@ func (w *Workspace) Query(query string, options ...QueryOption) (map[string]*bpb return nil, err } - return targets, nil + var workspaceEvents []*bpb.WorkspaceEvent + if queryOpts.workspaceLog != nil { + rdr := delimited.NewReader(queryOpts.workspaceLog) + var buf []byte + for buf, err = rdr.Next(); err == nil; buf, err = rdr.Next() { + var event bpb.WorkspaceEvent + if err := proto.Unmarshal(buf, &event); err != nil { + return nil, fmt.Errorf("failed to unmarshal WorkspaceEvent message: %w", err) + } + workspaceEvents = append(workspaceEvents, &event) + } + } + + return &QueryResult{ + Targets: targets, + WorkspaceEvents: workspaceEvents, + }, nil } // targetName returns the name of a Target message, which is part of a diff --git a/lib/bazel/query_test.go b/lib/bazel/query_test.go index 0fef95532..2962347a7 100644 --- a/lib/bazel/query_test.go +++ b/lib/bazel/query_test.go @@ -62,7 +62,7 @@ func TestQueryOutput(t *testing.T) { return } - assert.Equal(t, tc.wantCount, len(got)) + assert.Equal(t, tc.wantCount, len(got.Targets)) }) } } diff --git a/lib/git/worktree.go b/lib/git/worktree.go index 7697a1016..3537328be 100644 --- a/lib/git/worktree.go +++ b/lib/git/worktree.go @@ -31,11 +31,12 @@ func NewTempWorktree(repoPath string, committish string) (*TempWorktree, error) if err != nil { return nil, fmt.Errorf("failed to create temp directory: %w", err) } - cmd := exec.Command("git", "worktree", "add", tmpDir, committish) + // Command info: https://git-scm.com/docs/git-worktree + cmd := exec.Command("git", "worktree", "add", "--detach", tmpDir, committish) cmd.Dir = repoPath _, err = runCommand(cmd) if err != nil { - return nil, fmt.Errorf("failed to construct temp worktree: %w", err) + return nil, fmt.Errorf("failed to construct temp worktree with command %v: %w", cmd, err) } return &TempWorktree{ repoPath: repoPath, diff --git a/lib/git/worktree_test.go b/lib/git/worktree_test.go index b677f685c..b64b23207 100644 --- a/lib/git/worktree_test.go +++ b/lib/git/worktree_test.go @@ -22,8 +22,8 @@ func TestNewTempWorktree(t *testing.T) { assert.NoError(t, gotErr) assert.NoError(t, closeErr) assert.Equal(t, 2, len(gotCmds)) - assert.Equal(t, []string{"git", "worktree", "add"}, gotCmds[0].Args[0:3]) - assert.Equal(t, []string{"some_branch_name"}, gotCmds[0].Args[4:]) + assert.Equal(t, []string{"git", "worktree", "add", "--detach"}, gotCmds[0].Args[0:4]) + assert.Equal(t, []string{"some_branch_name"}, gotCmds[0].Args[5:]) assert.Equal(t, "/foo/bar", gotCmds[0].Dir) assert.Equal(t, []string{"git", "worktree", "remove"}, gotCmds[1].Args[0:3]) assert.Equal(t, "/foo/bar", gotCmds[1].Dir) diff --git a/machinist/testing/BUILD.bazel b/machinist/testing/BUILD.bazel index fdc515dc6..3e486a1d4 100644 --- a/machinist/testing/BUILD.bazel +++ b/machinist/testing/BUILD.bazel @@ -2,9 +2,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_test") go_test( name = "go_default_test", - srcs = [ - "machinist_e2e_test.go", - ], + srcs = ["machinist_e2e_test.go"], deps = [ "//lib/knetwork:go_default_library", "//lib/knetwork/kdns:go_default_library", @@ -13,7 +11,6 @@ go_test( "//machinist/machine:go_default_library", "//machinist/mserver:go_default_library", "//machinist/state:go_default_library", - "@com_github_miekg_dns//:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@org_golang_google_grpc//:go_default_library", "@org_golang_google_grpc//test/bufconn:go_default_library", From 0d234b06c52a135f92595db86eba3bebcb59145a Mon Sep 17 00:00:00 2001 From: Scott Minor Date: Wed, 20 Oct 2021 14:01:23 -0700 Subject: [PATCH 2/3] Remove debug info --- lib/bazel/affected_targets.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/bazel/affected_targets.go b/lib/bazel/affected_targets.go index f73e97fc9..cc9c4f2f0 100644 --- a/lib/bazel/affected_targets.go +++ b/lib/bazel/affected_targets.go @@ -73,9 +73,6 @@ func GetAffectedTargets(start string, end string) ([]string, error) { return nil, fmt.Errorf("failed to query deps for end point: %w", err) } - fmt.Fprintf(os.Stderr, "Before results:\tTargets: %d\tWorkspace events: %d\n", len(startResults.Targets), len(startResults.WorkspaceEvents)) - fmt.Fprintf(os.Stderr, "After results:\tTargets: %d\tWorkspace events: %d\n", len(endResults.Targets), len(endResults.WorkspaceEvents)) - // TODO(scott): Implement diffing of returned targets startResults = startResults endResults = endResults From 96177c954d1adb1a065a8ef841a23a185fd5ebb7 Mon Sep 17 00:00:00 2001 From: Scott Minor Date: Wed, 20 Oct 2021 14:55:33 -0700 Subject: [PATCH 3/3] Add comments to QueryResult --- lib/bazel/query.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/bazel/query.go b/lib/bazel/query.go index 55a29e350..b6c6921b9 100644 --- a/lib/bazel/query.go +++ b/lib/bazel/query.go @@ -48,8 +48,14 @@ var streamedBazelCommand = func(cmd *exec.Cmd) (io.Reader, chan error, error) { return pipeReader, errChan, nil } +// QueryResult contains the results of an arbitrary bazel query. type QueryResult struct { + // Targets is filled with a map of "target label" to target node. Targets map[string]*bpb.Target + + // If the WithTempWorkspaceRulesLog() option is passed, this contains a list + // of all the workspace events emitted during the bazel query. Otherwise, this + // is empty. WorkspaceEvents []*bpb.WorkspaceEvent }