Convert build parallelism to a more synchronous model - #3569
Merged
peterebden merged 8 commits intoSep 14, 2026
Merged
Conversation
peterebden
force-pushed
the
peter/sync-push-final
branch
from
August 17, 2026 08:59
7e7edfe to
d7b78da
Compare
peterebden
force-pushed
the
peter/sync-push-final
branch
from
August 25, 2026 10:58
fa3786b to
713ed97
Compare
This was referenced Aug 29, 2026
Collaborator
Author
|
Note to self: I think there might be something slightly off about debug tools somewhere (have observed this internally but I have little understanding yet of what the test in question is doing) |
Collaborator
Author
Nope not caused by this. I assumed a failure that obvious would be, but it seems broken on master. Fixed in #3593 |
toastwaffle
requested changes
Sep 9, 2026
peterebden
force-pushed
the
peter/sync-push-final
branch
from
September 9, 2026 16:19
c1ae6b0 to
8dfcf5d
Compare
toastwaffle
approved these changes
Sep 9, 2026
toastwaffle
approved these changes
Sep 10, 2026
toastwaffle
reviewed
Sep 14, 2026
| if v.Err != nil { | ||
| return v.Val, v.Err | ||
| } else if first { | ||
| defer func() { |
Contributor
There was a problem hiding this comment.
Please document the fact that this will recover if f panics
| } | ||
|
|
||
| // WaitForPackage drops the interpreter lock and waits for the given package to be parsed. Like | ||
| // WaitForSubrepo drops the interpreter lock and waits for the given package to be parsed. Like |
Contributor
There was a problem hiding this comment.
"given package" -> "given subrepo"
"to be parsed" -> "to be defined"?
|
|
||
| type runner struct { | ||
| tasks *errgroup.Group | ||
| ctx context.Context //nolint:containedctx |
Contributor
There was a problem hiding this comment.
Please justify why it's reasonable to store the context?
| if s != nil { | ||
| return nil | ||
| } | ||
| if _, err := r.parse(ctx, defining, dependent, true, wait); err != nil && !errors.Is(err, parse.ErrMissingBuildFile) { |
Contributor
There was a problem hiding this comment.
If we ignore ErrMissingBuildFile, doesn't that imply that this can return nil and the subrepo still is not available?
PeterNeiss
pushed a commit
to PeterNeiss/please
that referenced
this pull request
Sep 14, 2026
The fork's master carried the Windows port on top of 17.33.0. That same work, rebased onto upstream's master after thought-machine#3569 ("Convert build parallelism to a more synchronous model") and with plz update now downloading the whole release zip on Windows, is on windows-rebase. This merge takes windows-rebase's tree as it is, so master now matches what is headed upstream, while keeping the old history reachable instead of rewriting it.
PeterNeiss
pushed a commit
to PeterNeiss/please
that referenced
this pull request
Sep 14, 2026
Brings back the fork's 18.0.0 and 18.0.1 ChangeLog entries and milestone page, which the upstream-bound branch leaves out, and adds 18.1.0: the rebase onto thought-machine#3569, plz update downloading the whole zip on Windows, and --shell's slash-separated directory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AaLmkJrkt7eRJC9uPXvbKe
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This will (hopefully) make the parallelism easier to understand and avoids some of the weird edge cases we had before. The fan-out logic is now all in
src/plzand has been cut out ofsrc/core. The model there is quite a lot more synchronous - essentially it calls a function like Build or Test which builds all a target's dependencies, then that target, blocking until it's done.We can drop a few of the concepts we had before (like "parse mode", "resolving dependencies" and things like that) and we can lose the maps of "pending things" on the build state. It's simpler around things like subincludes because when we get there we just ask to build the thing, we don't have to ask it to parse then figure out at the end of that whether we should trigger things to build or not.
I ran the in-repo perf tests and they are actually faster than before. That's nice to see although it was not expected - it appears to be that we have fewer concurrently live goroutines because all the dependency resolution stuff is gone. I think it's a bit of a parse-only effect though.