Make nix-build --store whatever work
#4387
Merged
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
Ericson2314
force-pushed
the
non-local-store-build
branch
2 times, most recently
from
5d32e68 to
52f8c6e
Compare
|
Does this support a scenario where you need to check against multiple stores? For me/#3964, it's important that I'm able to check against |
|
If you mean the thing I said would need a "layered store", no this PR doesn't, but that's a separate feature in my mind. We can always do it too. This PR is purely about "removing the weaknesses and restrictions that make additional features appear necessary", but "layered store" is probably necessary either way :). |
|
🎉 All dependencies have been resolved ! |
Trying to make sure it work with obscurers stores.
We downcast in a few places, this will be refactored to be better later.
Remote stores still override so the other end schedules.
Just a few small things needed fixing!
Ericson2314
force-pushed
the
non-local-store-build
branch
from
52f8c6e to
fed1237
Compare
Thanks @regnat and @edolstra for catching this and comming up with the solution. They way I had generalized those is wrong, because local settings for non-local stores is confusing default. And due to the nature of C++ inheritance, fixing the defaults is more annoying than it should be. Additionally, I thought we might just drop the check in the substitution logic since `Store::addToStore` is now streaming, but @regnat rightfully pointed out that as it downloads dependencies first, that would still be too late, and also waste effort on possibly unneeded/unwanted dependencies. The simple and correct thing to do is just make a store method for the boolean logic, keeping all the setting and key stuff the way it was before. That new method is both used by `LocalStore::addToStore` and the substitution goal check. Perhaps we might eventually make it fancier, e.g. sending the ValidPathInfo to remote stores for them to validate, but this is good enough for now.
edolstra
reviewed
Jan 22, 2021
edolstra
reviewed
Jan 22, 2021
edolstra
reviewed
Jan 22, 2021
edolstra
reviewed
Jan 22, 2021
edolstra
reviewed
Jan 22, 2021
Thanks! Co-authored-by: Eelco Dolstra <[email protected]>
Ericson2314
force-pushed
the
non-local-store-build
branch
from
7eab7b9 to
53a7095
Compare
|
OK comments are fixed. |
|
Thanks! |
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.
Much of the logic for building is really about scheduling builds, not actually performing build steps. We really ought to separate
build-derivation.ccto make that concept distinct, but for now we just hack it up with downcasts.Why? Well consider #3946 and #4364, an feature that's clearly desired. The crux of the matter is that with today's work flow of doing a local
nix-buildand then a copy to the cache,nixdoesn't know what the end goal is until it's too late. First schedules to have everything local, and then it copies it all back to the cache.With this workflow, the schedule knows which store is the end destination. And just as only things you don't have are sent to the remote builder, now only things that are not in the cache are sent to the local store to build.
There might still be some extra downloading: I wanted to add a "corrupt nars, still no-op cause nar info" test, but that didn't work. Even if there was, however, that's now just a matter of small tweaks to fix, but the architecture is fundamentally rectified to support this feature.
Progress on #3946 and #4364.
Depends on #4366