Skip to content

Fix: ensure full package and subincludes are parsed for export - #3621

Merged
toastwaffle merged 10 commits into
thought-machine:masterfrom
DuBento:notrim-missing-target
Sep 30, 2026
Merged

toastwaffle merged 10 commits into
thought-machine:masterfrom
DuBento:notrim-missing-target

Conversation

@DuBento

@DuBento DuBento commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Fixes an issue where plz export --notrim fails when an exported package contains unrequested adjacent targets with dependencies outside the primary build graph - meaning that it wasn't parsed by the initial Please build.

  • Fix: ensure we parse the entire package and the subincludes (+transitive subincludes) for each target visited whenever ParseMetadata is enabled.
  • Removed dead code ParseEntirePackage since previous changes preferred the use of ParseMetadata for this purpose.
  • Regression test added - export_notrim_parse_entire_package under test/export.

Comment thread src/core/build_target.go Outdated
Comment thread src/plz/plz.go Outdated
Comment on lines +369 to +371
// If [state.ParseMetadata] is set, we need the entire package (and hence its metadata) to be
// parsed.
label = label.PackageLabel()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly is this for?

There's some subtlety around what these things need to wait for on parsing. I think it's less of a big deal for RecursiveParse because it's only called in here though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ParseMetadata is enabled, then we parse all the targets in the package. Here we change the label from "//:something" to "//:all", this way we handle it as the normal case of parsing the entire package. Again this is required for the --notrim implementation since we'll need to have all targets (and their declared dependencies) available.

Comment thread src/plz/plz.go
if r.state.ParseMetadata && !pkg.Subrepo.IsExternal() {
// For storing parse metadata we have to make sure the whole package is done here - all targets
// but also the subincludes.
for _, subinc := range pkg.AllSubincludes(r.state.Graph) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly are we trying to solve here? At this point the package is parsed, and all its subincludes must be parsed for AllSubincludes to work? (or fundamentally for it to have finished parsing)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention is to parse any adjacent targets. In a --notrim implementation, we expect the entire package to be exported. For example, subinclude(//lib:a) we'll want to parse the entire //lib:all for any other targets, such as //lib:b.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I gotcha, thanks

@DuBento
DuBento force-pushed the notrim-missing-target branch from 6d4d603 to a1cf900 Compare September 30, 2026 10:45
@toastwaffle
toastwaffle merged commit abb7d8f into thought-machine:master Sep 30, 2026
16 checks passed
@peterebden peterebden mentioned this pull request Oct 1, 2026
peterebden added a commit that referenced this pull request Oct 1, 2026
Think this is a cross between #3621 and #3615 (we don't rebase
everything and re-test on merge so the latter looked like it was all
good to go when it really wasn't).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants