diff --git a/ChangeLog b/ChangeLog index 79676000a..d0c67523c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +Version 17.35.1 +--------------- + * Fix labels on plugin_repo build def (#3651) + * Fix for remote execution filegroups with duplicate directories which are now correctly merged (#3655) + Version 17.35.0 --------------- * New `AdditionalCACerts` config option to work with intercepting `HTTPProxy` (#3639) diff --git a/VERSION b/VERSION index f4bb77f39..6d87ffe5f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -17.35.0 +17.35.1 diff --git a/src/remote/action.go b/src/remote/action.go index 248a25177..98d8036b1 100644 --- a/src/remote/action.go +++ b/src/remote/action.go @@ -186,7 +186,7 @@ func (c *Client) uploadInputs(ch chan<- *uploadinfo.Entry, target *core.BuildTar if err != nil { return nil, err } - return b.Build(ch), nil + return b.Build(ch) } // uploadInputDir uploads the inputs to the build rule. It returns an un-finalised directory builder representing the diff --git a/src/remote/filegroup.go b/src/remote/filegroup.go index 596c6583e..7dc65dafa 100644 --- a/src/remote/filegroup.go +++ b/src/remote/filegroup.go @@ -2,6 +2,7 @@ package remote import ( "bytes" + "cmp" "context" "encoding/hex" "fmt" @@ -48,8 +49,15 @@ func (c *Client) buildFilegroup(target *core.BuildTarget) (*core.BuildMetadata, } } // Sort the outputs so the hash doesn't depend on the order they were collected in. + // Directories can have the same name if several dependencies output one (they're merged by anything + // consuming them) so we have to sort on their digests too. slices.SortFunc(outs.Files, func(a, b *pb.FileNode) int { return strings.Compare(a.Name, b.Name) }) - slices.SortFunc(outs.Directories, func(a, b *pb.DirectoryNode) int { return strings.Compare(a.Name, b.Name) }) + slices.SortFunc(outs.Directories, func(a, b *pb.DirectoryNode) int { + return cmp.Or(strings.Compare(a.Name, b.Name), strings.Compare(a.Digest.GetHash(), b.Digest.GetHash())) + }) + outs.Directories = slices.CompactFunc(outs.Directories, func(a, b *pb.DirectoryNode) bool { + return a.Name == b.Name && a.Digest.GetHash() == b.Digest.GetHash() + }) slices.SortFunc(outs.Symlinks, func(a, b *pb.SymlinkNode) int { return strings.Compare(a.Name, b.Name) }) tree := &pb.Tree{Root: outs} b, err := proto.Marshal(tree) @@ -97,8 +105,8 @@ func (c *Client) addFilegroupSources(outs *pb.Directory, target *core.BuildTarge } // This is needed for the protos of any source directories; it's a bit wasteful for the ones // above them, but they're small and this isn't usually many files. - b.Build(ch) - return nil + _, err := b.Build(ch) + return err }); err != nil { return err } diff --git a/src/remote/filegroup_test.go b/src/remote/filegroup_test.go index 1c85d0a3b..0b98340f4 100644 --- a/src/remote/filegroup_test.go +++ b/src/remote/filegroup_test.go @@ -2,8 +2,11 @@ package remote import ( "encoding/hex" + "fmt" "os" "path/filepath" + "slices" + "strings" "testing" "github.com/bazelbuild/remote-apis-sdks/go/pkg/digest" @@ -169,7 +172,8 @@ func TestFilegroupConsumerInputRoot(t *testing.T) { c.state.Graph.AddTarget(consumer) b, err := c.uploadInputDir(nil, consumer, false) require.NoError(t, err) - b.Build(nil) + _, err = b.Build(nil) + require.NoError(t, err) // The filegroup's outputs appear in its package, not the package of its dependency. assert.Nil(t, b.dirs["package2"]) @@ -473,3 +477,182 @@ func assertFileContent(t *testing.T, filename string, expected []byte) { assert.Equal(t, expected, b) } } + +// putDirTree stores a tree of Directory protos containing the given files (keyed by slash-separated +// path) on the test server, and returns the root Directory and all its descendants. +func putDirTree(files map[string][]byte) (*pb.Directory, []*pb.Directory) { + dirs := map[string]*pb.Directory{".": {}} + var mkdir func(name string) *pb.Directory + mkdir = func(name string) *pb.Directory { + if d, present := dirs[name]; present { + return d + } + d := &pb.Directory{} + dirs[name] = d + mkdir(filepath.Dir(name)) + return d + } + for name, content := range files { + d := mkdir(filepath.Dir(name)) + d.Files = append(d.Files, &pb.FileNode{Name: filepath.Base(name), Digest: putBlob(content)}) + } + // Link up the directories deepest first, so each one is complete before its digest is taken. + names := make([]string, 0, len(dirs)) + for name := range dirs { + if name != "." { + names = append(names, name) + } + } + slices.SortFunc(names, func(a, b string) int { return strings.Count(b, "/") - strings.Count(a, "/") }) + children := make([]*pb.Directory, 0, len(names)) + for _, name := range names { + parent := dirs[filepath.Dir(name)] + parent.Directories = append(parent.Directories, &pb.DirectoryNode{Name: filepath.Base(name), Digest: putProto(dirs[name])}) + children = append(children, dirs[name]) + } + return dirs["."], children +} + +// buildRemoteTarget builds a target remotely with the given output files (keyed by path) and output +// directories (keyed by path, each containing files keyed by their path within it). +func buildRemoteTarget(t *testing.T, c *Client, label core.BuildLabel, files map[string][]byte, dirs map[string]map[string][]byte) *core.BuildTarget { + t.Helper() + ar := &pb.ActionResult{ + ExecutionMetadata: &pb.ExecutedActionMetadata{ + QueuedTimestamp: timestamppb.Now(), + ExecutionStartTimestamp: timestamppb.Now(), + ExecutionCompletedTimestamp: timestamppb.Now(), + }, + } + target := core.NewBuildTarget(label) + for name, content := range files { + ar.OutputFiles = append(ar.OutputFiles, &pb.OutputFile{Path: name, Digest: putBlob(content)}) + target.AddOutput(name) + } + for name, contents := range dirs { + root, children := putDirTree(contents) + ar.OutputDirectories = append(ar.OutputDirectories, &pb.OutputDirectory{ + Path: name, + TreeDigest: putProto(&pb.Tree{Root: root, Children: children}), + }) + target.AddOutput(name) + } + server.mockActionResult = ar + target.Command = "echo " + label.String() // Makes the action unique to this target. + c.state.Graph.AddTarget(target) + _, err := c.Build(target) + require.NoError(t, err) + target.SetState(core.BuiltRemotely) + return target +} + +// resolveFile finds a file in a built input root by its path. Directories known only by digest are read +// from the test server; the builder's own ones (which haven't been uploaded) are taken from it. +func resolveFile(t *testing.T, b *dirBuilder, path string) *pb.FileNode { + t.Helper() + parts := strings.Split(path, "/") + dir := b.root + for i, part := range parts[:len(parts)-1] { + d := findDir(dir, part) + if d == nil { + return nil + } + dirName := strings.Join(parts[:i+1], "/") + if built, present := b.dirs[dirName]; present && d.Digest.Hash == b.c.digestMessage(built).Hash { + dir = built + continue + } + blob, present := server.blobs[d.Digest.Hash] + require.True(t, present, "directory %s is neither in the builder nor the CAS", dirName) + dir = &pb.Directory{} + require.NoError(t, proto.Unmarshal(blob, dir)) + } + return findFile(dir, parts[len(parts)-1]) +} + +// assertUniqueNames asserts that no directory in the builder has two subdirectories with the same name. +func assertUniqueNames(t *testing.T, b *dirBuilder) { + t.Helper() + for name, dir := range b.dirs { + seen := map[string]bool{} + for _, d := range dir.Directories { + assert.False(t, seen[d.Name], "duplicate directory %s in %s", d.Name, name) + seen[d.Name] = true + } + } +} + +// assertInputFile asserts that the given file exists in a built input root with the given contents. +func assertInputFile(t *testing.T, b *dirBuilder, path string, content []byte) { + t.Helper() + if f := resolveFile(t, b, path); assert.NotNil(t, f, "%s should be in the input root", path) { + assert.Equal(t, digest.NewFromBlob(content).Hash, f.Digest.Hash) + } +} + +func TestFilegroupMergesDirectoriesWithTheSameName(t *testing.T) { + defer server.Reset() + c := newClientInstance("mock") + annotations := []byte("annotations.proto") + http := []byte("http.proto") + a := buildRemoteTarget(t, c, core.BuildLabel{PackageName: "package3", Name: "fg_merge_a"}, nil, map[string]map[string][]byte{ + "google": {"api/annotations.proto": annotations}, + }) + h := buildRemoteTarget(t, c, core.BuildLabel{PackageName: "package3", Name: "fg_merge_h"}, nil, map[string]map[string][]byte{ + "google": {"api/http.proto": http}, + }) + fg := newFilegroup(c, core.BuildLabel{PackageName: "package3", Name: "fg_merge"}, a.Label, h.Label) + _, err := c.Build(fg) + require.NoError(t, err) + + consumer := core.NewBuildTarget(core.BuildLabel{PackageName: "consumer", Name: "fg_merge_consumer"}) + consumer.AddSource(fg.Label) + consumer.AddOutput("consumer.txt") + c.state.Graph.AddTarget(consumer) + b, err := c.uploadInputDir(nil, consumer, false) + require.NoError(t, err) + _, err = b.Build(nil) + require.NoError(t, err) + + assertUniqueNames(t, b) + assertInputFile(t, b, "package3/google/api/annotations.proto", annotations) + assertInputFile(t, b, "package3/google/api/http.proto", http) +} + +func TestInputRootMergesOutputDirectoryWithOutputFiles(t *testing.T) { + // This mimics a proto_library: one rule outputs .proto files individually and another outputs a + // directory of generated code under the same top-level directory. + for _, filesFirst := range []bool{true, false} { + t.Run(fmt.Sprintf("filesFirst=%v", filesFirst), func(t *testing.T) { + defer server.Reset() + c := newClientInstance("mock") + protoFile := []byte("annotations.proto") + jsFile := []byte("annotations_pb.js") + name := fmt.Sprintf("merge_files_%v", filesFirst) + files := buildRemoteTarget(t, c, core.BuildLabel{PackageName: "package4", Name: name + "_files"}, map[string][]byte{ + "google/api/annotations.proto": protoFile, + }, nil) + dir := buildRemoteTarget(t, c, core.BuildLabel{PackageName: "package4", Name: name + "_dir"}, nil, map[string]map[string][]byte{ + "google": {"api/annotations_pb.js": jsFile}, + }) + consumer := core.NewBuildTarget(core.BuildLabel{PackageName: "consumer", Name: name + "_consumer"}) + if filesFirst { + consumer.AddSource(files.Label) + consumer.AddSource(dir.Label) + } else { + consumer.AddSource(dir.Label) + consumer.AddSource(files.Label) + } + consumer.AddOutput("consumer.txt") + c.state.Graph.AddTarget(consumer) + b, err := c.uploadInputDir(nil, consumer, false) + require.NoError(t, err) + _, err = b.Build(nil) + require.NoError(t, err) + + assertUniqueNames(t, b) + assertInputFile(t, b, "package4/google/api/annotations.proto", protoFile) + assertInputFile(t, b, "package4/google/api/annotations_pb.js", jsFile) + }) + } +} diff --git a/src/remote/remote.go b/src/remote/remote.go index 65881d2cd..49b1dd78d 100644 --- a/src/remote/remote.go +++ b/src/remote/remote.go @@ -94,6 +94,11 @@ type Client struct { // This map is of effective type `map[*core.BuildTarget]*pendingDownload` downloads sync.Map + // Directory protos we've read from the CAS, keyed by their hash. They're immutable so are safe to cache. + // + // This map is of effective type `map[string]*pb.Directory` + directories sync.Map + // Server-sent cache properties maxBlobBatchSize int64 diff --git a/src/remote/utils.go b/src/remote/utils.go index ae6803794..62279548d 100644 --- a/src/remote/utils.go +++ b/src/remote/utils.go @@ -215,6 +215,19 @@ func (c *Client) setOutputDirectoryOuts(target *core.BuildTarget, actionResultFS return nil } +// readDirectory reads a Directory proto from the CAS, caching it for future calls. +func (c *Client) readDirectory(dg *pb.Digest) (*pb.Directory, error) { + if dir, present := c.directories.Load(dg.Hash); present { + return dir.(*pb.Directory), nil + } + dir := &pb.Directory{} + if _, err := c.client.ReadProto(context.Background(), digest.NewFromProtoUnvalidated(dg), dir); err != nil { + return nil, err + } + c.directories.Store(dg.Hash, dir) + return dir, nil +} + // maybeGetOutDir will get the output directory based on the directory provided. If there's no matching directory, this // will return an empty string indicating that that action output was not an output directory. func maybeGetOutDir(dir string, outDirs []core.OutputDirectory) core.OutputDirectory { @@ -443,6 +456,9 @@ type dirBuilder struct { c *Client root *pb.Directory dirs map[string]*pb.Directory + // The digests we've calculated for directories in dirs, so we can tell them apart from directories + // that were added by digest if we're built more than once. + digests map[string]string } func newDirBuilder(c *Client) *dirBuilder { @@ -452,8 +468,9 @@ func newDirBuilder(c *Client) *dirBuilder { ".": root, // Ensure the root is in there "": root, // Some things might try to name it this way }, - root: root, - c: c, + digests: map[string]string{}, + root: root, + c: c, } } @@ -485,10 +502,16 @@ func (b *dirBuilder) dir(dir, child string) *pb.Directory { // Build "builds" the directory. It calculates the digests of all the items in the directory tree, and returns the root // directory. If ch is non-nil, it will upload the directory protos to ch. Build doesn't upload any of the actual files // in the directory tree, just the protos. -func (b *dirBuilder) Build(ch chan<- *uploadinfo.Entry) *pb.Directory { +// +// Directories with the same name (e.g. output directories of two dependencies in the same package, or one +// alongside individual output files under the same directory) are merged together, which may require reading +// their Directory protos from the remote. +func (b *dirBuilder) Build(ch chan<- *uploadinfo.Entry) (*pb.Directory, error) { // Upload the directory structure - b.walk(".", ch) - return b.root + if _, err := b.walk(".", ch); err != nil { + return nil, err + } + return b.root, nil } // Node returns the file, directory or symlink corresponding to the given path (or nil for all if not found) @@ -498,51 +521,113 @@ func (b *dirBuilder) Node(name string) (*pb.DirectoryNode, *pb.FileNode, *pb.Sym // Walk walks the directory tree calculating the digest. If ch is non-nil, it will also upload the direcory protos. // Walk does not upload the actual files in the tree, just the tree structure. -func (b *dirBuilder) walk(name string, ch chan<- *uploadinfo.Entry) *pb.Digest { +func (b *dirBuilder) walk(name string, ch chan<- *uploadinfo.Entry) (*pb.Digest, error) { dir := b.dirs[name] + if err := b.mergeDirs(name, dir); err != nil { + return nil, err + } for _, d := range dir.Directories { - if d.Digest == nil { // It's not nil if we're reusing outputs from an earlier call. - d.Digest = b.walk(filepath.Join(name, d.Name), ch) + if d.Digest == nil { // It's not nil if it was added by digest (e.g. the output of another target) + dg, err := b.walk(filepath.Join(name, d.Name), ch) + if err != nil { + return nil, err + } + d.Digest = dg } } // The protocol requires that these are sorted into lexicographic order. Not all servers // necessarily care, but some do, and we should be compliant. - files := dir.Files - dirs := dir.Directories - syms := dir.Symlinks - slices.SortFunc(files, func(a, b *pb.FileNode) int { return strings.Compare(a.Name, b.Name) }) - slices.SortFunc(dirs, func(a, b *pb.DirectoryNode) int { return strings.Compare(a.Name, b.Name) }) - slices.SortFunc(syms, func(a, b *pb.SymlinkNode) int { return strings.Compare(a.Name, b.Name) }) - // Ensure there are not duplicates in these slices. - last := "" - dir.Files = files[:0] - for _, f := range files { - if f.Name != last { - dir.Files = append(dir.Files, f) - last = f.Name - } + dir.Files = sortUnique(dir.Files, (*pb.FileNode).GetName) + dir.Directories = sortUnique(dir.Directories, (*pb.DirectoryNode).GetName) + dir.Symlinks = sortUnique(dir.Symlinks, (*pb.SymlinkNode).GetName) + + entry, _ := uploadinfo.EntryFromProto(dir) + if ch != nil { + ch <- entry + } + b.digests[name] = entry.Digest.Hash + return entry.Digest.ToProto(), nil +} + +// mergeDirs ensures that each subdirectory of the given directory appears only once in it. +// A subdirectory needs merging if it's been added more than once with different digests, or if it was added by +// digest and also has contents added to it in the builder (e.g. via Dir). In that case its contents are expanded +// into the builder, and it's replaced by a single entry whose digest will be calculated when it's walked. +func (b *dirBuilder) mergeDirs(name string, dir *pb.Directory) error { + byName := make(map[string][]*pb.DirectoryNode, len(dir.Directories)) + for _, d := range dir.Directories { + byName[d.Name] = append(byName[d.Name], d) + } + if len(byName) == len(dir.Directories) && !b.hasExpandedDigestNodes(name, dir) { + return nil // Nothing to do; this is by far the most common case. } - dir.Directories = dirs[:0] - for _, d := range dirs { - if d.Name != last { - dir.Directories = append(dir.Directories, d) - last = d.Name + dirs := make([]*pb.DirectoryNode, 0, len(byName)) + for _, d := range dir.Directories { + nodes, present := byName[d.Name] + if !present { + continue // Already handled this name + } + delete(byName, d.Name) + path := filepath.Join(name, d.Name) + digests := b.unbuiltDigests(path, nodes) + _, inBuilder := b.dirs[path] + if len(digests) == 0 || (len(digests) == 1 && !inBuilder) { + dirs = append(dirs, nodes[0]) // Either entirely the builder's own, or one directory added multiple times + continue + } + sub, present := b.dirs[path] + if !present { + sub = &pb.Directory{} + b.dirs[path] = sub + } + for _, dg := range digests { + contents, err := b.c.readDirectory(dg) + if err != nil { + return fmt.Errorf("failed to read directory %s for merging: %w", path, err) + } + // Any subdirectories of these that need further merging are handled when we walk sub. + sub.Files = append(sub.Files, contents.Files...) + sub.Directories = append(sub.Directories, contents.Directories...) + sub.Symlinks = append(sub.Symlinks, contents.Symlinks...) } + delete(b.digests, path) // Its contents have changed, so it'll need to be walked again + dirs = append(dirs, &pb.DirectoryNode{Name: d.Name}) } - dir.Symlinks = syms[:0] - for _, s := range syms { - if s.Name != last { - dir.Symlinks = append(dir.Symlinks, s) - last = s.Name + dir.Directories = dirs + return nil +} + +// hasExpandedDigestNodes returns true if any subdirectory of the given directory was added by digest but also has +// contents in the builder. +func (b *dirBuilder) hasExpandedDigestNodes(name string, dir *pb.Directory) bool { + for _, d := range dir.Directories { + path := filepath.Join(name, d.Name) + if _, present := b.dirs[path]; present && d.Digest != nil && b.digests[path] != d.Digest.Hash { + return true } } + return false +} - entry, _ := uploadinfo.EntryFromProto(dir) - if ch != nil { - ch <- entry +// unbuiltDigests returns the distinct digests of the given nodes (which are all for the given path) that weren't +// calculated by this builder. +func (b *dirBuilder) unbuiltDigests(path string, nodes []*pb.DirectoryNode) []*pb.Digest { + var digests []*pb.Digest + for _, n := range nodes { + if n.Digest == nil || n.Digest.Hash == b.digests[path] { + continue + } else if !slices.ContainsFunc(digests, func(dg *pb.Digest) bool { return dg.Hash == n.Digest.Hash }) { + digests = append(digests, n.Digest) + } } - return entry.Digest.ToProto() + return digests +} + +// sortUnique sorts the given nodes by name and removes any with duplicate names (keeping the first of each). +func sortUnique[T any](nodes []T, name func(T) string) []T { + slices.SortStableFunc(nodes, func(a, b T) int { return strings.Compare(name(a), name(b)) }) + return slices.CompactFunc(nodes, func(a, b T) bool { return name(a) == name(b) }) } // convertPlatform converts the platform entries from the config into a Platform proto.