-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Allow to substitute FODs with different storeDir #14113
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
base: master
Are you sure you want to change the base?
Allow to substitute FODs with different storeDir #14113
Conversation
I'm converting this to Draft for now since I should probably
|
We already handle `sub->storeDir` being different from `storeDir` when substituting or copying; in particular, if the store path being copied is a content-addressed derivation with no references (which all FODs are), we can safely substitute. In such cases, the store paths are different only because of different `storeDir`'s; the content will be exactly the same. The only nitpick is that FODs are technically allowed to "reference" `$NIX_STORE_DIR` or similar in the output (which would make the output different on different nix stores), but I could not find any FODs in the wild actually doing this, and in any case this is a bug since it would just break when building with a different store dir. What this commit does is to pass through the `StorePath` field of the `nix-cache-info` file to the `nix::Store` corresponding to the substituter. It also handles restoring this field from the local narinfo cache, however this was also mostly done already and the only change necessary is to pass through the field from the cache to the store. Finally, this also adds the test for this behaviour, both checking that FODs can be substituted from different storeDirs, and that non-FODs can't. The main reason to do this is to allow using substituters as/instead of "hashed mirrors" [1], while using different Nix stores on different machines. The benefits of using substituters for this task are: * They do not require external dependencies like curl, speeding up your first world rebuild; * They are more generic, working with any fetcher rather than just fetchurl; * They work for both `outputHashMode`s rather than just `flat`; * They do not require somewhat hacky tooling [2] that tarballs.nixos.org relies on, instead reusing an already existing mechanism; * They remove unnecessary duplication, since cache.nixos.org stores all redistributable tarballs already. To see this change in action for yourself, run $ NIX_STORE=/tmp/nix/store nix build --store /tmp -j 0 --substituters https://cache.nixos.org nixpkgs#hello.src And observe how the archive is substituted instead of being fetched. [1]: https://github.com/NixOS/nixpkgs/blob/3ba94658f039035d17ff6b963e6fb4c66093ccf0/pkgs/build-support/fetchurl/mirrors.nix#L4 [2]: https://github.com/NixOS/nixpkgs/blob/3ba94658f039035d17ff6b963e6fb4c66093ccf0/maintainers/scripts/copy-tarballs.pl
bc66267
to
c28eab5
Compare
config.getHumanReadableURI(), | ||
value, | ||
storeDir); | ||
config.storeDir = value; |
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 would love to have some systematic stuff for local vs remote setting, fyi. E.g. if you explicitly do ?store=...
, you probably don't want the remote side to silently overwrite that.
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.
Hm, great point. I didn't even think about passing ?store=
manually! This diminishes the point of this PR, but I will try to amend it to introduce a way to "merge" local and remote settings.
public: | ||
|
||
const PathSetting storeDir_{ | ||
PathSetting storeDir_{ |
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.
From the first impression this seems like breaking a very important invariant of the stores (store directory is an inherent property of a store). Making storeDir mutable is not the right approach here.
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've tried to think of an alternative solution but fell short.
It would of course be possible to move the whole nix-cache-info
fetching into the constructor, but that seems even worse.
Do you have anything in mind?
Motivation
We already handle
sub->storeDir
being different fromstoreDir
whensubstituting or copying; in particular, if the store path being copied
is a content-addressed derivation with no references (which all FODs
are), we can safely substitute.
In such cases, the store paths are different only because of different
storeDir
's; the content will be exactly the same. The only nitpickis that FODs are technically allowed to "reference"
$NIX_STORE_DIR
or similar in the output (which would make the output different on
different nix stores), but I could not find any FODs in the wild
actually doing this, and in any case this is a bug since it would just
break when building with a different store dir.
What this commit does is to pass through the
StorePath
field ofthe
nix-cache-info
file to thenix::Store
corresponding to thesubstituter. It also handles restoring this field from the local narinfo
cache, however this was also mostly done already and the only change
necessary is to pass through the field from the cache to the store.
Finally, this also adds the test for this behaviour, both checking that
FODs can be substituted from different storeDirs, and that non-FODs
can't.
The main reason to do this is to allow using substituters as/instead of
"hashed mirrors" 1, while using different Nix stores on different
machines. The benefits of using substituters for this task are:
your first world rebuild;
fetchurl;
outputHashMode
s rather than justflat
;relies on, instead reusing an already existing mechanism;
redistributable tarballs already.
To see this change in action for yourself, run
$ NIX_STORE=/tmp/nix/store nix build --store /tmp -j 0 --substituters https://cache.nixos.org nixpkgs#hello.src
And observe how the archive is substituted instead of being fetched.
Context
Issues with this implementation
This PR makes
storeDir
field ofStoreDirConfig
non-const; this is needed to set it in theinit()
method ofBinaryCacheStore
, rather than at construction time.I can't see how this can possibly be avoided without adding a new field and refactoring almost everything to take it into account, but if I'm missing something please let me know!
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.