Fix selected outputs in the existing target eval cache - #57
Conversation
71495c1 to
b5a040c
Compare
b5a040c to
c6edca7
Compare
c6edca7 to
e4ccd76
Compare
6a47eb0 to
a845f0d
Compare
387e1f6 to
d0a5026
Compare
d0a5026 to
24c9577
Compare
24c9577 to
effda45
Compare
lilyinstarlight
left a comment
There was a problem hiding this comment.
Perf testing shows no issues as well. I'm fine with this approach. I'll let you decide on the nit, I don't feel all that strongly on it
There was a problem hiding this comment.
Actually gonna leave one more comment, this does make one more cache miss/hit difference:
A cold target would have no outputSpecified value and a warm target would now have outputSpecified = true
The fix would probably be to make whether to add outputName to cache based on whether outputSpecified is present/true and fix why derivationToValue is returning a Value that has a default output that isn't the first output in outputs (at least according to PR description, I haven't personally validated this)
effda45 to
28c73c6
Compare
@lilyinstarlight I don't think this is true in our case. afaict nixpkgs does this, but I don't think our caching logic does. |
Drop the redundant cachePayload switch; useCache already gates the cache write, and the selected-output fields were already forced either way. Check that cold and warm targets leave outputSpecified absent for both lazy output selection and a direct drv.dev selection. Keep caching the outputName because selecting an output does not add outputSpecified in this fork, and importing the recipe chooses the alphabetically first output rather than the original default. Assisted-By: devx/be2f8951-dcdd-4edb-800e-b2b04251a210
28f9271 to
f70e31d
Compare
A warm Tecnix target-cache hit can return the wrong output. For instance, the cache can return
deveven when the target selectedout. The existing cache remembers only the.drvpath—the build recipe—so importing it loses the original output selection.Reproduced on seven real World target IDs at commit
5ea0946b556c, using Tecnix9d05607feonx86_64-linux. Representative examples://system/scm/worldtree:rustBinout: executable binariesdebug: debug symbols//docs/world:bookout: combined HTML/PDF directoryhtml: HTML output only//system/aquifer/secrets-bootstrap:incidentLibgit2lib: runtime librarydev: development filesEach cold/warm pair used the same World revision and
.drvpath, with an isolated cache and logs confirming the warm hit. The docs target’s Nix string dependency context also changed fromouttohtml.This PR fixes that existing behavior. It stores
{drvPath, outputName}in a versioned result payload and reconstructs the selected output with its Nix string dependency information intact. Both fields are forced while source tracking is active, including a lazy output selector that does not change the recipe. Malformed payloads, unavailable recipes, and invalid output names fall back to evaluation.The dependency-blob version advances from 1 to 2. Existing eval cache databases require no manual migration or deletion: old records become cache misses and are replaced as normal evaluation repopulates the cache. Users will see a cold cache that warms through use. This invalidation is necessary because older records never tracked output-selection inputs and cannot safely serve even dependency-only queries. The SQLite filename and schema, cache eligibility, evaluator-version handling, and packaging remain unchanged.
Checks:
builtinsandgcfunctional suites plusnix-expr-tests: 3/3 passed.Revision identities and build metadata for future remote-cache reuse are handled separately in #58. This PR depends on #59 for SQLite in the isolated test environment. #58 is the next layer above this selected-output fix. Native stack order: #59 → #57 → #58. This PR adds no remote transport or revision-stamping machinery.