Fix: ensure full package and subincludes are parsed for export

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}

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.

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

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

src/export/export.go

@@ -102,55 +102,68 @@ 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()
 	be.exportTargets(targets)
 	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)

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

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()

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",
+)

test/export/test_notrim_parse_entire_package/expected_repo/.plzconfig

@@ -0,0 +1,3 @@
+[Parse]
+
+BuildFileName = BUILD_FILE

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",
+)

test/export/test_notrim_parse_entire_package/expected_repo/file1.txt

@@ -0,0 +1 @@
+file1

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",
+)

test/export/test_notrim_parse_entire_package/expected_repo/intermediate/adjacent.txt

@@ -0,0 +1 @@
+adjacent content

test/export/test_notrim_parse_entire_package/expected_repo/intermediate/lib.txt

@@ -0,0 +1 @@
+lib content