Revert "[perf] Skip cache check if store path exists locally" #2054
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
This reverts commit ec0a703.
gcurtis
approved these changes
May 15, 2024
mikeland73
added a commit
that referenced
this pull request
May 16, 2024
## Summary Fixes bug described here #2054 A better (but a bit more involved) solution is to parallelize by output. Currently we parallelize by meta-output (which includes `__default_output__`). This has two downsides: * `__default_output__` can be multiple outputs, in which case we don't parallelize them at all. * We still do redundant work when `__default_output__` is multiple outputs. (once as `__default_output__` and once for each individual outout that forms part of defaults). ## How was it tested?
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.
Reverts #2042
#2042 introduced a bug where adding non-cached packages would fail to add them to the flake on the first attempt. (it would succeed on subsequent attempts after the package is already in the store).
The root cause is that
fetchNarInfoStatusgets called twice per package (even though we cache it and we're not supposed to call it twice). The first call returnsfalsebecause the package is not cached. The second call returnstruebecause the package is already in store. This discrepancy essentially causes the package not to appear on the flake at all. When updating state again, the package is already in the nix store so bothfetchNarInfoStatuscalls return true.I think reverting this is best immediate course of action. In follow up we should fix
fetchNarInfoStatusso it only gets called once (it will improve performance and is more correct).Later on we can think of better way to do #2042. The current implementation is a bit fragile and not 100% consistent with the initial intention of the function, so I'm concerned it can lead to more bugs in the future.