-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try to realise CA derivations during queryMissing #8622
Conversation
BTW an alternative approach I've thought of is trying to add some sort of "dry run" mode to the main builder logic. Then we wouldn't need effectively two implementations of the same thing. |
@Ericson2314 Hmm- IIRC So far as I understand, that approach would be quite a bit more extensive of a change with more conseqeunces, I think. At least, it's not something I would want to attempt to implement. What do you think to getting this in as a simpler fix first, or is that a no-go? The fix in this PR does make a dramatic difference for me as it stands, so I would be quite happy to see an improvement land soon even if there is a better/simpler implementation that could be done in the future which makes it obsolete. What do you think? Am I overestimating the difficulty of your proposal, is it something that conceivably go into the next version of nix? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
The logic looks essentially good, just a few things (that you mostly already noticed)
src/libstore/misc.cc
Outdated
if (!bfd.outputs.contains(outputName)) | ||
continue; | ||
|
||
bool found = false; | ||
for (auto &sub : getDefaultSubstituters()) { | ||
auto realisation = sub->queryRealisation({hash, outputName}); | ||
if (!realisation) | ||
continue; | ||
invalid.insert(realisation->outPath); | ||
found = true; | ||
} | ||
if (!found) { | ||
knownOutputPaths = false; | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this run in parallel (using the thread pool we already have)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this for a moment. There is already a level of paralleism at play, which is doPath
is running in a thread along other doPath
s. The paralellism benefit would be if each output's static output has could be queried in parallel for realisation. My following reasoning assumes I've understood the purpose of what you're suggesting.
I can see the potential benefit of that. I have derivations with hundreds of outputs. I like the software going fast.
However, if any of those outputs fail to realise, we must build the derivation. We potentially put in flight a lot of useless work in the negative case. In practice for most derivations, there are only a handful of outputs. Processing the static output hashes in serial allows us to stop early, and stopping early would save requesting the realisations when we have to build anyway. The most time that will be saved in the typical positive case would be 'O(typical # outputs)' of round trips, i.e. a few.
I also observe that for my usecase of tens of derivation outputs, the performance is not unreasonable as it stands.
Any other thoughts here?
Aside: I ran headlong into #6623 trying to get the test working. It would be really nice if |
Friendly one-month review ping. Please let me know if that's not appropriate, or if there is anything else I can do to help progress this. I currently have time to follow up for a short while, and less after that, but I am very keen to help see this behaviour improved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping @pwaller (and sorry for not coming back to this earlier).
Looks good, let's ship it!
(I've just force-pushed the branch to unstuck the CI) |
It's got a failing test, I could use a hand if you can see what's missing. I'm also suffering from a broken build[*]. Additionally, if I take a fresh clone of nix and bootstrap/configure it, I get:
The broken build I'm getting is that I'm unable to run my nix binary:
Not sure why this started failing. I have been playing with building nix using LLVM, but those experiments live entirely outside my current development environment and it's repeatable even when I do a make clean (in my old nix tree where configure doesn't fail...). Is this a problem of |
Aah, finally found it. LD_DEBUG=all to the rescue. The outputs directory was included via the runpath and had a contaminated shared object in it. Don't understand however why librapidcheck is now failing during configure. |
I just ran the tests on 32d3052 locally and can't reproduce the failure given by ubuntu-latest. https://github.com/NixOS/nix/actions/runs/5805240629/job/15736024216?pr=8622 @thufschmitt is this for me to dig into or can I leave it with you? I'm not sure where to begin right now, I guess I should try and reproduce in an ubuntu environment? |
This enables nix to correctly report what will be fetched in the case that everything is a cache hit. Note however that if an intermediate build of something which is not cached could still cause products to end up being substituted if the intermediate build results in a CA path which is in the cache. Fixes NixOS#8615. Signed-off-by: Peter Waller <[email protected]>
Head branch was pushed to by a user without write access
@thufschmitt I've introduced |
This enables nix to correctly report what will be fetched in the case
that everything is a cache hit.
Note however that if an intermediate build of something which is not
cached could still cause products to end up being substituted if the
intermediate build results in a CA path which is in the cache.
Fixes #8615.
Signed-off-by: Peter Waller [email protected]
Motivation
Context
See #8615. TL;DR: If you use content-addressed derivations, the behaviour of cache hits was poor, and nix would spend quite a bit of time to incorrectly report that things needed to be built when they were cache hits.
This is my first contribution to nix. I've applied some basic level of reasoning to the fix, but you should assume the logic of the first implementation is completely incorrect until proven otherwise: please help me to get it correct.
I also have not yet had time to write a test for this, which I intend to do, but I wanted to send the PR early to avoid wasting time on this if the approach is somehow wrong.
Please see the TODO notes within the PR and comment on them. I intend to remove these before taking the PR out of draft.
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*
Priorities
Add 👍 to pull requests you find important.