Analysis of a single configured target should be tolerant to Skyframe nulls #3785
Assignees
Labels
Comments
|
(Giving to @dslomov for Skylark triage/discussion) |
bazel-io
pushed a commit
that referenced
this issue
Sep 26, 2017
…computed value. Instead, manually check if the value has changed, and if it has, invalidate its consuming WorkspaceStatusValue node, forcing its re-evaluation, where it will pick up the new value. This seems more awkward than the original code, but it is more correct in spirit: injecting a precomputed value which can change even while the source state does not is a smell. Long-term, the key for the WorkspaceStatusValue should incorporate a hash of the action, and that hash should be in the configuration, just as other configuration flags are. That isn't possible right now just because we don't have configuration trimming, and we drop all nodes on configuration changes, so putting workspace status options into the configuration would lose change pruning whenever we changed workspace status options. If/when those problems are fixed, we can extend this change to have WorkspaceStatusFunction continue to request the action out-of-band, but keyed by the hash. Then we can stop invalidating stale nodes. See also #3785. PiperOrigin-RevId: 169947071
|
TODO is here . We have big use cases where the workspace status command actually changes on every build, so we'll need to figure something out here, probably via a further hack, since I don't know what the timeline is for a principled solution to this issue. |
|
Do you know if it's still an issue? I don't see the TODO. |
|
I thought everything was fixed by 0484053. |
|
Thanks! Please reopen if there's still an issue |
luca-digrazia
pushed a commit
to luca-digrazia/DatasetCommitsDiffSearch
that referenced
this issue
Sep 4, 2022
…ecomputed value. Instead, manually check if the value has changed, and if it has, invalidate its consuming WorkspaceStatusValue node, forcing its re-evaluation, where it will pick up the new value.
This seems more awkward than the original code, but it is more correct in spirit: injecting a precomputed value which can change even while the source state does not is a smell. Long-term, the key for the WorkspaceStatusValue should incorporate a hash of the action, and that hash should be in the configuration, just as other configuration flags are. That isn't possible right now just because we don't have configuration trimming, and we drop all nodes on configuration changes, so putting workspace status options into the configuration would lose change pruning whenever we changed workspace status options.
If/when those problems are fixed, we can extend this change to have WorkspaceStatusFunction continue to request the action out-of-band, but keyed by the hash. Then we can stop invalidating stale nodes.
See also bazelbuild/bazel#3785.
PiperOrigin-RevId: 169947071
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The TODO in SkyframeBuildView should be fixed. Currently, workspace status changes (even really benign ones like an environment variable changing) cause full re-analysis of all configured targets in the graph.
I think the biggest issue with it is going to be Skylark, since Skylark exposes these files via info_file and the similar version_file. So all callers would have to be tolerant to those functions returning null and restarting.
The text was updated successfully, but these errors were encountered: