Export: trimming enhancements by DuBento · Pull Request #3530 · thought-machine/please

DuBento · 2026-05-01T14:45:16Z

Enhancements to plz export, moving from a basic target-level trimming (using gc.RewriteFile) to build statement-level trimming, including only the required build rules and subincludes.
For consistency, we format all the exported BUILD files.

Changelog:

Introduced PackageMetadata to track the relationship between BUILD file statements, the targets they generate, and the subincludes they require. Made optional to avoid the overhead for most ops and enabled for the export.
Introduced ScopeMetadata to track object origins (subincluded labels) and statement tracking during the interpreter phase. Made optional to avoid the overhead for most ops and enabled for the export.
Refactored src/export/export.go to enforce better separation of the DefaultExporter (for trimming) and NoTrimExporter.
Added logic to parse BUILD files and selectively write back statements based on whether the generated targets are part of the export set.
Implemented "minimal subinclude" generation, which rewrites subinclude() calls to only include labels actually used by the exported targets.
Update and added some of the e2e to reflect the changes in implementation.
Moved trimming from a GC-based target removal to a full AST statement walk.

toastwaffle · 2026-05-11T09:24:36Z

+	state     *core.BuildState
+	targetDir string
+
+	exportedTargets map[*core.Package]map[core.BuildLabel]bool


Using a pointer as a key always makes me uncomfortable; can we avoid this?

More generally, could we avoid the nested map? core.BuildLabel includes the package name anyway, right?

We can probably avoid using the pointer has key by using the package label, but we will have to look up in the graph each time. I was using the pointer directly assuming some consistency of no repeated package instances. Should I use the string instead and lookup in the graph each time we want to use it?

The nested map is useful for looping though the exported target per each package (and for efficient verification of visited targets). What's your opinion, should I try to unnest?

I think using the pointer is a bug waiting to happen, and I don't think fetching the package itself will be too expensive (it's a map lookup protected by an RWLock). We should probably export packageKey and tuse that.

The nesting makes sense to keep, but add a comment?

This is comment refers to outdate code. I forgot to mention here but I did change this after your initial comment. It currently uses a build label directly and is not nested.

// exportedTargets maintains a record of the targets that have been exported so far. exportedTargets map[core.BuildLabel]bool

This integrated well with the visited check and I've added an explicit visitedPackages map for the implementation that requires it. I'll assume this aligns with your opinion and will not action on the comment.

toastwaffle

Nearly there!

As ever, comments phrased as questions probably imply a need for code comments

peterebden

I have some worries about crossing package responsibilities here - I get there's a lot more information we need to store to support this, and we can work through a bunch of that, but I think some of these changes like wanting to request parses from code post the actual build is a line we shouldn't cross (and I think maybe we don't have to).

… build targets

setting subincludes at package level instead of at target level

- register subinclude statements in the package metadata - filter subincludes label - export all non build_target related statements

this is no longer relevant for the symbol tracking since we mirror the implicit logic for the actual scope symbols.

…arser alive Replaces the fragile background-daemon "KeepParserRunning" parsing logic with a synchronous upfront-parsing design. When "ForceParseEntirePackage" is enabled, we queue and parse all other targets in a visited package exactly once at parse time.

… in ASP

DuBento · 2026-08-28T19:47:13Z

Comments addressed and ready for another review.

peterebden · 2026-08-31T07:34:33Z

+	// Give the waiters time to block on the wait channel
+	time.Sleep(10 * time.Millisecond)


this doesn't guarantee that they do start blocking

I've added some synchronisation to hopefully improve this, but I don't think we can avoid having a sleep. If you have any ideas, or prefer I remove the test, let me know.

peterebden · 2026-08-31T07:36:10Z

 	d.printLines(targets)
-	for _, line := range cli.CurrentBackend.Output() {
-		d.printf("${ERASE_AFTER}%s\n", line)
+	logs := cli.CurrentBackend.Output()


Maybe it's a bit late now but it would have been really nice to have changes like this split from the bulk of the export change here; it'd be easier to reason about a set of logging changes in isolation. I'm not really clear at the moment why you need to do this; I don't especially object to it but it doesn't seem like plz export has any particularly unique output requirements.

peterebden · 2026-08-31T07:46:17Z

+	// to its StatementMetadata. Refer to [StatementMetadata] for more details but this single
+	// mapping tracks the targets produced by the statement, the subincluded labels required for its
+	// interpretation, and other information.
+	statements *cmap.Map[BuildStatement, *StatementMetadata]


do you really need these to be cmaps? They are explicitly optimised for high concurrency but not low overhead, I didn't anticipate generating large numbers of them (in this case two per package).

Would a map-and-mutex not be adequate here? Or, from the comment, I'm a little unclear if the mutex is even required - they can only be written once and I assume you'd just be reading them later during the export operation?

We initially favoured the thread-safe type instead of a shared mutex. I eventually understood that the write phase is single threaded. We maintained the cmaps for consistency but I agree that it is a waste since it results in creating 8 maps + 8 mutexes per package and likely to include few objects in each map.
I believe it would be correct to have no locking whatsoever but I've refactored to include a RWlock for consistency and to be resistant to any future changes (or multi-threaded export). It's better than having cmaps and hopefully with minimal overhead for our current single threaded logic.

peterebden · 2026-08-31T07:49:52Z

+	// The intention is to finds all the subincluded labels required by the package but not used to
+	// generate targets. An example could be a variable declaration that depends on a subincluded value.
+	// We range over all interpreted statements that require any subincluded target. From those, we
+	// filter out the statements that generate targets and any explicit subinclude() statement calls.


Curious why you need this? I'm struggling a bit to see how this links to what's required for export to work

This is required to support the following examples (snippets from tests):

subinclude("//build_defs:versions_build_def") for version, name in VERSIONS.items(): pass # Trimmed during export

for file in glob(["file*.in"]): genrule( name = "target_" + file.removesuffix(".in"), srcs = [file], outs = [file.removesuffix(".in") + ".out"], cmd = "cp $SRCS $OUT", )

Since we are not trimming variables or for headers, we need to determine what else is required by the BUILD file but doesn't necessarily generate a build target.

in the current implementation the mutex is not required but for consistence (and because the overhead should be minimal) we added the support for a shared mutex

…for query metadata

this fixes an issue with running our //src/core:core export test on our CI runners that don't include the python binary

toastwaffle reviewed May 11, 2026

View reviewed changes

toastwaffle reviewed May 12, 2026

View reviewed changes

Comment thread src/parse/asp/interpreter.go Outdated

Comment thread src/parse/asp/objects.go Outdated

Comment thread src/parse/asp/interpreter.go Outdated

Comment thread src/parse/asp/interpreter.go Outdated

Comment thread src/parse/asp/targets.go Outdated

DuBento changed the title ~~Export: enhance trimming using build statement metadata~~ Export: trimming enhancements Jun 5, 2026

toastwaffle reviewed Jun 9, 2026

View reviewed changes

DuBento force-pushed the export-from-package-metadata branch 4 times, most recently from f6fbf1a to 7152109 Compare June 17, 2026 10:01

toastwaffle requested changes Jul 11, 2026

View reviewed changes

peterebden requested changes Aug 13, 2026

View reviewed changes

Comment thread src/core/state.go Outdated

Comment thread src/core/state.go Outdated

Comment thread src/core/package_metadata.go Outdated

peterebden reviewed Aug 13, 2026

View reviewed changes

Comment thread test/export/please_export_e2e_test.build_defs Outdated

duarte added 18 commits August 28, 2026 20:24

Package level metadata that includes a mapping of build statements to…

8d0ecc4

… build targets

export using BuildFileMetadata logic

c91668c

register caller scope for callstack-like traversal

1276403

bufio writer for build statements

836fff5

rewrite export logic into explicit flow

a505981

export targets related to build statement

d3e67bc

enrich target with subincludes by looping though all scopes

d09e7d0

separate buildstmt register from adding target

1fb4f18

select and write subincludes

b0c3c1e

setting subincludes at package level instead of at target level

skip statement for preloaded subincludes

3abc3cd

test: trim subincludes

68b61e0

rename file

aeb2e4a

use slices.collect and sort interface

1966979

rewrite export with 2 concrete interfaces: default and notrim

50110d3

notrim: full copy BUILD file and visit sources

65fd873

double new lines between targets

82f9587

suppress diff output when enforcing repo differences

00f617f

export by filtering the original BUILD file

47ab394

- register subinclude statements in the package metadata - filter subincludes label - export all non build_target related statements

duarte added 14 commits August 28, 2026 20:33

strip symbols

0931003

s/CleanUp/Cleanup

1cbfd18

s/unneded/unneeded

e6ff712

standardise use of "unused" in tests

ab48c30

comments for interpreter.go

7f0bdfc

test: target variable trimming

a081324

revert: track preloaded in interpreter

229ea4f

this is no longer relevant for the symbol tracking since we mirror the implicit logic for the actual scope symbols.

fix: add new lines at the end of files

10b3f05

revert one time variable

23b4f99

test: move complex go test to e2e

7ba7f18

lint: plz fmt

585d95a

fix: wrong reference to unused lib in e2e test

d32f1e6

refactor: decouple buildStatement from core by implementing interface…

7552be7

… in ASP

DuBento force-pushed the export-from-package-metadata branch from 3f140a1 to 7552be7 Compare August 28, 2026 19:42

go lint

ef3bd43

peterebden reviewed Aug 31, 2026

View reviewed changes

duarte added 6 commits September 2, 2026 17:10

unexport labelset

9b8ca27

refactor: use RWmutex instead of cmaps for package metadata

4f7c136

in the current implementation the mutex is not required but for consistence (and because the overhead should be minimal) we added the support for a shared mutex

refactor: introduce struct TrackedStatement instead of sorted slices …

0f073e4

…for query metadata

test: trimming of variables that include targets

0ae4949

test: refactor build error test for better consistency

2c4ae9f

refactor: subincludeArgs as iterator

ff407e4

toastwaffle approved these changes Sep 7, 2026

View reviewed changes

Comment thread src/core/package_metadata.go Outdated

Comment thread src/parse/asp/interpreter.go Outdated

Comment thread test/export/test_go_bin/source_repo/third_party/go/BUILD_FILE Outdated

Comment thread test/export/test_native_target_with_go_dep/source_repo/third_party/go/BUILD_FILE Outdated

peterebden approved these changes Sep 8, 2026

View reviewed changes

Comment thread src/core/package_metadata.go Outdated

duarte added 3 commits September 8, 2026 19:15

typos and unused code

7e37fb3

fix: graciously skip system dependencies when exporting sources

08f6d86

this fixes an issue with running our //src/core:core export test on our CI runners that don't include the python binary

doc: readme on the export process

341397f

peterebden merged commit 611567b into thought-machine:master Sep 9, 2026
16 checks passed

		// Give the waiters time to block on the wait channel
		time.Sleep(10 * time.Millisecond)

Conversation

DuBento commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toastwaffle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peterebden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DuBento commented Aug 28, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DuBento commented May 1, 2026 •

edited

Loading