From dc692af442ab965f4c1cdf5749a1e0f645f8c911 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Thu, 10 Oct 2024 14:50:51 +0100 Subject: [PATCH] Replace channel-based iterators with new-style ones (#3272) * Convert functions to iterators * Change things to use it * Convert this too * lint --- src/build/build_step.go | 4 +- src/build/incrementality.go | 12 +- src/core/state.go | 26 +-- src/core/utils.go | 334 +++++++++++++++++++----------------- src/core/utils_test.go | 17 +- src/query/graph.go | 4 +- src/run/run_step.go | 4 +- 7 files changed, 213 insertions(+), 188 deletions(-) diff --git a/src/build/build_step.go b/src/build/build_step.go index 0a6c6a328d..8a99d0eb92 100644 --- a/src/build/build_step.go +++ b/src/build/build_step.go @@ -596,8 +596,8 @@ func prepareDirectory(directory string, remove bool) error { // Symlinks the source files of this rule into its temp directory. func prepareSources(state *core.BuildState, graph *core.BuildGraph, target *core.BuildTarget) error { - for source := range core.IterSources(state, graph, target, false) { - if err := core.PrepareSourcePair(source); err != nil { + for src, tmp := range core.IterSources(state, graph, target, false) { + if err := core.PrepareSource(src, tmp); err != nil { return err } } diff --git a/src/build/incrementality.go b/src/build/incrementality.go index 23577568aa..92307cef7f 100644 --- a/src/build/incrementality.go +++ b/src/build/incrementality.go @@ -111,13 +111,13 @@ func mustSourceHash(state *core.BuildState, target *core.BuildTarget) []byte { // Calculate the hash of all sources of this rule func sourceHash(state *core.BuildState, target *core.BuildTarget) ([]byte, error) { h := sha1.New() - for source := range core.IterSources(state, state.Graph, target, false) { - result, err := state.PathHasher.Hash(source.Src, false, true, false) + for src := range core.IterSources(state, state.Graph, target, false) { + result, err := state.PathHasher.Hash(src, false, true, false) if err != nil { return nil, err } h.Write(result) - h.Write([]byte(source.Src)) + h.Write([]byte(src)) } for _, tool := range target.AllTools() { for _, path := range tool.FullPaths(state.Graph) { @@ -440,8 +440,8 @@ func RuntimeHash(state *core.BuildState, target *core.BuildTarget, testRun int) hash := append(RuleHash(state, target, true, false), RuleHash(state, target, true, true)...) hash = append(hash, state.Hashes.Config...) h := sha1.New() - for source := range core.IterRuntimeFiles(state.Graph, target, true, target.TestDir(testRun)) { - result, err := state.PathHasher.Hash(source.Src, false, true, false) + for src := range core.IterRuntimeFiles(state.Graph, target, true, target.TestDir(testRun)) { + result, err := state.PathHasher.Hash(src, false, true, false) if err != nil { return result, err } @@ -465,7 +465,7 @@ func PrintHashes(state *core.BuildState, target *core.BuildTarget) { // Note that the logic here mimics sourceHash, but I don't want to pollute that with // optional printing nonsense since it's on our hot path. for source := range core.IterSources(state, state.Graph, target, false) { - fmt.Printf(" Source: %s: %s\n", source.Src, b64(state.PathHasher.MustHash(source.Src, target.HashLastModified()))) + fmt.Printf(" Source: %s: %s\n", source, b64(state.PathHasher.MustHash(source, target.HashLastModified()))) } for _, tool := range target.AllTools() { if label, ok := tool.Label(); ok { diff --git a/src/core/state.go b/src/core/state.go index 7e5cb5eb00..f98cce26cc 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -9,6 +9,7 @@ import ( "hash/crc64" "io" iofs "io/fs" + "iter" "path/filepath" "sort" "strings" @@ -1329,23 +1330,26 @@ func (state *BuildState) DownloadAllInputs(target *BuildTarget, targetDir string return state.RemoteClient.DownloadInputs(target, targetDir, isTest) } -// IterInputs returns a channel that iterates all the input files needed for a target. -func (state *BuildState) IterInputs(target *BuildTarget, test bool) <-chan BuildInput { +// IterInputs returns a an iterator over all the input files needed for a target. +func (state *BuildState) IterInputs(target *BuildTarget, test bool) iter.Seq[BuildInput] { if !test { return IterInputs(state, state.Graph, target, true, target.IsFilegroup) } - ch := make(chan BuildInput) - go func() { - ch <- target.Label + return func(yield func(BuildInput) bool) { + if !yield(target.Label) { + return + } for _, datum := range target.AllData() { - ch <- datum + if !yield(datum) { + return + } } - for _, datum := range target.AllTestTools() { - ch <- datum + for _, tool := range target.AllTestTools() { + if !yield(tool) { + return + } } - close(ch) - }() - return ch + } } // DisableXattrs disables xattr support for this build. This is done for filesystems that diff --git a/src/core/utils.go b/src/core/utils.go index 1f9969be29..abc7998cff 100644 --- a/src/core/utils.go +++ b/src/core/utils.go @@ -4,6 +4,7 @@ import ( "bytes" "crypto/sha1" "fmt" + "iter" "os" "path/filepath" "strings" @@ -113,139 +114,155 @@ func ReturnToInitialWorkingDir() { } } -// A SourcePair represents a source file with its source and temporary locations. -// This isn't typically used much by callers; it's just useful to have a single type for channels. -type SourcePair struct{ Src, Tmp string } - // IterSources returns all the sources for a function, allowing for sources that are other rules // and rules that require transitive dependencies. // Yielded values are pairs of the original source location and its temporary location for this rule. // If includeTools is true it yields the target's tools as well. -func IterSources(state *BuildState, graph *BuildGraph, target *BuildTarget, includeTools bool) <-chan SourcePair { - ch := make(chan SourcePair) - done := map[string]bool{} - tmpDir := target.TmpDir() - go func() { +func IterSources(state *BuildState, graph *BuildGraph, target *BuildTarget, includeTools bool) iter.Seq2[string, string] { + return func(yield func(string, string) bool) { + done := map[string]bool{} + tmpDir := target.TmpDir() for input := range IterInputs(state, graph, target, includeTools, false) { fullPaths := input.FullPaths(graph) for i, sourcePath := range input.Paths(graph) { if tmpPath := filepath.Join(tmpDir, sourcePath); !done[tmpPath] { - ch <- SourcePair{fullPaths[i], tmpPath} + if !yield(fullPaths[i], tmpPath) { + return + } done[tmpPath] = true } } } - close(ch) - }() - return ch + } } // IterInputs iterates all the inputs for a target. -func IterInputs(state *BuildState, graph *BuildGraph, target *BuildTarget, includeTools, sourcesOnly bool) <-chan BuildInput { - ch := make(chan BuildInput) - done := map[BuildLabel]bool{} - var inner func(dependency *BuildTarget) - inner = func(dependency *BuildTarget) { - if dependency != target { - ch <- dependency.Label +func IterInputs(state *BuildState, graph *BuildGraph, target *BuildTarget, includeTools, sourcesOnly bool) iter.Seq[BuildInput] { + return func(yield func(BuildInput) bool) { + done := map[BuildLabel]bool{} + recursivelyProvideSource := func(target *BuildTarget, src BuildInput) bool { + if label, ok := src.nonOutputLabel(); ok { + for p := range recursivelyProvideFor(graph, target, target, label) { + if !yield(p) { + return false + } + } + return true + } + return yield(src) } + var inner func(dependency *BuildTarget) bool + inner = func(dependency *BuildTarget) bool { + if dependency != target { + if !yield(dependency.Label) { + return false + } + } - done[dependency.Label] = true - if target == dependency || (target.NeedsTransitiveDependencies && !dependency.OutputIsComplete) { - for _, dep := range dependency.BuildDependencies() { - for _, dep2 := range recursivelyProvideFor(graph, target, dependency, dep.Label) { - if !done[dep2] && !dependency.IsTool(dep2) { - inner(graph.TargetOrDie(dep2)) + done[dependency.Label] = true + if target == dependency || (target.NeedsTransitiveDependencies && !dependency.OutputIsComplete) { + for _, dep := range dependency.BuildDependencies() { + for dep2 := range recursivelyProvideFor(graph, target, dependency, dep.Label) { + if !done[dep2] && !dependency.IsTool(dep2) { + if !inner(graph.TargetOrDie(dep2)) { + return false + } + } } } - } - } else { - for _, dep := range dependency.ExportedDependencies() { - for _, dep2 := range recursivelyProvideFor(graph, target, dependency, dep) { - if !done[dep2] { - inner(graph.TargetOrDie(dep2)) + } else { + for _, dep := range dependency.ExportedDependencies() { + for dep2 := range recursivelyProvideFor(graph, target, dependency, dep) { + if !done[dep2] { + if !inner(graph.TargetOrDie(dep2)) { + return false + } + } } } } + return true } - } - go func() { for _, source := range target.AllSources() { - recursivelyProvideSource(graph, target, source, ch) + if !recursivelyProvideSource(target, source) { + return + } } if includeTools { for _, tool := range target.AllTools() { - recursivelyProvideSource(graph, target, tool, ch) + if !recursivelyProvideSource(target, tool) { + return + } } } if !sourcesOnly { inner(target) } - close(ch) - }() - return ch + } } // recursivelyProvideFor recursively applies ProvideFor to a target. -func recursivelyProvideFor(graph *BuildGraph, target, dependency *BuildTarget, dep BuildLabel) []BuildLabel { - depTarget := graph.TargetOrDie(dep) - ret := depTarget.ProvideFor(dependency) - if len(ret) == 1 && ret[0] == dep { - // Dependency doesn't have a require/provide directly on this guy, up to the top-level - // target. We have to check the dep first to keep things consistent with what targets - // have actually been built. - ret = depTarget.ProvideFor(target) +func recursivelyProvideFor(graph *BuildGraph, target, dependency *BuildTarget, dep BuildLabel) iter.Seq[BuildLabel] { + return func(yield func(BuildLabel) bool) { + depTarget := graph.TargetOrDie(dep) + ret := depTarget.ProvideFor(dependency) if len(ret) == 1 && ret[0] == dep { - return ret - } - } - ret2 := make([]BuildLabel, 0, len(ret)) - for _, r := range ret { - if r == dep { - ret2 = append(ret2, r) // Providing itself, don't recurse - } else { - ret2 = append(ret2, recursivelyProvideFor(graph, target, dependency, r)...) + // Dependency doesn't have a require/provide directly on this guy, up to the top-level + // target. We have to check the dep first to keep things consistent with what targets + // have actually been built. + ret = depTarget.ProvideFor(target) + if len(ret) == 1 && ret[0] == dep { + yield(ret[0]) + return + } } - } - return ret2 -} - -// recursivelyProvideSource is similar to recursivelyProvideFor but operates on a BuildInput. -func recursivelyProvideSource(graph *BuildGraph, target *BuildTarget, src BuildInput, ch chan BuildInput) { - if label, ok := src.nonOutputLabel(); ok { - for _, p := range recursivelyProvideFor(graph, target, target, label) { - ch <- p + for _, r := range ret { + if r == dep { + if !yield(r) { // Providing itself, don't recurse + return + } + } else { + for p := range recursivelyProvideFor(graph, target, dependency, r) { + if !yield(p) { + return + } + } + } } - return } - ch <- src } // IterRuntimeFiles yields all the runtime files for a rule (outputs, tools & data files), similar to above. -func IterRuntimeFiles(graph *BuildGraph, target *BuildTarget, absoluteOuts bool, runtimeDir string) <-chan SourcePair { - done := map[string]bool{} - ch := make(chan SourcePair) +func IterRuntimeFiles(graph *BuildGraph, target *BuildTarget, absoluteOuts bool, runtimeDir string) iter.Seq2[string, string] { + return func(yield func(string, string) bool) { + done := map[string]bool{} - pushOut := func(src, out string) { - if absoluteOuts { - out = filepath.Join(RepoRoot, runtimeDir, out) - } - if !done[out] { - ch <- SourcePair{src, out} - done[out] = true + pushOut := func(src, out string) bool { + if absoluteOuts { + out = filepath.Join(RepoRoot, runtimeDir, out) + } + if !done[out] { + done[out] = true + if !yield(src, out) { + return false + } + } + return true } - } - go func() { outDir := target.OutDir() for _, out := range target.Outputs() { - pushOut(filepath.Join(outDir, out), out) + if !pushOut(filepath.Join(outDir, out), out) { + return + } } for _, data := range target.AllData() { fullPaths := data.FullPaths(graph) for i, dataPath := range data.Paths(graph) { - pushOut(fullPaths[i], dataPath) + if !pushOut(fullPaths[i], dataPath) { + return + } } } @@ -253,7 +270,9 @@ func IterRuntimeFiles(graph *BuildGraph, target *BuildTarget, absoluteOuts bool, for _, tool := range target.AllTestTools() { fullPaths := tool.FullPaths(graph) for i, toolPath := range tool.Paths(graph) { - pushOut(fullPaths[i], toolPath) + if !pushOut(fullPaths[i], toolPath) { + return + } } } } @@ -262,96 +281,109 @@ func IterRuntimeFiles(graph *BuildGraph, target *BuildTarget, absoluteOuts bool, for _, data := range target.AllDebugData() { fullPaths := data.FullPaths(graph) for i, dataPath := range data.Paths(graph) { - pushOut(fullPaths[i], dataPath) + if !pushOut(fullPaths[i], dataPath) { + return + } } } for _, tool := range target.AllDebugTools() { fullPaths := tool.FullPaths(graph) for i, toolPath := range tool.Paths(graph) { - pushOut(fullPaths[i], toolPath) + if !pushOut(fullPaths[i], toolPath) { + return + } } } } - close(ch) - }() - return ch + } } // IterInputPaths yields all the transitive input files for a rule (sources & data files), similar to above (again). -func IterInputPaths(graph *BuildGraph, target *BuildTarget) <-chan string { - // Use a couple of maps to protect us from dep-graph loops and to stop parsing the same target - // multiple times. We also only want to push files to the channel that it has not already seen. - donePaths := map[string]bool{} - doneTargets := map[*BuildTarget]bool{} - ch := make(chan string) - var inner func(*BuildTarget) - inner = func(target *BuildTarget) { - if !doneTargets[target] { - // First yield all the sources of the target only ever pushing declared paths to - // the channel to prevent us outputting any intermediate files. - for _, source := range target.AllSources() { - // If the label is nil add any input paths contained here. - if label, ok := source.nonOutputLabel(); !ok { - // Don't emit inputs from subrepos; they appear to be files in many ways but they are themselves generated - if _, ok := source.(SubrepoFileLabel); ok { - continue - } - for _, sourcePath := range source.FullPaths(graph) { - if !donePaths[sourcePath] { - ch <- sourcePath - donePaths[sourcePath] = true +func IterInputPaths(graph *BuildGraph, target *BuildTarget) iter.Seq[string] { + return func(yield func(string) bool) { + // Use a couple of maps to protect us from dep-graph loops and to stop parsing the same target + // multiple times. We also only want to push files to the channel that it has not already seen. + donePaths := map[string]bool{} + doneTargets := map[*BuildTarget]bool{} + var inner func(*BuildTarget) bool + inner = func(target *BuildTarget) bool { + if !doneTargets[target] { + // First yield all the sources of the target only ever pushing declared paths to + // the channel to prevent us outputting any intermediate files. + for _, source := range target.AllSources() { + // If the label is nil add any input paths contained here. + if label, ok := source.nonOutputLabel(); !ok { + // Don't emit inputs from subrepos; they appear to be files in many ways but they are themselves generated + if _, ok := source.(SubrepoFileLabel); ok { + continue + } + for _, sourcePath := range source.FullPaths(graph) { + if !donePaths[sourcePath] { + if !yield(sourcePath) { + return false + } + donePaths[sourcePath] = true + } + } + // Otherwise we should recurse for this build label (and gather its sources) + } else { + t := graph.TargetOrDie(label) + for d := range recursivelyProvideFor(graph, target, t, t.Label) { + if !inner(graph.TargetOrDie(d)) { + return false + } } - } - // Otherwise we should recurse for this build label (and gather its sources) - } else { - t := graph.TargetOrDie(label) - for _, d := range recursivelyProvideFor(graph, target, t, t.Label) { - inner(graph.TargetOrDie(d)) } } - } - // Now yield all the data deps of this rule. - for _, data := range target.AllData() { - // If the label is nil add any input paths contained here. - if label, ok := data.Label(); !ok { - // Don't emit inputs from subrepos; they appear to be files in many ways but they are themselves generated - if _, ok := data.(SubrepoFileLabel); ok { - continue - } - for _, sourcePath := range data.FullPaths(graph) { - if !donePaths[sourcePath] { - ch <- sourcePath - donePaths[sourcePath] = true + // Now yield all the data deps of this rule. + for _, data := range target.AllData() { + // If the label is nil add any input paths contained here. + if label, ok := data.Label(); !ok { + // Don't emit inputs from subrepos; they appear to be files in many ways but they are themselves generated + if _, ok := data.(SubrepoFileLabel); ok { + continue + } + for _, sourcePath := range data.FullPaths(graph) { + if !donePaths[sourcePath] { + if !yield(sourcePath) { + return false + } + donePaths[sourcePath] = true + } + } + // Otherwise we should recurse for this build label (and gather its sources) + } else { + t := graph.TargetOrDie(label) + for d := range recursivelyProvideFor(graph, target, t, t.Label) { + if !inner(graph.TargetOrDie(d)) { + return false + } } - } - // Otherwise we should recurse for this build label (and gather its sources) - } else { - t := graph.TargetOrDie(label) - for _, d := range recursivelyProvideFor(graph, target, t, t.Label) { - inner(graph.TargetOrDie(d)) } } - } - // Finally recurse for all the deps of this rule. - for _, dep := range target.Dependencies() { - for _, d := range recursivelyProvideFor(graph, target, dep, dep.Label) { - inner(graph.TargetOrDie(d)) + // Finally recurse for all the deps of this rule. + for _, dep := range target.Dependencies() { + for d := range recursivelyProvideFor(graph, target, dep, dep.Label) { + if !inner(graph.TargetOrDie(d)) { + return false + } + } } + doneTargets[target] = true } - doneTargets[target] = true + return true } - } - go func() { inner(target) - close(ch) - }() - return ch + } } // PrepareSource symlinks a single source file for a build rule. func PrepareSource(sourcePath string, tmpPath string) error { + if !filepath.IsAbs(sourcePath) { + sourcePath = filepath.Join(RepoRoot, sourcePath) + } dir := filepath.Dir(tmpPath) if !PathExists(dir) { if err := os.MkdirAll(dir, DirPermissions); err != nil { @@ -364,14 +396,6 @@ func PrepareSource(sourcePath string, tmpPath string) error { return fs.RecursiveLink(sourcePath, tmpPath) } -// PrepareSourcePair prepares a source file for a build. -func PrepareSourcePair(pair SourcePair) error { - if filepath.IsAbs(pair.Src) { - return PrepareSource(pair.Src, pair.Tmp) - } - return PrepareSource(filepath.Join(RepoRoot, pair.Src), pair.Tmp) -} - // PrepareRuntimeDir prepares a directory with a target's runtime data for a command to be run on. func PrepareRuntimeDir(state *BuildState, target *BuildTarget, dir string) error { if err := fs.RemoveAll(dir); err != nil { @@ -386,8 +410,8 @@ func PrepareRuntimeDir(state *BuildState, target *BuildTarget, dir string) error return err } - for out := range IterRuntimeFiles(state.Graph, target, true, dir) { - if err := PrepareSourcePair(out); err != nil { + for src, tmp := range IterRuntimeFiles(state.Graph, target, true, dir) { + if err := PrepareSource(src, tmp); err != nil { return err } } diff --git a/src/core/utils_test.go b/src/core/utils_test.go index aec9cf1b45..adfa07dd63 100644 --- a/src/core/utils_test.go +++ b/src/core/utils_test.go @@ -35,10 +35,15 @@ func TestCollapseHash2(t *testing.T) { func TestIterSources(t *testing.T) { state := NewDefaultBuildState() - graph := buildGraph() + + type SourcePair struct{ Src, Tmp string } iterSources := func(label string) []SourcePair { - return toSlice(IterSources(state, graph, graph.TargetOrDie(ParseBuildLabel(label, "")), false)) + ret := []SourcePair{} + for src, tmp := range IterSources(state, graph, graph.TargetOrDie(ParseBuildLabel(label, "")), false) { + ret = append(ret, SourcePair{src, tmp}) + } + return ret } assert.Equal(t, []SourcePair{ @@ -176,11 +181,3 @@ func makeTarget4(graph *BuildGraph, label string, deps ...string) *BuildTarget { target.AddOutput(target.Label.Name + ".a") return target } - -func toSlice(ch <-chan SourcePair) []SourcePair { - ret := []SourcePair{} - for x := range ch { - ret = append(ret, x) - } - return ret -} diff --git a/src/query/graph.go b/src/query/graph.go index e15356b317..4b24517827 100644 --- a/src/query/graph.go +++ b/src/query/graph.go @@ -152,7 +152,7 @@ func makeJSONTarget(state *core.BuildState, target *core.BuildTarget) JSONTarget Tools: makeJSONInputField(state.Graph, buildInputsToStrings(state.Graph, target.AllTools()), target.AllNamedTools()), } for in := range core.IterSources(state, state.Graph, target, false) { - t.Inputs = append(t.Inputs, in.Src) + t.Inputs = append(t.Inputs, in) } for _, out := range target.Outputs() { t.Outputs = append(t.Outputs, filepath.Join(target.Label.PackageName, out)) @@ -162,7 +162,7 @@ func makeJSONTarget(state *core.BuildState, target *core.BuildTarget) JSONTarget } // just use run 1 as this is only used to print the test dir for data := range core.IterRuntimeFiles(state.Graph, target, false, target.TestDir(1)) { - t.Data = append(t.Data, data.Src) + t.Data = append(t.Data, data) } t.Labels = target.Labels t.Requires = target.Requires diff --git a/src/run/run_step.go b/src/run/run_step.go index f4055ae392..8c471997d6 100644 --- a/src/run/run_step.go +++ b/src/run/run_step.go @@ -217,8 +217,8 @@ func prepareRunDir(state *core.BuildState, target *core.BuildTarget) (string, er return "", err } - for out := range core.IterRuntimeFiles(state.Graph, target, true, path) { - if err := core.PrepareSourcePair(out); err != nil { + for src, tmp := range core.IterRuntimeFiles(state.Graph, target, true, path) { + if err := core.PrepareSource(src, tmp); err != nil { return "", err } }