Improve the comments around the build target state #3217
Conversation
This file contains 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
toastwaffle
requested changes
Aug 12, 2024
| Active // Target is going to be used in current build | ||
| Pending // Target is ready to be built but not yet started. | ||
| Active // Target is going to be used in current build, but we're waiting for its dependencies to build | ||
| Pending // Target has been added to the build queue built but not yet started. |
There was a problem hiding this comment.
Suggested change
| Pending // Target has been added to the build queue built but not yet started. | |
| Pending // Target has been added to the build queue but not yet started. |
| Reused // Outputs of previous build have been reused. | ||
| Stopped // We stopped building the target because we'd gone as far as needed i.e. because we're only preparing the build dir for --shell | ||
| Built // Target has been successfully built. The target wasn't cached in any way. | ||
| Cached // Target has been retrieved from the build cache, but was not found in plz-out |
There was a problem hiding this comment.
but was not found in plz-out
It's unclear why this is relevant? Is this in comparison to Reused, where it was found in plz-out?
| Cached // Target has been retrieved from the cache | ||
| Unchanged // Target has been built but hasn't changed since last build | ||
| Reused // Outputs of previous build have been reused. | ||
| Stopped // We stopped building the target because we'd gone as far as needed i.e. because we're only preparing the build dir for --shell |
There was a problem hiding this comment.
Nit: is "we're only preparing the build dir for --shell" actually the only case where we'd see this statement?
If so, I suggest "We stopped building the target because we're only preparing the build dir for --shell"
If not, "i.e." should be "e.g."
| // The available states for a target. | ||
| // The available states for a target. States with higher integer values are always later on, or at the same point in the | ||
| // target lifecycle. This property is used to make sure we never transition a target backwards i.e. you can't go back | ||
| // to Active from Built. | ||
| const ( | ||
| Inactive BuildTargetState = iota // Target isn't used in current build |
There was a problem hiding this comment.
Nit: please add trailing full stops consistently
| Built // Target has been successfully built. The target wasn't cached in any way. | ||
| Cached // Target has been retrieved from the build cache, but was not found in plz-out | ||
| Unchanged // Target has been re-built (e.g. because the input or rule hash changed) but the outputs didn't change since last build | ||
| Reused // Outputs of previous build have been reused i.e. the outputs in plz-out were up-to-date | ||
| BuiltRemotely // Target has been built but outputs are not necessarily local. |
There was a problem hiding this comment.
What does "not necessarily local" mean? Not necessarily available in plz-out?
| Stopped // We stopped building the target because we'd gone as far as needed i.e. because we're only preparing the build dir for --shell | ||
| Built // Target has been successfully built. The target wasn't cached in any way. | ||
| Cached // Target has been retrieved from the build cache, but was not found in plz-out | ||
| Unchanged // Target has been re-built (e.g. because the input or rule hash changed) but the outputs didn't change since last build |
There was a problem hiding this comment.
Suggested change
| Unchanged // Target has been re-built (e.g. because the input or rule hash changed) but the outputs didn't change since last build | |
| Unchanged // Target has been re-built (e.g. because the input or rule hash changed) but the outputs didn't change since the last build |
goddenrich
approved these changes
Aug 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Sat down with @goddenrich to clarify what each of these mean. Thought I'd update the comments to make it easier to understand the life cycle of a target from this enum.