Skip to content
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

Make nix-build --store whatever work #4387

Merged
merged 10 commits into from Jan 25, 2021

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Dec 20, 2020

Much of the logic for building is really about scheduling builds, not actually performing build steps. We really ought to separate build-derivation.cc to 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-build and then a copy to the cache, nix doesn'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

@colemickens
Copy link
Member

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 cache.nixos.org and my-own.cachix.org at the same time.

@Ericson2314
Copy link
Member Author

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 :).

@dpulls
Copy link

dpulls bot commented Dec 23, 2020

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 changed the base branch from master to 0.5-release December 23, 2020 22:40
@Ericson2314 Ericson2314 changed the base branch from 0.5-release to master December 23, 2020 22:40
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!
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.
src/libstore/store-api.hh Outdated Show resolved Hide resolved
Ericson2314 and others added 2 commits January 22, 2021 15:58
Thanks!

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
@Ericson2314
Copy link
Member Author

OK comments are fixed.

@edolstra edolstra merged commit 680d8a5 into NixOS:master Jan 25, 2021
@Ericson2314 Ericson2314 deleted the non-local-store-build branch January 25, 2021 15:11
@Ericson2314
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants