diff --git a/src/core/build_label.go b/src/core/build_label.go index f7e2755f6..7750a6d80 100644 --- a/src/core/build_label.go +++ b/src/core/build_label.go @@ -478,6 +478,15 @@ func HashBuildLabel(l BuildLabel) uint64 { return cmap.XXHashes(l.Subrepo, l.PackageName, l.Name) } +// PackageLabel returns a build label representing the enclosing package (i.e. //pkg:all). +func (label BuildLabel) PackageLabel() BuildLabel { + return BuildLabel{ + Subrepo: label.Subrepo, + PackageName: label.PackageName, + Name: "all", + } +} + // packageKey returns a key for this build label that only uses the subrepo and package parts. func (label BuildLabel) packageKey() packageKey { return packageKey{Name: label.PackageName, Subrepo: label.Subrepo} diff --git a/src/core/build_target.go b/src/core/build_target.go index 4ac6fd6a6..b49c53509 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -1596,18 +1596,25 @@ func (target *BuildTarget) getCommand(state *BuildState, commands map[string]str return highestCommand } -// AllBuildInputs returns all the inputs for this target. -func (target *BuildTarget) AllBuildInputs() []BuildInput { - srcs := target.AllSources() - data := target.AllData() - tools := target.AllTools() - - size := len(srcs) + len(data) + len(tools) - inputs := make([]BuildInput, 0, size) - inputs = append(inputs, srcs...) - inputs = append(inputs, data...) - inputs = append(inputs, tools...) - return inputs +// AllBuildInputs returns yields over the inputs for this target. +func (target *BuildTarget) AllBuildInputs() iter.Seq[BuildInput] { + return func(yield func(BuildInput) bool) { + for _, src := range target.AllSources() { + if !yield(src) { + return + } + } + for _, data := range target.AllData() { + if !yield(data) { + return + } + } + for _, tool := range target.AllTools() { + if !yield(tool) { + return + } + } + } } // AllSources returns all the sources of this rule. diff --git a/src/core/state.go b/src/core/state.go index 0c24158fe..1adc6907f 100644 --- a/src/core/state.go +++ b/src/core/state.go @@ -217,11 +217,6 @@ type BuildState struct { NeedDebugDeps bool // ParseMetadata is true if we want to store BUILD file metadata ParseMetadata bool - // ForceParseEntirePackage is true if we want to force parse and activate all targets in every visited - // package during the parse phase. This is used by the export operation to ensure that the entire - // dependency graph of visited packages is parsed upfront, including co-defined related targets - // and sibling targets. - ForceParseEntirePackage bool // initOnce is used to control loading the subrepo .plzconfig initOnce *sync.Once diff --git a/src/export/README.md b/src/export/README.md index 96a508b32..326adc98c 100644 --- a/src/export/README.md +++ b/src/export/README.md @@ -83,7 +83,7 @@ statement details: To support package-level trimming, the exporter must have access to metadata for adjacent targets that normally (e.g., during a standard build operation) would not be parsed or resolved. -For this purpose, Please utilizes an upfront-parsing model. When `ForceParseEntirePackage` is set to +For this purpose, Please utilizes an upfront-parsing model. When `ParseMetadata` is set to `true` during the parse phase, the interpreter synchronously parses and resolves all targets in any visited package upfront. This guarantees that all package-level statement and target metadata is fully populated and available, eliminating the need for any dynamic or lazy parsing during the diff --git a/src/export/export.go b/src/export/export.go index b5c403bea..bf3137eec 100644 --- a/src/export/export.go +++ b/src/export/export.go @@ -102,9 +102,8 @@ type baseExporter struct { // run specifies the main steps when running an export. func (be *baseExporter) run(targets core.BuildLabels) { - stop := make(chan struct{}) - go be.startMonitor(stop) - defer close(stop) + stop := be.startMonitor() + defer stop() be.exportRepoConfig() be.strategy.exportPreloaded() @@ -112,45 +111,59 @@ func (be *baseExporter) run(targets core.BuildLabels) { be.strategy.writePackageFiles() } -func (be *baseExporter) startMonitor(stop chan struct{}) { - // this total is meant to provide some general idea of the progress but in reality we might - // visit more than the targets currently in the build graph (those will have to be parsed - // adhoc). - total := len(be.state.Graph.AllTargets()) - startTime := time.Now() - isTerminal := cli.StdErrIsATerminal - - if isTerminal { - cli.CurrentBackend.SetPassthrough(false, 1, false) - defer cli.CurrentBackend.SetPassthrough(true, 1, false) - } +func (be *baseExporter) startMonitor() func() { + stop, done := make(chan struct{}), make(chan struct{}) + + go func() { + defer close(done) + + // this total is meant to provide some general idea of the progress but in reality we might + // visit more than the targets currently in the build graph (those will have to be parsed + // adhoc). + total := len(be.state.Graph.AllTargets()) + startTime := time.Now() + isTerminal := cli.StdErrIsATerminal + + if isTerminal { + cli.CurrentBackend.SetPassthrough(false, 1, false) + defer cli.CurrentBackend.SetPassthrough(true, 1, false) + } + + ticker := time.NewTicker(100 * time.Millisecond) + defer ticker.Stop() - ticker := time.NewTicker(100 * time.Millisecond) - defer ticker.Stop() - - for { - select { - case <-stop: - log.Infof("Exported %d targets (%.1fs)...", be.targetCounter, time.Since(startTime).Seconds()) - return - case <-ticker.C: - done := be.targetCounter - elapsed := time.Since(startTime).Seconds() - stats := be.state.SystemStats() - - if isTerminal { - logs := cli.CurrentBackend.FlushOutput() - if len(logs) > 0 { - // Remove footer line before writing logs. - cli.Fprintf(os.Stderr, "${RESETLN}") - for _, line := range logs { - fmt.Fprintln(os.Stderr, line) + for { + select { + case <-stop: + elapsed := time.Since(startTime).Seconds() + cli.Fprintf(os.Stderr, "${RESETLN}${BOLD_WHITE}Exported${RESET} ${BOLD_GREEN}%d${RESET} targets in ${BOLD_YELLOW}%.1fs${RESET}", + be.targetCounter, elapsed) + log.Infof("Exported %d targets (%.1fs)", be.targetCounter, time.Since(startTime).Seconds()) + return + case <-ticker.C: + done := be.targetCounter + elapsed := time.Since(startTime).Seconds() + stats := be.state.SystemStats() + + if isTerminal { + logs := cli.CurrentBackend.FlushOutput() + if len(logs) > 0 { + // Remove footer line before writing logs. + cli.Fprintf(os.Stderr, "${RESETLN}") + for _, line := range logs { + fmt.Fprintln(os.Stderr, line) + } } + cli.Fprintf(os.Stderr, "${RESETLN}${BOLD_WHITE}Exporting${RESET} [${BOLD_GREEN}%d${RESET}/${BOLD_GREEN}%d${RESET}, ${BOLD_YELLOW}%.1fs${RESET}]:: CPU: %5.1f%% Mem: %5.1f%% IO: %5.1f%%${RESET}", + done, total, elapsed, stats.CPU.Used, stats.Memory.UsedPercent, stats.CPU.IOWait) } - cli.Fprintf(os.Stderr, "${RESETLN}${BOLD_WHITE}Exporting${RESET} [${BOLD_GREEN}%d${RESET}/${BOLD_GREEN}%d${RESET}, ${BOLD_YELLOW}%.1fs${RESET}]:: CPU: %5.1f%% Mem: %5.1f%% IO: %5.1f%%${RESET}", - done, total, elapsed, stats.CPU.Used, stats.Memory.UsedPercent, stats.CPU.IOWait) } } + }() + + return func() { + close(stop) + <-done // Blocks until the monitor finishes the post instructions. } } @@ -184,7 +197,7 @@ func (be *baseExporter) exportTargets(labels core.BuildLabels) { } target, err := be.getTarget(l) if err != nil { - log.Errorf("Unable to lookup target %s: %s", l, err) + log.Errorf("Unable to lookup target: %w", err) continue } be.strategy.exportTarget(target) @@ -199,7 +212,7 @@ func (be *baseExporter) exportDependencies(target *core.BuildTarget) { // exportSources exports all files required by the target. func (be *baseExporter) exportSources(target *core.BuildTarget) { - for _, src := range target.AllBuildInputs() { + for src := range target.AllBuildInputs() { if _, ok := src.Label(); ok { continue // These will be handled as dependencies later } @@ -241,7 +254,7 @@ func (be *baseExporter) exportFiles(paths []string) { } // getTarget attempts to lookup a target in the build graph. -// This is a synchronous lookup and it assumes that [state.ForceParseEntirePackage] +// This is a synchronous lookup and it assumes that [state.ParseMetadata] // was enabled during the parse phase to guarantee that all required targets are pre-parsed. func (be *baseExporter) getTarget(label core.BuildLabel) (*core.BuildTarget, error) { target := be.state.Graph.Target(label) @@ -252,7 +265,7 @@ func (be *baseExporter) getTarget(label core.BuildLabel) (*core.BuildTarget, err } // getPackage attempts to lookup a package in the build graph. -// This is a synchronous lookup and it assumes that [state.ForceParseEntirePackage] +// This is a synchronous lookup and it assumes that [state.ParseMetadata] // was enabled during the parse phase to guarantee that all required packages are pre-parsed. func (be *baseExporter) getPackage(label core.BuildLabel) (*core.Package, error) { pkg := be.state.Graph.PackageByLabel(label) diff --git a/src/please.go b/src/please.go index 2551c9deb..f88fa5ebc 100644 --- a/src/please.go +++ b/src/please.go @@ -784,7 +784,7 @@ var buildFunctions = map[string]func() int{ return 0 }, "export": func() int { - success, state := runBuild(opts.Export.Args.Targets, buildOpts{ParseMetadata: true, ForceParseEntirePackage: true}) + success, state := runBuild(opts.Export.Args.Targets, buildOpts{ParseMetadata: true}) if success { export.Repo(state, opts.Export.Output, opts.Export.NoTrim, state.ExpandOriginalLabels()) @@ -1201,7 +1201,6 @@ func Please(targets []core.BuildLabel, config *core.Configuration, buildOpts bui state.NeedBuild = buildOpts.Build state.NeedTests = buildOpts.Test state.ParseMetadata = buildOpts.ParseMetadata - state.ForceParseEntirePackage = buildOpts.ForceParseEntirePackage state.NeedDebugDeps = debug // What outputs get downloaded in remote execution. @@ -1363,10 +1362,6 @@ type buildOpts struct { IsQuery bool // ParseMetadata is true if we want to store BUILD file metadata during parsing. ParseMetadata bool - // ForceParseEntirePackage is true if we want to force parse and activate all targets in every - // visited package. This is required to include adjacent targets in the build graph for operations - // like export. - ForceParseEntirePackage bool } // Runs the actual build diff --git a/src/plz/plz.go b/src/plz/plz.go index d1d1e209e..102e930e2 100644 --- a/src/plz/plz.go +++ b/src/plz/plz.go @@ -365,57 +365,73 @@ func inSamePackage(label, dependent core.BuildLabel) bool { // RecursiveParse is like Parse but recurses down into all dependencies of the target as well. func (r *runner) RecursiveParse(ctx context.Context, label, dependent core.BuildLabel) error { + if r.state.ParseMetadata && !r.isExternal(label) { + // If [state.ParseMetadata] is set, we need the entire package to be parsed and present in + // the graph so that all adjacent targets and their dependencies are available. Converting + // the label to its package pseudo-label (e.g. //pkg:target -> //pkg:all) routes execution + // to recursiveParsePackage below. + label = label.PackageLabel() + } if !label.IsAllTargets() { - return r.recursiveParse(ctx, label, dependent) + return r.recursiveParseTarget(ctx, label, dependent) } - pkg, err := r.Parse(ctx, label, dependent) + return r.recursiveParsePackage(ctx, label, dependent) +} + +// isExternal reports whether the label represents a target from an external repo. +func (r *runner) isExternal(label core.BuildLabel) bool { + if label.Subrepo == "" { + return false + } + return r.state.Graph.Subrepo(label.Subrepo).IsExternal() +} + +// recursiveParseTarget parses a target and, transitively, everything it depends on. +func (r *runner) recursiveParseTarget(ctx context.Context, label, dependent core.BuildLabel) error { + if !r.parseOnce.Add(label, struct{}{}) { + return nil // Someone else has this one; they're in the same errgroup so we needn't wait for them. + } + target, err := r.parseTarget(ctx, label, dependent) if err != nil { return err } g, gctx := r.group(ctx) - for _, target := range pkg.AllTargets() { - for dep := range target.DeclaredDependencies() { - g.Go(func() error { - // N.B. No need to deduplicate these; recursiveParse does that for the whole walk. - return r.recursiveParse(gctx, dep, target.Label) - }) - } + for dep := range target.DeclaredDependencies() { + g.Go(func() error { + return r.RecursiveParse(gctx, dep, target.Label) + }) } return g.Wait() } -// recursiveParse parses a target and, transitively, everything it depends on. -func (r *runner) recursiveParse(ctx context.Context, label, dependent core.BuildLabel) error { +// recursiveParsePackage parses all targets in a package. +func (r *runner) recursiveParsePackage(ctx context.Context, label, dependent core.BuildLabel) error { if !r.parseOnce.Add(label, struct{}{}) { return nil // Someone else has this one; they're in the same errgroup so we needn't wait for them. } - target, err := r.parseTarget(ctx, label, dependent) + pkg, err := r.Parse(ctx, label, dependent) if err != nil { return err } g, gctx := r.group(ctx) - for dep := range target.DeclaredDependencies() { + for _, target := range pkg.AllTargets() { g.Go(func() error { - return r.recursiveParse(gctx, dep, target.Label) + // N.B. No need to deduplicate these; recursiveParse does that for the whole walk. + return r.recursiveParseTarget(gctx, target.Label, label) }) } - if r.state.ParseMetadata { - // parseTarget can return as soon as the target itself turns up, before the rest of its package - // (and hence its metadata) has been parsed, so we have to make sure the whole package is done here. - pkg, err := r.Parse(ctx, target.Label, dependent) - if err != nil { - return err - } - if !pkg.Subrepo.IsExternal() { - related, err := pkg.Metadata.FindRelatedTargets(target.Label) - if err != nil { - return err - } - for _, rel := range related { - g.Go(func() error { - return r.recursiveParse(gctx, rel, target.Label) - }) - } + if r.state.ParseMetadata && !pkg.Subrepo.IsExternal() { + // When storing parse metadata we need to fully parse any packages of subincluded targets - + // including transitive subincludes - to have all their adjacent targets parsed as well. + // While `pkg`'s own parse already resolved the individual subinclude targets (e.g. + // //lib:a), adjacent targets in those packages (e.g. //lib:b) were not necessarily parsed. + // Passing each subinclude to RecursiveParse will expand it to its enclosing package label + // when ParseMetadata is set, ensuring those packages and their adjacent targets are fully + // parsed into the graph. + for _, subinc := range pkg.AllSubincludes(r.state.Graph) { + g.Go(func() error { + return r.RecursiveParse(gctx, subinc, label) + }) } } return g.Wait() diff --git a/test/export/test_notrim_parse_entire_package/BUILD b/test/export/test_notrim_parse_entire_package/BUILD new file mode 100644 index 000000000..02b0a5abf --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/BUILD @@ -0,0 +1,11 @@ +subinclude("//test/export:export_e2e_test_build_def") + +# Test --notrim exports adjacent targets and dependencies from packages that would not have been +# included in the original parse. This is a regression test to make sure we enforce entire package +# parsing when using the export command. +please_export_e2e_test( + name = "export_notrim_parse_entire_package", + enforce_different_repos = False, + export_targets = ["//:target1"], + flags = "--notrim", +) diff --git a/test/export/test_notrim_parse_entire_package/expected_repo/.plzconfig b/test/export/test_notrim_parse_entire_package/expected_repo/.plzconfig new file mode 100644 index 000000000..8e1ae5655 --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/expected_repo/.plzconfig @@ -0,0 +1,3 @@ +[Parse] + +BuildFileName = BUILD_FILE diff --git a/test/export/test_notrim_parse_entire_package/expected_repo/BUILD_FILE b/test/export/test_notrim_parse_entire_package/expected_repo/BUILD_FILE new file mode 100644 index 000000000..5c4fb397a --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/expected_repo/BUILD_FILE @@ -0,0 +1,9 @@ +genrule( + name = "target1", + srcs = [ + "file1.txt", + "//intermediate:lib", + ], + outs = ["out1.txt"], + cmd = "cat $SRCS > $OUT", +) diff --git a/test/export/test_notrim_parse_entire_package/expected_repo/file1.txt b/test/export/test_notrim_parse_entire_package/expected_repo/file1.txt new file mode 100644 index 000000000..e2129701f --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/expected_repo/file1.txt @@ -0,0 +1 @@ +file1 diff --git a/test/export/test_notrim_parse_entire_package/expected_repo/intermediate/BUILD_FILE b/test/export/test_notrim_parse_entire_package/expected_repo/intermediate/BUILD_FILE new file mode 100644 index 000000000..723c521bb --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/expected_repo/intermediate/BUILD_FILE @@ -0,0 +1,17 @@ +genrule( + name = "lib", + srcs = ["lib.txt"], + outs = ["out_lib.txt"], + cmd = "cat $SRCS > $OUT", + visibility = ["PUBLIC"], +) + +genrule( + name = "adjacent", + srcs = [ + "adjacent.txt", + "//other_package:other_target", + ], + outs = ["out_adjacent.txt"], + cmd = "cat $SRCS > $OUT", +) diff --git a/test/export/test_notrim_parse_entire_package/expected_repo/intermediate/adjacent.txt b/test/export/test_notrim_parse_entire_package/expected_repo/intermediate/adjacent.txt new file mode 100644 index 000000000..ac9595f5e --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/expected_repo/intermediate/adjacent.txt @@ -0,0 +1 @@ +adjacent content diff --git a/test/export/test_notrim_parse_entire_package/expected_repo/intermediate/lib.txt b/test/export/test_notrim_parse_entire_package/expected_repo/intermediate/lib.txt new file mode 100644 index 000000000..3cb05a1df --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/expected_repo/intermediate/lib.txt @@ -0,0 +1 @@ +lib content diff --git a/test/export/test_notrim_parse_entire_package/expected_repo/other_package/BUILD_FILE b/test/export/test_notrim_parse_entire_package/expected_repo/other_package/BUILD_FILE new file mode 100644 index 000000000..bd7b76ddd --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/expected_repo/other_package/BUILD_FILE @@ -0,0 +1,7 @@ +genrule( + name = "other_target", + srcs = ["file3.txt"], + outs = ["out3.txt"], + cmd = "cat $SRCS > $OUT", + visibility = ["PUBLIC"], +) diff --git a/test/export/test_notrim_parse_entire_package/expected_repo/other_package/file3.txt b/test/export/test_notrim_parse_entire_package/expected_repo/other_package/file3.txt new file mode 100644 index 000000000..7c8ac2f8d --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/expected_repo/other_package/file3.txt @@ -0,0 +1 @@ +file3 diff --git a/test/export/test_notrim_parse_entire_package/source_repo/.plzconfig b/test/export/test_notrim_parse_entire_package/source_repo/.plzconfig new file mode 100644 index 000000000..8e1ae5655 --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/source_repo/.plzconfig @@ -0,0 +1,3 @@ +[Parse] + +BuildFileName = BUILD_FILE diff --git a/test/export/test_notrim_parse_entire_package/source_repo/BUILD_FILE b/test/export/test_notrim_parse_entire_package/source_repo/BUILD_FILE new file mode 100644 index 000000000..5c4fb397a --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/source_repo/BUILD_FILE @@ -0,0 +1,9 @@ +genrule( + name = "target1", + srcs = [ + "file1.txt", + "//intermediate:lib", + ], + outs = ["out1.txt"], + cmd = "cat $SRCS > $OUT", +) diff --git a/test/export/test_notrim_parse_entire_package/source_repo/file1.txt b/test/export/test_notrim_parse_entire_package/source_repo/file1.txt new file mode 100644 index 000000000..e2129701f --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/source_repo/file1.txt @@ -0,0 +1 @@ +file1 diff --git a/test/export/test_notrim_parse_entire_package/source_repo/intermediate/BUILD_FILE b/test/export/test_notrim_parse_entire_package/source_repo/intermediate/BUILD_FILE new file mode 100644 index 000000000..723c521bb --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/source_repo/intermediate/BUILD_FILE @@ -0,0 +1,17 @@ +genrule( + name = "lib", + srcs = ["lib.txt"], + outs = ["out_lib.txt"], + cmd = "cat $SRCS > $OUT", + visibility = ["PUBLIC"], +) + +genrule( + name = "adjacent", + srcs = [ + "adjacent.txt", + "//other_package:other_target", + ], + outs = ["out_adjacent.txt"], + cmd = "cat $SRCS > $OUT", +) diff --git a/test/export/test_notrim_parse_entire_package/source_repo/intermediate/adjacent.txt b/test/export/test_notrim_parse_entire_package/source_repo/intermediate/adjacent.txt new file mode 100644 index 000000000..ac9595f5e --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/source_repo/intermediate/adjacent.txt @@ -0,0 +1 @@ +adjacent content diff --git a/test/export/test_notrim_parse_entire_package/source_repo/intermediate/lib.txt b/test/export/test_notrim_parse_entire_package/source_repo/intermediate/lib.txt new file mode 100644 index 000000000..3cb05a1df --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/source_repo/intermediate/lib.txt @@ -0,0 +1 @@ +lib content diff --git a/test/export/test_notrim_parse_entire_package/source_repo/other_package/BUILD_FILE b/test/export/test_notrim_parse_entire_package/source_repo/other_package/BUILD_FILE new file mode 100644 index 000000000..bd7b76ddd --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/source_repo/other_package/BUILD_FILE @@ -0,0 +1,7 @@ +genrule( + name = "other_target", + srcs = ["file3.txt"], + outs = ["out3.txt"], + cmd = "cat $SRCS > $OUT", + visibility = ["PUBLIC"], +) diff --git a/test/export/test_notrim_parse_entire_package/source_repo/other_package/file3.txt b/test/export/test_notrim_parse_entire_package/source_repo/other_package/file3.txt new file mode 100644 index 000000000..7c8ac2f8d --- /dev/null +++ b/test/export/test_notrim_parse_entire_package/source_repo/other_package/file3.txt @@ -0,0 +1 @@ +file3